Skip to content
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

Upgrade to Rails 7 #570

Merged
merged 1 commit into from
Aug 22, 2022
Merged

Upgrade to Rails 7 #570

merged 1 commit into from
Aug 22, 2022

Conversation

jazairi
Copy link
Contributor

@jazairi jazairi commented Aug 19, 2022

Why these changes are being introduced:

We want to stay up to date with the latest Rails.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/ENGX-155

How this addresses that need:

This upgrades to Rails 7 and loads the new framework defaults.
Prior to this, I ran rails app:update, checked the diffs for all
files, and confirmed that the new framework defaults would not
cause problems. In this case, it seemed like there were no Rails
files that needed to be updated, so I kept the existing files.

Side effects of this change:

  • The way we handle domain redirects
    (ApplicationController#ensure_domain) now throws an unsafe
    redirect error. I've added the allow_other_host param to that
    method to bypass this. In this case the other host would come
    from env, so the risk seems low, but I'd encourage the reviewer
    to consider any edge cases I may have missed.
  • The only new framework default that seemed like it could cause
    problems for us was the key generator digest changing to SHA256:
    https://guides.rubyonrails.org/v7.0/upgrading_ruby_on_rails.html#key-generator-digest-class-changing-to-use-sha256.
    However, I don't think we are using encrypted cookies
    in this app, so I don't anticipate problems there.

Developer

  • All new ENV is documented in README
  • All new ENV has been added to Heroku Pipeline, Staging and Prod
  • ANDI or Wave has been run in accordance to
    our guide and
    all issues introduced by these changes have been resolved or opened as new
    issues (link to those issues in the Pull Request details above)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Requires database migrations?

NO

Includes new or updated dependencies?

YES

@mitlib mitlib temporarily deployed to timdex-pr-570 August 19, 2022 20:10 Inactive
Why these changes are being introduced:

We want to stay up to date with the latest Rails.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/ENGX-155

How this addresses that need:

This upgrades to Rails 7 and loads the new framework defaults.
Prior to this, I ran `rails app:update`, checked the diffs for all
files, and confirmed that the new framework defaults would not
cause problems. In this case, it seemed like there were no Rails
files that needed to be updated, so I kept the existing files.

Side effects of this change:

* The way we handle domain redirects
(`ApplicationController#ensure_domain`) now throws an unsafe
redirect error. I've added the `allow_other_host` param to that
method to bypass this. In this case the other host would come
from env, so the risk seems low, but I'd encourage the reviewer
to consider any edge cases I may have missed.
* The only new framework default that seemed like it could cause
problems for us was the key generator digest changing to SHA256:
https://guides.rubyonrails.org/v7.0/upgrading_ruby_on_rails.html#key-generator-digest-class-changing-to-use-sha256.
However, I don't think we are using encrypted cookies in this app,
so I don't anticipate problems there.
@mitlib mitlib temporarily deployed to timdex-pr-570 August 19, 2022 20:43 Inactive
@JPrevost JPrevost self-assigned this Aug 22, 2022
Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind checking if we already have a ticket to update the dockerfile and if not creating one? It looks more out of date than the scope of this work so I'm not asking for it to be included here, but I think a TIMX ticket to update the various docker bits for this would be good if we don't have one yet.

I think this is the first major rails upgrade that didn't require deprecation cleanup... I'm betting some of our other apps won't be so clean :)

:shipit:

app/controllers/application_controller.rb Show resolved Hide resolved
@jazairi
Copy link
Contributor Author

jazairi commented Aug 22, 2022

Thanks, @JPrevost. I opened that ticket here: https://mitlibraries.atlassian.net/browse/TIMX-86

@jazairi jazairi merged commit a391e67 into main Aug 22, 2022
@jazairi jazairi deleted the engx-155-rails-7 branch August 22, 2022 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants