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 v7.1.3 #3346

Merged
merged 6 commits into from
Mar 12, 2024
Merged

Upgrade rails to v7.1.3 #3346

merged 6 commits into from
Mar 12, 2024

Conversation

murny
Copy link
Contributor

@murny murny commented Jan 18, 2024

Upgrades Jupiter Rails version from 7.0.8 to 7.1.3

https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#upgrading-from-rails-7-0-to-rails-7-1

The upgrade itself is trivial, no real changes required.

However, Rails 7.1 allows the lib directory to be autoloaded by default. This causes some issues with our two files jupiter.rb and jupiter/version.rb as they don't conform to Zeitwerk standards.

  • jupiter.rb tries to redefine creating a Jupiter constant that already exists. application.rb creates it before we start autoloading code, so Zeitwerk doesn't try to load it again which isn't ideal.
  • jupiter/version.rb Zeitwerk expects this to define a Jupiter::Version constant which it doesn't. So need to refactor this.

Note: Rails 7.1 gives one big deprecation warning whenever we use Rails.secrets. Created an issue in the backlog for this here: #3347

@@ -22,11 +22,13 @@
module Jupiter
class Application < Rails::Application

require 'jupiter'
Copy link
Contributor Author

@murny murny Feb 11, 2024

Choose a reason for hiding this comment

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

Had to change this a bit, using the config.autoload_lib(ignore: %w[assets tasks]) below will automatically autoload everything in lib directory for us.

But these jupiter.rb and jupiter/version.rb files don't exactly comply with Zeitwerk conventions.

  • jupiter.rb reopens the module Jupiter constant and essentially "monkey patches" it by adding new constants. This isn't really allowed in Zeitwerk, if a constant is already loaded, Zeitwerk will not reload it again.
    • I just moved the PRODUCTION_URL/TEST_URL constants down into secrets for now, not ideal, but it works. (Ideally, since we're not doing digitization anymore and no longer need subdomains and all that stuff, maybe we could just remove all this code.)
  • jupiter/version.rbdoesn't actually define a "Version" constant, so Zeitwerk isn't smart enough to figure out how to load this.
    • For version.rb I just refactored it a bit, to just be a simple class with a class level method for version info.

@murny
Copy link
Contributor Author

murny commented Mar 5, 2024

Let's attempt to do #3389 in this PR

@murny
Copy link
Contributor Author

murny commented Mar 6, 2024

Did a bad rebase here, need to fix this (lost some of the lib changes and changelog) this is fixed now

Copy link
Contributor

@ConnorSheremeta ConnorSheremeta left a comment

Choose a reason for hiding this comment

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

Looks good, just a question regarding one of the changes

@@ -1,7 +1,5 @@
require 'sidekiq/web'
require 'sidekiq/cron/web'
require 'admin_constraint'
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious about the change here removing these two lines, I can't seem to find anything relevant to this in the upgrade guide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These live in /lib directory, and before we treated lib code as if they were external gems, they were not autoloaded in our application and require you to manually require 'admin_constraint' to use them.

In Rails 7.1, they offer a way to autload the entire lib directory as if it was no different then any code in your app folder:

    # Please, add to the `ignore` list any other `lib` subdirectories that do
    # not contain `.rb` files, or that should not be reloaded or eager loaded.
    # Common ones are `templates`, `generators`, or `middleware`, for example.
    config.autoload_lib(ignore: %w[assets tasks])

So because this code is now being autoloaded, we don't need these require 'admin_constraint' statements anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thank you for the clarification!

Copy link
Contributor

@ConnorSheremeta ConnorSheremeta left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@murny murny merged commit d77231b into master Mar 12, 2024
4 checks passed
@murny murny deleted the Rails-7.1-upgrade branch March 12, 2024 18:12
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