Skip to content

Upgrade Ruby (3.1.4 -> 3.3.8) & Rails (7.1.3 -> 7.2.2.1) #749

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

Merged
merged 40 commits into from
May 9, 2025

Conversation

fosterfarrell9
Copy link
Collaborator

@fosterfarrell9 fosterfarrell9 commented Feb 22, 2025

Note: This PR has a "best before" date: 2025-03-31, see here. The respective Ruby on Rails upgrade guide is here.

This PR upgrades Ruby to 3.3.7 and Rails to 7.2.2.1 (and in particular resolves #611 and goes most of the way for #709), and some gems that needed an upgrade in order to make the Rails upgrade work. So far, it seems to work mostly nicely. Several deprecation warnings were shown; I fixed them all except one (see below),
and some unit tests are failing (see below).

Important

For your local setup to still work after this PR, perform the steps outlined here in the wiki.
Furthermore, since we are changing the Docker base image (FROM), you have to rebuild all MaMpf Docker Containers from scratch.

Notes for the reviewer:

  • I let the app:update script overwrite our puma.rb file which seemed quite out of date compared to the current standard one. One notable difference is the handling of the pid file. Before, it was
ENV.fetch("PIDFILE") { "tmp/pids/server.pid" }

Now, it is

# Specify the PID file. Defaults to tmp/pids/server.pid in development.
# In other environments, only set the PID file if requested.
pidfile ENV["PIDFILE"] if ENV["PIDFILE"]

On partiulcar, if we need the PID file in production, we should set an environment variable. I am not sure: We have

echo "Ensuring no stale server pid file is present"
rm -f tmp/pids/server.pid

in production/entrypoint_worker.sh.

  • We still have files related to a "development" environment (and if you search in our codebase, you will find several occurences of "development" referring to a "development" environment. Do we need that? All our development takes place in docker_development .
  • Rails 7.2 standard production.rb ccontains the line config.force_ssl = true. I added it, but put it in a comment. We should find out what happens if we uncomment it.
  • It appears that the standard in Rails 7.2 is that in a production console you can onbly see the id of database entries, nothing more. I overrode this in production.rb via
config.active_record.attributes_for_inspect = :all
  • In docker_development.rb I added the following lines wich come form the current Rails default devlopment file:
  # Raise error when a before_action's only/except options reference missing actions.
  config.action_controller.raise_on_missing_callback_actions = true

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

I think the first one makes sense, and the second one should remain a comment since we have our own rubocop thing going on.

  • We still get this warning
DEPRECATION WARNING [legacy-js-api]: The legacy JS API is deprecated and will be removed in Dart Sass 2.0.0.
More info: https://sass-lang.com/d/legacy-js-api

See here. It appears we could get rid of it if we upgrade sass-loader to v16 in our yarn modules, but do we want that? It seems that vite offers a better solution, see here.

  • I am a little confused about the failing unit tests. The error messages say
Failure/Error:
  expect do
    UserCleaner.new.send_additional_warning_mails
  end.not_to have_enqueued_mail(UserCleanerMailer, :pending_deletion_email)

StandardError:
  To use HaveEnqueuedMail matcher set `ActiveJob::Base.queue_adapter = :test`

so this seems to refer to this. But in config/environments/test.rb we already set

config.active_job.queue_adapter = :test

Update: I guess the problem is in config/initializers/active_job.rb where we set (for all environments)

Rails.application.config.active_job.queue_adapter = :sidekiq

which probably overrides the :test setting. Do we have tests (unit or cypress) that rely on sidekiq? If not, we could just do a conditional here.

  • I checked the things mentioned in Upgrade to Rails 7.2 #611 and did not see any more deprecation warnings. Concerning the active_support.cache_format_version: I think we should be able to safely change this in production now (which happens automatically in this PR since we use the Rails 7.2 framework defaults in application.rb).

@fosterfarrell9 fosterfarrell9 changed the title Upgrade/ruby and rails Upgrade ruby and rails Feb 22, 2025
Splines added 3 commits May 6, 2025 19:41
Note that I also removed `config.assets.debug = true` since I don't see
why we would need this. Maybe we will stumble upon a scenario in the
future, but then at least we know why we need it. Note also that this
was 8 years old.
In the end, I ended up with `docker_development.rb` being exactly the
same as `development.rb` 😅 Except for the last line:
`config.web_console.development_only = false`.
@Splines Splines temporarily deployed to testing-review May 6, 2025 17:45 — with GitHub Actions Inactive
Splines
Splines previously approved these changes May 6, 2025
Copy link
Member

@Splines Splines left a comment

Choose a reason for hiding this comment

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

I went again through everything and pushed some more commits for the rest of the changes. The body of my commit messages explain key points.


Note that I've activated

config.action_view.annotate_rendered_view_with_filenames = true

which I think is pretty handy for development, e.g. see the browser console now:

image

One you've reviewed my changes as well, feel free to squash-merge into dev after tests run through successfully.

Copy link

codecov bot commented May 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (dev@294eaf8). Learn more about missing BASE report.

Additional details and impacted files
@@          Coverage Diff           @@
##             dev     #749   +/-   ##
======================================
  Coverage       ?   54.21%           
======================================
  Files          ?      178           
  Lines          ?     7490           
  Branches       ?        0           
======================================
  Hits           ?     4061           
  Misses         ?     3429           
  Partials       ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Splines Splines temporarily deployed to testing-review May 9, 2025 17:07 — with GitHub Actions Inactive
Splines
Splines previously approved these changes May 9, 2025
@Splines
Copy link
Member

Splines commented May 9, 2025

squash and merge ;)

I removed that as I thought this was done by default, but turns out this is
not true for our specific option with $stdout.

Config level is by default set to debug for all environments except production.
This is done by Rails itself (7.2 defaults).
@fosterfarrell9 fosterfarrell9 merged commit 5e244b6 into dev May 9, 2025
5 checks passed
@fosterfarrell9 fosterfarrell9 deleted the upgrade/ruby_and_rails branch May 9, 2025 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants