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 Rails to 7.1.1 #1322

Closed
wants to merge 19 commits into from
Closed

Upgrade Rails to 7.1.1 #1322

wants to merge 19 commits into from

Conversation

kartiki975
Copy link
Contributor

@kartiki975 kartiki975 commented Jan 22, 2024

What does this PR change?

This PR involves a lot of changes. I highly suggest reviewing changes commit by commit.

  • Upgraded Rails to 7.1.1
  • Realized that Rails upgrade was no longer compatible with the Ruby versions that were supported
  • Upgrade Ruby to 3.2.0
  • Upgraded Rubocop to be compatible with the Ruby upgrade
  • Fix encrypted fields that were no longer being decrypted successfully: d09368c
  • Create mandatory storage file - will not affect production
  • Misc:
    • get rid of deprecations
    • swab pry with pry-byebug gem

Focus for reviewers

Due to some limitations, I had to manually do this upgrade. Please carefully these changes!
Most important of all is the encryption algorithm change. Reading through the section, it doesn't seem anything applies to this repo since config.active_support.key_generator_hash_digest_class was never manually configured. If someone could give that a check, please leave 👍!

Aside

Rails 7.2 is expected to officially deprecate Rails.application.secrets in favor of Rails.application.credentials. This will be handled in a follow up PR.

@kartiki975 kartiki975 requested a review from a team January 24, 2024 22:03
@kartiki975 kartiki975 changed the title Temp Upgrade Rails to 7.1.1 Jan 24, 2024
@kartiki975 kartiki975 marked this pull request as ready for review January 25, 2024 20:27
@kartiki975
Copy link
Contributor Author

@casperisfine Hey, Jean! I had to manually upgrade rails and a bunch of other dependencies for some internal Shopify ticket which is due tomorrow. Could you please take a look at these changes?

s.add_dependency('pubsubstub', '~> 0.2.0')
s.add_dependency('rails', '~> 7.0.0')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure whether this change is necessary. If it is necessary, should I also specify the ruby upgrade? 🤔

Copy link
Contributor

@casperisfine casperisfine left a comment

Choose a reason for hiding this comment

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

I'm sorry but there is just too much totally unrelated changes caused by rubocop to reasonably review this.

Please do a PR that only upgrade Rails, and dont' upgrade rubocop nor drop support for Ruby versions.

Comment on lines -110 to -112
- '2.7'
- '3.0'
- '3.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep some reasonable backward compatibility. I don't mind dropping 2.7 or even 3.0, but 3.1 is too much.

@@ -1,5 +1,5 @@
AllCops:
TargetRubyVersion: 2.7
TargetRubyVersion: 3.2
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't change the rubocop config at the same time, it makes for lots of unrelated changes making the review harder.

require 'active_job/railtie'
require 'rails/test_unit/railtie'
require 'sprockets/railtie'
require "rails/all"
Copy link
Contributor

Choose a reason for hiding this comment

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

This loads a lot of things that aren't needed. We should keep the old version.

@kartiki975
Copy link
Contributor Author

Closing this out in favour of #1324

@kartiki975 kartiki975 closed this Jan 26, 2024
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.

2 participants