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 version 7.2.1 #3306

Merged
merged 3 commits into from
Sep 13, 2024
Merged

Upgrade Rails to version 7.2.1 #3306

merged 3 commits into from
Sep 13, 2024

Conversation

unoduetre
Copy link
Contributor

@unoduetre unoduetre commented Aug 22, 2024

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

What

Upgrade government-frontend to use the latest version of Rails (7.2.1).

Why

Trello card

@unoduetre unoduetre changed the title Upgrade Rails to version 7.2 Upgrade Rails to version 7.2.0 Aug 22, 2024
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3306 August 22, 2024 14:07 Inactive
@unoduetre unoduetre force-pushed the upgrade-rails-to-7.2 branch from 2930f88 to 1a0ddbe Compare August 22, 2024 14:10
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3306 August 22, 2024 14:11 Inactive
@@ -0,0 +1,7 @@
#!/usr/bin/env ruby
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added by Rails app:update

bin/bundle Outdated
@@ -1,3 +1,114 @@
#!/usr/bin/env ruby
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating this file was suggested by Rails.

@@ -0,0 +1,8 @@
#!/usr/bin/env ruby
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in order to support the config option.

@@ -1,8 +1,8 @@
#!/usr/bin/env ruby
require "fileutils"

# path to your application root.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes suggested by rails app:update

# 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])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change suggested by rails app:update

@@ -1,11 +1,13 @@
# Be sure to restart your server when you modify this file.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes suggested by rails app:update.

@@ -4,21 +4,21 @@
<title>The page you were looking for doesn't exist (404)</title>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes suggested by rails app:update.

@@ -0,0 +1,66 @@
<!DOCTYPE html>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

File added by rails app:update.

@@ -4,21 +4,21 @@
<title>The change you wanted was rejected (422)</title>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes suggested by rails app:update.

@@ -4,21 +4,21 @@
<title>We're sorry, but something went wrong (500)</title>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes suggested by rails app:update.

@unoduetre
Copy link
Contributor Author

unoduetre commented Aug 22, 2024

The default 7.2 config enables YJIT if Ruby version is at least 3.3. The current version of Ruby in this app is lower than that, but I tested this with Ruby 3.3.4.

@unoduetre unoduetre force-pushed the upgrade-rails-to-7.2 branch from 1a0ddbe to ba7f53c Compare August 22, 2024 15:22
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3306 August 22, 2024 15:23 Inactive

# Settings in config/environments/* take precedence over those specified here.
# Application configuration can go into files in config/initializers
# -- all .rb files in that directory are automatically loaded after loading
# the framework and any gems in your application.

# Custom directories with classes and modules you want to be autoloadable.
config.autoload_paths += %W[#{config.root}/lib]
config.autoload_paths += %W[#{config.root}/lib #{config.root}/lib/helpers]
Copy link
Contributor Author

@unoduetre unoduetre Aug 22, 2024

Choose a reason for hiding this comment

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

Allow /lib/helpers to be autoloaded after migrating to 7.2 config. This resolves a bug e.g. in the Heroku review app.

@unoduetre unoduetre marked this pull request as ready for review August 22, 2024 15:37
@unoduetre
Copy link
Contributor Author

I have removed the icon files.

@unoduetre
Copy link
Contributor Author

I'm updating the ticket to upgrade to 7.2.1

@unoduetre unoduetre marked this pull request as draft August 28, 2024 09:55
@unoduetre unoduetre force-pushed the upgrade-rails-to-7.2 branch from ba7f53c to 9144135 Compare August 28, 2024 11:15
@unoduetre unoduetre changed the title Upgrade Rails to version 7.2.0 Upgrade Rails to version 7.2.1 Aug 28, 2024
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3306 August 28, 2024 11:15 Inactive
@unoduetre
Copy link
Contributor Author

I'm updating the ticket to upgrade to 7.2.1

Done

@unoduetre unoduetre force-pushed the upgrade-rails-to-7.2 branch from 9144135 to ce776d6 Compare August 28, 2024 11:16
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3306 August 28, 2024 11:17 Inactive
@unoduetre unoduetre marked this pull request as ready for review August 28, 2024 11:25
@unoduetre unoduetre force-pushed the upgrade-rails-to-7.2 branch from ce776d6 to 4a500a0 Compare August 29, 2024 13:07
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3306 August 29, 2024 13:07 Inactive
@unoduetre unoduetre force-pushed the upgrade-rails-to-7.2 branch from 4a500a0 to f9975c8 Compare August 29, 2024 13:44
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3306 August 29, 2024 13:45 Inactive
@unoduetre unoduetre force-pushed the upgrade-rails-to-7.2 branch from f9975c8 to c12cb4d Compare August 29, 2024 13:47
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3306 August 29, 2024 13:47 Inactive
@unoduetre unoduetre force-pushed the upgrade-rails-to-7.2 branch from c12cb4d to a565590 Compare August 30, 2024 16:04
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3306 August 30, 2024 16:05 Inactive
config.action_controller.raise_on_missing_callback_actions = false

# Apply autocorrection by RuboCop to files generated by `bin/rails generate`.
config.generators.apply_rubocop_autocorrect_after_generate!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabled so our rubocop rules apply automatically.

@unoduetre unoduetre force-pushed the upgrade-rails-to-7.2 branch from a565590 to adc2015 Compare September 5, 2024 09:10
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3306 September 5, 2024 09:10 Inactive
@@ -52,10 +52,13 @@
# config.i18n.raise_on_missing_translations = true

# Annotate rendered view with file names.
# config.action_view.annotate_rendered_view_with_filenames = true
config.action_view.annotate_rendered_view_with_filenames = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but would be nice to call out why we're enabling this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a change suggested by app:update.

As it only applies to the development environment, it's safe to apply with no impact on tests or production.

It helps in debugging. Here is the description of what it does: https://www.bigbinary.com/blog/rails-6-1-adds-annotate_rendered_view_with_filenames-to-annotate-html-output

What is your opinion on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think 'It helps in debugging.' is enough. Just so people looking back can understand why we decided to uncomment it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think this conversation is probably enough documentation too! So I think all good to be merged! 💪🏻

Copy link
Contributor

@beccapearce beccapearce 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!
One non-blocking comment :)

@unoduetre unoduetre merged commit 8d30342 into main Sep 13, 2024
11 checks passed
@unoduetre unoduetre deleted the upgrade-rails-to-7.2 branch September 13, 2024 12:14
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