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

Update to rails 7.1 #2240

Merged
merged 13 commits into from
Feb 13, 2024
Merged

Update to rails 7.1 #2240

merged 13 commits into from
Feb 13, 2024

Conversation

ivan-kocienski-gfsc
Copy link
Contributor

@ivan-kocienski-gfsc ivan-kocienski-gfsc commented Feb 8, 2024

Closes #1110

When this is merged i'd advise you delete your gemset and reinstall all the gems fresh

Gems updated

  • rails
  • devise
  • papertrail

Notes

Deprecation warning

DEPRECATION WARNING: Support for `config.active_support.cache_format_version = 6.1` has been deprecated and will be removed in Rails 7.2.

is coming from /config/application.rb:18 - the rails cache needs to be upgraded before this line can be removed so we will need to deploy the code to production first before we can remove this line. (Or we could just YOLO it and nullify the staging/production caches which may impact site performance). Come to think of it- do we even use the cache if the site container is completely rebuilt on every deploy?

Devise redirects

There is a monkey-patch hack in /app/controllers/users/sessions_controller.rb that patches how Devise does redirects. In rails 7+ it looks like by default redirect_to does not allow inter-domain redirects. Unfortunately PC (and devise) bounces users from placecal.org to admin.placecal.org. I looked to see if there is a configuration to get the original behaviour back but came up short. So I had to "intercept" the redirect devise makes and patch it with the correct parameter. Please for all that is good can we revisit this at some point in the future and take it out when it has been fixed in devise.

Datatables additional links

In /app/views/layouts/admin/_datatables.html.erb there is some code dealing with additional links that stopped working so I commented this out. Do we use this feature at all? It doesn't look like we do.

Credentials (.yml.enc)

It looks like in rails 7+ they have moved the credentials (or secrets) in to an encrypted file (so they can be published publicly but securely?). See here, here and here for background.

The TLDR of it is that secrets.yml is gone and credentials.yml.enc is the new way. This file needs a master.key file to access. This file is generated if it doesn't exist and you cannot add it to the repo (it is ignored by git). You can edit the credentials file by running rails credentials:edit. So we have to share the master key file privately if we want to use this mechanism.

SO... this is a pain and if you just rm config/master.key then rails won't try to load it during runs / tests. The only thing set in the secrets.yml is SECRET_KEY_BASE which is only used by Devise to encrypt all it's various tokens (invitation, session, password reset). I have patched the devise initialiser to load this from from the environment if present so hopefully prod and staging should keep working.

@aaaaargZombies
Copy link
Contributor

I notice this is still in draft but gave it a run and noticed

DEPRECATION WARNING: Support for `config.active_support.cache_format_version = 6.1` has been deprecated and will be removed in Rails 7.2.

Check the Rails upgrade guide at https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#new-activesupport-cache-serialization-format
for more information on how to upgrade.
 (called from <main> at /home/unicorn/projects/PlaceCal/config/environment.rb:7)
DEPRECATION WARNING: `Rails.application.secrets` is deprecated in favor of `Rails.application.credentials` and will be removed in Rails 7.2. (called from <main> at /home/unicorn/projects/PlaceCal/config/environment.rb:7)
DEPRECATION WARNING: Support for `config.active_support.cache_format_version = 6.1` has been deprecated and will be removed in Rails 7.2.

Check the Rails upgrade guide at https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#new-activesupport-cache-serialization-format
for more information on how to upgrade.
 (called from <main> at /home/unicorn/projects/PlaceCal/config/environment.rb:7)
DEPRECATION WARNING: `Rails.application.secrets` is deprecated in favor of `Rails.application.credentials` and will be removed in Rails 7.2. (called from <main> at /home/unicorn/projects/PlaceCal/config/environment.rb:7)

looking good though, feels a bit faster too.

@r-ferrier
Copy link
Contributor

This looks great. On start up I'm only seeing this deprecation:

DEPRECATION WARNING: Support for `config.active_support.cache_format_version = 6.1` has been deprecated and will be removed in Rails 7.2.

In testing I'm also seeing

DEPRECATION WARNING: `Rails.application.secrets` is deprecated in favor of `Rails.application.credentials` and will be removed in Rails 7.2. (called from <main> at /Users/rosemaryferrier/GFSC/PlaceCal/config/environment.rb:7)

All the tests pass and a quick runthrough using the app was fine. So if you could investigate these deprecations, once they're done, this can probably go in!

@ivan-kocienski-gfsc
Copy link
Contributor Author

Thanks for giving it a test!

Yes that deprecation warning is part of the upgrade procedure.

@ivan-kocienski-gfsc ivan-kocienski-gfsc changed the title Update to rails 7.1 (WIP) Update to rails 7.1 Feb 13, 2024
@ivan-kocienski-gfsc ivan-kocienski-gfsc marked this pull request as ready for review February 13, 2024 09:11
@ivan-kocienski-gfsc ivan-kocienski-gfsc changed the title (WIP) Update to rails 7.1 Update to rails 7.1 Feb 13, 2024
Copy link
Contributor

@r-ferrier r-ferrier left a comment

Choose a reason for hiding this comment

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

This looks great and reading through the description everything there sounds sensible. I also agree that using env variables over the new credentials set up sounds more sensible for our configuration, we can always revisit this if we think it would be useful in the future.
I've added another push to this to change the gemfile.lock slightly as my machine demands a slightly different version of nokogiri. Tests are still passing so hopefully that's fine.
let's get this on staging!

@ivan-kocienski-gfsc ivan-kocienski-gfsc merged commit 9b73b99 into main Feb 13, 2024
2 checks passed
@ivan-kocienski-gfsc ivan-kocienski-gfsc deleted the ik-1110-update-rails branch February 13, 2024 13:18
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.

Upgrade to Rails 7
3 participants