Ap 587 update dependencies by davezuckerman · Pull Request #21 · BerkeleyLibrary/UCBEARS

davezuckerman · 2026-02-25T18:20:05Z

I ran into issues with the build. It was running out of memory during the tests so I had to rework the test section of build.yml. I'm not sure why this is using so much memory though. Any thoughts? This currently builds but it won't using the previous build.yml

…se.ci

…recompile and db:prepare

…ed memory for selenium

…ifferent step

…p early

… building during test

…r tests

…sue running tests

…zer for post only

awilfox

There are some things to review and fix (see inline comments), but overall this looks good.

I have a few general/overview questions as well:

Was bundle exec rails app:update run? The lack of changes in config/ surprises me.

Unfortunately, that is usually the most time-consuming part of updating Rails apps when I was doing it. The format/style doesn't match ours at all, so you need to use d (diff) and apply the relevant changes manually.
For the memory issues seen in CI, I'm wondering if adding this to config/application.rb would fix it:
```
config.yjit = false
```
If it does, I would hazard that we could be more specific; perhaps putting it behind if ENV['CI'] or in config/environments/test.rb. Still worth a try to see if it fixes it, though.
Do we want to mix the Twitter removal ticket with the dependency updates?

awilfox · 2026-02-25T23:29:43Z

app/controllers/items_controller.rb

  # Only allow a list of trusted parameters through.
  def item_params
-    params.require(:item).permit(:directory, :title, :author, :copies, :active, :publisher, :physical_desc, term_ids: [])
+    params.expect(item: [:directory, :title, :author, :copies, :active, :publisher, :physical_desc, { term_ids: [] }])


To be clear, this change means all of these fields are now required. Is this desired? (permit will allow any of them to be blank, but allows them through; expect requires all listed parameters to have a value.)

I thought params.expect allows for empty values but simply enforces the structure in a more modern way.
That was a rubocop suggestion.

app/lib/lending/borrower_token.rb

app/controllers/sessions_controller.rb

app/controllers/lending_controller.rb

app/views/sessions/new.html.erb

awilfox · 2026-02-26T00:04:11Z

docker-compose.yml

    environment: &app-environment
      LIT_LENDING_ROOT: spec/data/lending
      LIT_IIIF_BASE_URL: http://iipsrv.test/iiif/
+      #LIT_IIIF_BASE_URL: http://localhost/iiif/


Was this a leftover from testing?

Yes, when running it locally the viewer wasn't rendering the images unless I set the base URL to http://localhost/iiif. I'm not sure why it's set to http://iipsrv.test/iiif/. I can take that out and change a test that looks to look for http://localhost/iiif instead?

Gemfile

awilfox · 2026-02-26T00:06:47Z

docker-compose.ci.yml

 services:
  app:
    build: !reset
+    command: sleep infinity


This should be unnecessary.

It fails with a 137 (out of memory) in the test stage of the build without it. I tried it with config.yjit = false as well. Not sure why it's using so much memory for testing.

…nfity from compose.ci

…date to rails 8

…thout it

awilfox

Glad to see rails app:update wasn't quite the heavy diff I feared it could be. There's a few things we should discuss before merging.

config/environments/development.rb

awilfox · 2026-02-27T00:46:29Z

config/environments/production.rb

+  # Log to STDOUT with the current request id as a default log tag.
  config.log_tags = [:request_id]
+  config.logger   = ActiveSupport::TaggedLogging.logger($stdout)


I don't think we want this in prod?

That's a new default in rails 8. It wraps the standard logger and adds some context like request_id etc..

Should I take this out?

config/environments/production.rb

awilfox · 2026-02-27T00:50:24Z

config/puma.rb

-# This directive tells Puma to first boot the application and load code
-# before forking the application. This takes advantage of Copy On Write
-# process behavior so workers use less memory.
+# The default is set to 3 threads as it's deemed a decent compromise between


Suggested change

# The default is set to 3 threads as it's deemed a decent compromise between

# The default is set to 5 threads as it's deemed a decent compromise between

Documentation should reflect reality - this caught me when I did Framework's update, too.

Good catch. I saw that framework was using 5 so I bumped it up here as well but missed the default comment. Will change that

awilfox · 2026-02-27T00:55:28Z

Gemfile

-gem 'okcomputer', '~> 1.19', '>= 1.19.1'
-gem 'omniauth-cas', '~> 2.0'
-gem 'pagy', '~> 5.6'
+gem 'nokogiri', '~> 1.18.0'


Why are we explicitly specifying a dependency on nokogiri? I don't see any explicit usage of it in the app.

Is this a version pin for a security vulnerability? This should be documented if so, as I would remove this while doing other maintenance.

It's used by some gems the app uses. I can remove it and let those gems handle it.

awilfox · 2026-02-27T00:55:42Z

Gemfile

-gem 'pagy', '~> 5.6'
+gem 'nokogiri', '~> 1.18.0'
+gem 'non-digest-assets', '~> 2.6.0'
+gem 'observer', '~> 0.1.2'


Same question as above re: nokogiri.

factory-bot needs observer but it looks like I can remove non-digest-assets and replace it with mutex_m which is used. It's currently brought in via non-digest-assets but I'll remove that and so the app uses mutex_x directly

awilfox · 2026-02-27T00:55:54Z

Gemfile

-gem 'ruby-vips', '~> 2.0'
-gem 'sprockets-rails', '~> 3.4'
-gem 'typesafe_enum', '~> 0.3'
+gem 'psych', '~> 2.0.10'


And same question here; we don't seem to use psych anywhere directly.

That can be removed

config/initializers/assets.rb

…rectly to calnet

…d static page instead of direct redirect to CAS

David Zuckerman added 21 commits February 20, 2026 16:10

upgrades for Rails 8

7b40fd3

dependency updates

86fa80b

Pagy uses pagy.limit now for number of items on page

5aec166

updating depencies, moving to rails 8.0.4, ruby 3.4 and pagy 43

1b1651c

increased memory for tests

16c1a70

increased memomory for app for tests, only run tests if setup succeeds

5232e92

changed based url for IIF server, removed memory allocation for compo…

f38e87f

…se.ci

using selenium stand alone chrome, broke out steps in build.yml for p…

29d8df3

…recompile and db:prepare

bringin up selenium after assets are precompiled, lowering the reserv…

e81dbe2

…ed memory for selenium

bypassing yarn build -watch in ci since assets are precompiled in a d…

26b174c

…ifferent step

only start selenium before rspec needs it

5a8f763

bringing up compose without dependencies so sellenium doesn't start u…

947f3bc

…p early

running precompile as a standalone. hope this fixes memory issue when…

d36596b

… building during test

using dummy key_base for precompile

1e27edf

for rspec need to have iipsrv, app and selenium running

70dcef9

for rspec need to have iipsrv, app and selenium running

ce23cd9

need bundle exec when calling rspec and rubocop

027e216

full rails server not needed for tests, using slimmed down version fo…

6186f89

…r tests

running system tests as stand alone

b85b856

adding sleep infinity to docker-compose.ci, hopefully fixes memory is…

2558ccd

…sue running tests

refactored do_profile in application_controller

81d2b19

davezuckerman requested review from anarchivist and danschmidt5189 February 25, 2026 18:20

Added omniauth-rails_csrf_protection gem and updated omniauth intiali…

8f8d748

…zer for post only

davezuckerman requested a review from awilfox February 25, 2026 19:16

removed twitter/x logo from footer and deleted icon

44c2021

awilfox requested changes Feb 26, 2026

View reviewed changes

David Zuckerman and others added 3 commits February 26, 2026 07:43

removed commented out code, set config.yjit false and removed sleep i…

9538275

…nfity from compose.ci

ran rails app:update and made necessary changes to accommodate the up…

20f72a5

…date to rails 8

added sleep infinity back in to compose.ci, tests failing in build wi…

402beb9

…thout it

awilfox requested changes Feb 27, 2026

View reviewed changes

davezuckerman added 2 commits February 26, 2026 17:46

changed unauthorized user redirect so it goes to a page instead of di…

ab39ba3

…rectly to calnet

removed some Gem dependencies, changed a comment in puma config, adde…

7413da5

…d static page instead of direct redirect to CAS

	# The default is set to 3 threads as it's deemed a decent compromise between
	# The default is set to 5 threads as it's deemed a decent compromise between

Conversation

davezuckerman commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awilfox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awilfox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

davezuckerman commented Feb 25, 2026 •

edited

Loading