-
Notifications
You must be signed in to change notification settings - Fork 662
AO3-7231 Upgrade gems and configs to Rails 8.0.x #5515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Looked at all uses of to_time: * used for duration (difference, so zone vs offset doesn't matter) * stored in model/DB (rails converts to UTC) * read from model and sent to akismet (rails converts to UTC) * or just forced to utc strict_freshness: I don't see any issues arising from it Regexp.timeout: in my mind, 1s should be sufficient
better safe than sorry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upgrade script wanted puma.rb, but I ignored it per this comment.
It also wanted to set silence_healthcheck_path in production.rb but it doesn't look like we're using the system anyways. (Noting this just in case we want something silenced, but I doubt it.)
|
|
||
| # Library for helping run pt-online-schema-change commands: | ||
| gem "departure", "~> 6.8" | ||
| gem "departure", "~> 8.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't see anything of note in the changelog. Note that 8.0.1 seems to have not been published to rubygems (if it does get published I would like to pull it in).
|
|
||
| group :test do | ||
| gem "rspec-rails", "~> 6.0" | ||
| gem "rspec-rails", "~> 8.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelog, nothing stood out as well.
| # Raise exceptions for disallowed deprecations. | ||
| config.active_support.disallowed_deprecation = :raise | ||
|
|
||
| # Tell Active Support which deprecation messages to disallow. | ||
| config.active_support.disallowed_deprecation_warnings = [] | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed from example config, we're not using it anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this file is needed since the app should be able to respond itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These get copied to the Nginx proxy/frontend servers, so if memory serves, they can respond with this rather than going to the backend for it
The setting will be undeprecated and enabled by default in Rails 8.2 https://redirect.github.com/rails/rails/commit/a477a3273c3c71305cc8ae1835638dc75184ad9d
brianjaustin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor things, and I'd like to see testing steps in the ticket before approving. Looks good, generally, though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These get copied to the Nginx proxy/frontend servers, so if memory serves, they can respond with this rather than going to the backend for it
brianjaustin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking as requesting changes for the to_time thing, regex complexity manual test and rails-i18n upgrade, rest of my comments are optional to address
the only thing that is different is trailing newlines - looks like it was done in otwcode#5277
Bilka2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Issue
https://otwarchive.atlassian.net/browse/AO3-7231
Purpose
Upgrade to Rails 8.0.
The upgrade was surprisingly (suspiciously?) straightforward, considering it's a major version bump. I'll add additional notes/comments in a review.
Testing Instructions
TBD, see Jira.
Credit
marcus8448 (he/him)