Update to Rails 8.1 - #361
Conversation
This includes setting config.load_defaults to 8.0 from 7.2, which is slightly out of order from the usual process.
Running bin/rails app:update generated a number of changes that were accepted without any modification. Those include: - Updates to public/error pages - Files in bin/ - Updated logging of filtered parameters (new filtered values) - Removed a comment block from assets.rb - Changes to the test environment initialization
After running bin/rails app:update, a few files were changed in a way
that needed some modification before committing. Those modifications
are in this commit, and include:
1. config/environments/development.rb
- Kept local logging overrides in place
2. config/environments/production.rb
- kept config.public_file_server.enabled, but behind an env var
- kept config.assets.compile = false
- kept the config.logger block intact, which adds a formatter prior
to tagging
3. config/initilizaers/cors.rb
- rejected the suggested removal of the block we're actually using
(this results in no actual change to the file)
4. config/puma.rb
- kept the barnes gem in place for additional reporting in Heroku
See: https://mitlibraries.atlassian.net/browse/TCO-78
https://github.com/heroku/barnes
This also adds a readme section describing the existing makefile command
Running bin/rails app:update generated a number of changes that were accepted without any modification. Those include: - Updates to public/error pages - Files in bin/ - Adding config/ci.rb - Adding a comment block in CSP initializer
After running bin/rails app:update, a few files were changed in a way
that needed some modifications before committing. Those modifications
are in this commit, and include:
1. config/environments/development.rb
- Kept local logging overrides in place
2. config/environments/production.rb
- kept config.public_file_server.enabled, but behind an env var
- kept config.assets.compile = false
- kept config.assume_ssl and config.force_ssl in place
- kept the config.logger block intact, which adds a formatter prior
to tagging
TODO: Check order of config.logger block against config.log_tags directive
2. config/initializers/cors.rb
- rejected the suggested removal of the block we're actually using
(this results in no change being made to this file)
2. config/puma.rb
- kept the barnes gem in place for additional reporting in Heroku
https://mitlibraries.atlassian.net/browse/TCO-78
https://github.com/heroku/barnes
- Comment out an unimplemented plugin that relies on an env var
(SOLID_QUEUE_IN_PUMA) we have never adopted
There was a problem hiding this comment.
Pull request overview
This PR upgrades the application from Rails 7.2 to Rails 8.1, updating framework defaults, dependencies, and some generated/config scaffolding to match the newer Rails version while keeping the app functional.
Changes:
- Bump Rails to
~> 8.1.0, update related gems (including theme + tooling), and switch app defaults toconfig.load_defaults 8.1. - Refresh model/test/fixture annotations and update developer tooling/docs (AnnotateRB, CI script, setup/dev scripts).
- Update public error pages and production/development/test environment configuration to Rails 8-era defaults.
Reviewed changes
Copilot reviewed 58 out of 65 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
Gemfile |
Upgrades Rails/theme; swaps annotate gem; relaxes some version constraints. |
Gemfile.lock |
Locks Rails 8.1 and updated dependency graph. |
config/application.rb |
Switches load_defaults to 8.1. |
config/environments/development.rb |
Updates dev defaults/logging/caching behavior to Rails 8-era template. |
config/environments/test.rb |
Updates test env defaults and header config. |
config/environments/production.rb |
Updates production defaults (static serving, SSL assumptions, logging). |
config/puma.rb |
Updates Puma config comments and adds Solid Queue note. |
config/ci.rb |
Adds Rails-style bin/ci configuration. |
bin/ci |
Adds CI runner entrypoint. |
bin/dev |
Adds a simple dev-server launcher. |
bin/setup |
Updates setup flow (optional reset; starts bin/dev unless skipped). |
bin/rubocop |
Minor comment capitalization tweak. |
config/initializers/filter_parameter_logging.rb |
Expands filtered parameters list (PCI/PII-like fields). |
config/initializers/content_security_policy.rb |
Adds commented nonce auto-insertion option (Rails template update). |
config/initializers/assets.rb |
Removes commented precompile boilerplate. |
Makefile |
Updates annotate target to use annotaterb. |
README.md |
Documents annotation workflow (make annotate, annotaterb). |
renovate.json |
Allows newer Ruby and Rails versions for Renovate updates. |
public/400.html |
Adds new Rails-style 400 error page. |
public/404.html |
Replaces default 404 page with Rails 8 template. |
public/406-unsupported-browser.html |
Replaces default 406 page with Rails 8 template. |
public/422.html |
Replaces default 422 page with Rails 8 template. |
public/500.html |
Replaces default 500 page with Rails 8 template. |
public/icon.svg |
Updates app icon SVG content/size. |
app/models/user.rb |
Updates model annotation header (columns/indexes). |
app/models/term.rb |
Updates model annotation header (columns/indexes/FKs). |
app/models/suggested_resource.rb |
Updates model annotation header (indexes/FKs). |
app/models/suggested_pattern.rb |
Updates model annotation header (indexes/FKs). |
app/models/search_event.rb |
Updates model annotation header (indexes). |
app/models/metrics/algorithms.rb |
Updates model annotation header (column ordering). |
app/models/journal.rb |
Updates model annotation header (indexes). |
app/models/fingerprint.rb |
Updates model annotation header (indexes). |
app/models/detector.rb |
Updates model annotation header (indexes). |
app/models/detector_category.rb |
Updates model annotation header (indexes/FKs). |
app/models/detection.rb |
Updates model annotation header (indexes/FKs). |
app/models/confirmation.rb |
Updates model annotation header (indexes/FKs). |
app/models/category.rb |
Updates model annotation header (indexes). |
app/models/categorization.rb |
Updates model annotation header (indexes/FKs). |
test/models/user_test.rb |
Updates schema annotation header. |
test/models/term_test.rb |
Updates schema annotation header. |
test/models/suggested_resource_test.rb |
Updates schema annotation header. |
test/models/suggested_pattern_test.rb |
Updates schema annotation header. |
test/models/search_event_test.rb |
Updates schema annotation header. |
test/models/metrics/algorithms_test.rb |
Updates schema annotation header. |
test/models/journal_test.rb |
Updates schema annotation header. |
test/models/fingerprint_test.rb |
Updates schema annotation header. |
test/models/detector_test.rb |
Updates schema annotation header. |
test/models/detection_test.rb |
Updates schema annotation header. |
test/models/confirmation_test.rb |
Updates schema annotation header. |
test/models/category_test.rb |
Updates schema annotation header. |
test/models/categorization_test.rb |
Updates schema annotation header. |
test/fixtures/users.yml |
Updates schema annotation header. |
test/fixtures/terms.yml |
Updates schema annotation header. |
test/fixtures/suggested_resources.yml |
Updates schema annotation header. |
test/fixtures/suggested_patterns.yml |
Updates schema annotation header. |
test/fixtures/search_events.yml |
Updates schema annotation header. |
test/fixtures/journals.yml |
Updates schema annotation header. |
test/fixtures/fingerprints.yml |
Updates schema annotation header. |
test/fixtures/detectors.yml |
Updates schema annotation header. |
test/fixtures/detector_categories.yml |
Updates schema annotation header. |
test/fixtures/detections.yml |
Updates schema annotation header. |
test/fixtures/confirmations.yml |
Updates schema annotation header. |
test/fixtures/categorizations.yml |
Updates schema annotation header. |
test/fixtures/categories.yml |
Updates schema annotation header. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| main article br { | ||
| display: none; | ||
|
|
||
| @media(min-width: 48em) { | ||
| display: inline; | ||
| } | ||
| } |
There was a problem hiding this comment.
This comment applies to all the feedback recieved about these public/*.html templates - I don't think Copilot is correct in flagging this:
From: https://developer.mozilla.org/en-US/docs/Web/CSS/Guides/Nesting/At-rules
Any at-rule whose body contains style rules can be nested inside another style rule using CSS nesting. Style rules nested inside at-rules take their nesting selector definition from the nearest ancestor style rule.
That article specifically includes examples of @media queries being nested inside selector rules.
Additionally, inspecting these pages as served in the review app, I see this rule taking effect at different page sizes on the 406-unsupported-browser example page.
| main article br { | ||
| display: none; | ||
|
|
||
| @media(min-width: 48em) { | ||
| display: inline; | ||
| } | ||
| } |
| main article br { | ||
| display: none; | ||
|
|
||
| @media(min-width: 48em) { | ||
| display: inline; | ||
| } | ||
| } |
| main article br { | ||
| display: none; | ||
|
|
||
| @media(min-width: 48em) { | ||
| display: inline; | ||
| } | ||
| } |
| main article br { | ||
| display: none; | ||
|
|
||
| @media(min-width: 48em) { | ||
| display: inline; | ||
| } | ||
| } |
| # Change to :null_store to avoid any caching. | ||
| config.cache_store = :memory_store |
There was a problem hiding this comment.
This feedback is at odds with the Rails documentation on caching in development: https://guides.rubyonrails.org/caching_with_rails.html#caching-in-development
| # Disable serving static files from `public/`, relying on NGINX/Apache to do so instead. | ||
| config.public_file_server.enabled = true | ||
| config.public_file_server.enabled = ENV['RAILS_SERVE_STATIC_FILES'].present? | ||
|
|
There was a problem hiding this comment.
After looking at our Heroku config and reasoning through this feature a bit more, I'm walking back this change. the public file server will be set to always be enabled in environments which use the production config.
Gating the public file server behind an env var was flagged during automated review - which was made more relevant by seeing that this env isn't defined in the live tier at the moment. This walks back from that gating choice, and commits to always keeping the public file server enabled in environments that use the production config.
jazairi
left a comment
There was a problem hiding this comment.
I'd suggest including the Rails health check route (as in here). Other suggestions are non-blocking.
| # config.action_dispatch.x_sendfile_header = "X-Accel-Redirect" # for NGINX | ||
|
|
||
| # Store uploaded files on the local file system (see config/storage.yml for options). | ||
| config.active_storage.service = :local |
There was a problem hiding this comment.
Kinda weird that we've always had this set to local, but I don't see any need to change it since we don't store files in TACOS. 🤷♀️
There was a problem hiding this comment.
Yeah, if we were actually going to use this, it would need to be some other value (like ETD is). It looks like TIMDEX leaves out this directive entirely, while TIMDEX-UI also has it set to local.
I'm not opposed to having a coordinated value for these, but I think it'd be best to tackle that specifically, and not here.
|
|
||
| step "Style: Ruby", "bin/rubocop" | ||
|
|
||
| step "Security: Importmap vulnerability audit", "bin/importmap audit" |
There was a problem hiding this comment.
This can be commented out, since we're not using importmaps. (I think? We don't in our other repos, at least.)
There was a problem hiding this comment.
I noticed that this had been flagged in one of the other Rails 8 upgrades, and wasn't sure whether to disable or not I'm happy to take it out in the name of consistency - I agree that we don't use them in this app, at least.
| end | ||
|
|
||
| # Run the Solid Queue supervisor inside of Puma for single-server deployments. | ||
| # plugin :solid_queue if ENV["SOLID_QUEUE_IN_PUMA"] |
There was a problem hiding this comment.
Fwiw, we have this line uncommented in our other repos, but it's probably okay to leave it commented out since I don't think we're actually using it yet.
- Comment out importmap audit from config/ci because we don't use them - Uncomment the solid_queue_in_puma config to maintain pairity with other apps - Move the health check route to the top of config/routes, along with one other documentation comment
|
Hey @jazairi - I made a number of the changes you mentioned. I got confused when adding the health check route broke a bunch of tests, but then noticed that it had already been added about halfway down the routes file (I swear I'd added it during the upgrade at one point, but the blame report for this file indicates that it's been here for a while). Hopefully re-organizing the routes file to put it at the top will help with consistency and not confuse future-me so much. |
|
@matt-bernhardt Ah, that explains why the routes file wasn't touched. Sorry, should've checked the current state! |
|
No worries! I saw your requested change, and thought "wait, I thought I did add that" - and then saw that it wasn't in the diff, and thought maybe I slow-walked it for some reason. It wasn't until I then added it and saw things break that I realized it was already there, and had been for some time. So, I definitely missed that it was already there too. GMTA :-) |
This incrementally upgrades the application from 7.2 to 8.0 and then on to 8.1, following the general pattern I see from Isra and Jeremy doing this for the other Rails apps.
Ticket(s)
https://mitlibraries.atlassian.net/browse/TCO-216