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

feat: Add Rails 7.1 - 8 support #141

Merged
merged 17 commits into from
Jan 24, 2025

Conversation

argvniyx-enroute
Copy link
Contributor

@argvniyx-enroute argvniyx-enroute commented Jan 22, 2025

Summary

/task https://app.asana.com/0/1205835826533561/1209206146090339/f

Provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.

This:

  • Drops Rails <=6.1 support and Ruby <3.2 support
  • Adds appraisals for Rails 7.1 through 8
  • Swaps rubocop-* deps for betterlint (and in bundle updateing, bumps rubocop)
    • Also, rubocop -a's a bunch of HashSyntax lints
  • Does some other misc stuff to address deprecation warnings

I realize that there's a lot going on here; mostly dealt with it as I went along, so I'm happy to break this into multiple PRs.

Other Information

If there's anything else that's important and relevant to your pull
request, mention that information here. This could include
benchmarks, or other information.

The rubocop changes come from bumping the TargetRubyVersion, but I realized we do not do NewCops: enable. Lints stemming from that were less straightforward than the HashSyntax lints, so I decided to hold off for now.

Thanks for contributing to TestTrack!

/domain @Betterment/test_track_core
/platform @Betterment/test_track_core

- remove it from the ci matrix
- bump required_ruby_version in the gemspec
- remove call to `ruby` in Gemfile, regen appraisals
- bump TargetRubyVersion in rubocop.yml
  - and rubocop -a, since all of these are HashSyntax changes
- We also rm secrets.yml from the dummy app to deal with a deprecation
warning
- And updated the dummy application.rb to `config.load_defaults` for
Rails 7 and 7.1 (whilst dropping previous versions)
and update the dummy app config, as well as the ci matrix
@argvniyx-enroute argvniyx-enroute marked this pull request as ready for review January 22, 2025 18:42
@@ -11,10 +11,12 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
ruby: [3.0, 3.1, 3.2]
ruby: ["3.2"]
Copy link
Member

Choose a reason for hiding this comment

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

3.1 is technically still supported for a couple months -- since this is an open source client I might be inclined to increment required_ruby_version more slowly and keep support for 3.1 unless it's too costly for us.

Copy link
Member

Choose a reason for hiding this comment

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

We can always exclude combinations in the matrix if 3.1 isn't compatible with newest Rails.

@argvniyx-enroute argvniyx-enroute force-pushed the argv/rails-7-1-plus-support branch from 405628b to d338dd7 Compare January 22, 2025 20:25
`railties`'s constraint on zeitwerk is '~> 2.6' (or 2.5 for older
Rails), which means that bundle updating can put us on 2.7, which
requires Ruby 3.2. This will break CI tests for Ruby 3.1.

When we drop support for 3.1, we can unpin this dep (i.e. not even list
it at all, as before).
s.add_dependency 'request_store', '~> 1.3'
s.add_dependency 'sprockets-rails'
s.add_dependency 'zeitwerk', '< 2.7'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We get (AFAICT) zeitwerk because of railties, for which the constraint is ~> 2.6 (https://rubygems.org/gems/railties/versions/8.0.1; or ~> 2.5 for older Rails). The problem is that that means that bundle update can bump us to 2.7, which requires Ruby 3.2, effectively breaking 3.1 tests.

Copy link
Contributor

@rzane rzane Jan 23, 2025

Choose a reason for hiding this comment

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

I don't think we need to constrain the Zeitwerk version at runtime. We should move this to development dependencies.

Comment on lines 22 to 25
config.load_defaults 7.0 if rails_version_between?('7.0', '7.1')
config.load_defaults 7.1 if rails_version_between?('7.1', '7.2')
config.load_defaults 7.2 if rails_version_between?('7.2', '8.0')
config.load_defaults 8.0 if rails_version_between?('8.0', '8.1')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
config.load_defaults 7.0 if rails_version_between?('7.0', '7.1')
config.load_defaults 7.1 if rails_version_between?('7.1', '7.2')
config.load_defaults 7.2 if rails_version_between?('7.2', '8.0')
config.load_defaults 8.0 if rails_version_between?('8.0', '8.1')
config.load_defaults Rails::VERSION::STRING.to_f

We can also delete rails_version_between?

Copy link
Contributor

@rzane rzane left a comment

Choose a reason for hiding this comment

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

domain lgtm
platform lgtm

@argvniyx-enroute
Copy link
Contributor Author

I don't have merge permissions @rzane @smudge if y'all don't mind doing the honors

@rzane rzane merged commit 109bcb0 into Betterment:master Jan 24, 2025
8 checks passed
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