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 & update gems (mostly only minor versions) #609

Merged
merged 33 commits into from
Apr 26, 2024

Conversation

Splines
Copy link
Member

@Splines Splines commented Apr 6, 2024

This fixes #554. See the upgrade guide and all config keys.

Commands used to perform the update:

# Edit Gemfile manually and update Rails version there. Then run
bundle update

# And then (development env variable was only set later in the process)
RAILS_ENV="development"  bin/rails app:update --trace

# I've also run `bin/rails app:update --trace` inside a local dev mampf container to finish the process
# (active record needed to connect to the database)

Notable changes

Only changes I found noteworthy in particular.

  • Memcache is using connection pooling by default. Let's see if that will cause any troubles in production.
  • raise_on_missing_translation is great, I guess. So i've activated it.
  • Set default log level in production to info instead of warn. This will log a bit more information that may be helpful in production. info is also the default by Rails.
  • Set config.require_master_key = true. With this config option activated, If the Rails master key is not available in production, the app won't boot, which is good.
  • ❓ I'm unsure whether this hash change affects us: allow_deprecated_parameters_hash_equality
  • Transactions callbacks are handled in a better way to avoid stale data. See here.
  • ❓❗ Active Record Encryption now uses SHA-256 as its hash digest algorithm.
    --> Not affected as we don't use Active Record Encryption.
  • Some migrations were introduced by the Rails updater. I don't really know what they do, will have to review them.
    --> Deleted them as we don't use ActiveStorage, but shrine instead.
  • globalize_attribute_names, see this comment

For the future

  • Content-Security-Policy might be useful to set up but we have to watch out if this is working in conjunction with nginx.

TODO

  • Go through notable changes list and update new_framework_defaults_7_1.rb accordingly
  • Check if everything is working locally
  • Check if everything is working in mampf-experimental
  • The Rails updater introduced some migrations and I'm not sure yet what their purpose is. Will have to review them. (see above)

Splines and others added 6 commits February 16, 2024 12:07
Merge pull request #595 from MaMpf-HD/dev
Merge pull request #601 from MaMpf-HD/dev
Merge pull request #604 from MaMpf-HD/dev
Merge pull request #605 from MaMpf-HD/dev
Annotation Tool 🙌
Merge pull request #608 from MaMpf-HD/dev
@Splines Splines added the dependencies Pull requests that update a dependency file label Apr 6, 2024
@Splines Splines self-assigned this Apr 6, 2024
@Splines Splines changed the base branch from main to dev April 6, 2024 22:50
This was done because `bin/rails app:update` failed with:
** Execute app:update:active_storage
       rails  active_storage:update
bin/rails aborted!
Gem::LoadError: can't activate listen (~> 3.5), already activated listen-3.0.8.
Make sure all dependencies are added to Gemfile.
@Splines Splines changed the base branch from dev to main April 6, 2024 23:25
@Splines Splines changed the base branch from main to dev April 6, 2024 23:25
@Splines Splines changed the title Upgrade Rails to v7.1 Upgrade Rails to v7.1 & update gems (mostly only minor versions) Apr 6, 2024
@fosterfarrell9
Copy link
Collaborator

fosterfarrell9 commented Apr 7, 2024

Some migrations were introduced by the Rails updater. I don't really know what they do, will have to review them.

I think we can delete them since we do not use ActiveStorage (but the shrine gem instead for uploads). They would not do anything anyway since they all contain the line

return unless table_exists?(:active_storage_blobs)

and this table does not exist in our setting.

Active Record Encryption now uses SHA-256 as its hash digest algorithm.

At least as of now, this does not affect us since we do not use ActiveRecord Encryption (which was introduced in Rails 7.0).

I'm unsure whether this hash change affects us: allow_deprecated_parameters_hash_equality

Me as well. This seems to refer to this:

Comparing equality between `ActionController::Parameters` and a
`Hash` is deprecated and will be removed in Rails 7.2. Please only do
comparisons between instances of `ActionController::Parameters`. If
you need to compare to a hash, first convert it using
`ActionController::Parameters#new`.
To disable the deprecated behaviour set
`Rails.application.config.action_controller.allow_deprecated_parameters_hash_equality = false`

I would expect we do not use the deprecated comparisons but to be sure one would have to go through all the controllers.

@Splines
Copy link
Member Author

Splines commented Apr 7, 2024

I think we can delete them since we do not use ActiveStorage (but the shrine gem instead for uploads).

Thanks, I just deleted these migrations.

At least as of now, this does not affect us since we do not use ActiveRecord Encryption (which was introduced in Rails 7.0).

Ah, great, one less thing to worry about.

@Splines
Copy link
Member Author

Splines commented Apr 7, 2024

This seems to refer to this:

@fosterfarrell9, Where do you have that info from, i.e. the code block with the explanation for the parameter?

@fosterfarrell9
Copy link
Collaborator

fosterfarrell9 commented Apr 7, 2024

In the Rails source code: https://github.com/rails/rails/blob/v7.1.3.2/actionpack/lib/action_controller/metal/strong_parameters.rb

So I think we just have to look if for == with params involved in our controllers.

@Splines
Copy link
Member Author

Splines commented Apr 8, 2024

So I think we just have to look if for == with params involved in our controllers.

I've checked that for our controllers by searching for "==". I could only find comparison to string or integers but not to any hashes. Maybe you can take a look as well?

In the comments_controller we have @comment.thread.config.comment_voting.to_sym == :ld, but that's not an issue, I guess, since the first argument is not an action controller parameter.

In the future, before upgrading to Rails 7.2, we should check our production log and see if the warning you mentioned pops up there. If it does, we know where to fix this in the code.

config/initializers/new_framework_defaults_7_1.rb Outdated Show resolved Hide resolved
config/initializers/new_framework_defaults_7_1.rb Outdated Show resolved Hide resolved
config/initializers/new_framework_defaults_7_1.rb Outdated Show resolved Hide resolved
config/initializers/new_framework_defaults_7_1.rb Outdated Show resolved Hide resolved
@Splines Splines mentioned this pull request Apr 8, 2024
3 tasks
You can do so locally via `bundle update --bundler`
Performed automatically via `bundle install`.
config/environments/test.rb Show resolved Hide resolved
config/application.rb Show resolved Hide resolved
@Splines Splines marked this pull request as ready for review April 17, 2024 21:03
@Splines Splines marked this pull request as draft April 17, 2024 21:31
@Splines
Copy link
Member Author

Splines commented Apr 24, 2024

In f486935, I made the changes regarding globalize_attribute_names according to this mobility issue.

For reviewers

  • I've verified that it works for Division, could you @fosterfarrell9 verify it for Program and Subject? (since I don't know where to find the latter two in the UI and I'd have to look through the code where we use it, maybe you can show it to me today in persona).

@Splines Splines marked this pull request as ready for review April 24, 2024 08:30
@Splines
Copy link
Member Author

Splines commented Apr 25, 2024

@fosterfarrell9 Could you resolve the one missing conversation?

@Splines Splines merged commit e41797f into dev Apr 26, 2024
7 checks passed
@Splines Splines deleted the deps/upgrade-rails branch April 26, 2024 13:15
@Splines Splines restored the deps/upgrade-rails branch April 26, 2024 13:15
@Splines Splines deleted the deps/upgrade-rails branch April 26, 2024 13:15
Splines added a commit that referenced this pull request Jun 26, 2024
Along side this, the schema was automatically updated and got rid of
old translation tables originally created by the globalize gem.
They were still in here as remnant of #609.
Splines added a commit that referenced this pull request Jul 2, 2024
#652)

* Setup X11 Cypress GUI forwarding

Also see my StackOverflow question:
https://stackoverflow.com/q/78639075

* Delete unnecessary Cypress sample files

* Simplify Cypress config setup

* Use internal docker host port

* Delete old cypress helper

* Remove e2e configuration

* Fix wrong network assignment in Docker

* Add cypress ESLint plugin back

* Copy up-to-date on-rails commands from GitHub

See this file:
https://github.com/shakacode/cypress-on-rails/blob/master/lib/generators/cypress_on_rails/templates/spec/cypress/support/on-rails.js

* Init dummy submission cypress test

* Clean up comments in cypress-interactive Dockerfile

* Outsource cypress docker setup in separate file

* Remove `run_tests.sh` file

* Add `createUserAndLogin` cypress command

Also updated the cypress user emails and passwords.

* Name cypress users more creatively

* Add own Cypress FactoryBot implementation

* Move cypress controller to subfolder

* Remove cypress-on-rails dependency

Note that at this point, some cypress commands don't work yet.

* Outsource js error parsing logic to its own file

* Add database cleaner and user creator for cypress

* Fix RuboCop errors and improve documentation

* Remove unnecessary `Cypress::` specifier

* Remove unnecessary `index.js` file

* Improve documentation for FactoryBot

* Start Cypress UI automatically with open browser

* Add Cypress tests to GitHub Actions CI/CD

* Pass ./cypress.yml file to docker & split lines

* Disable existing cypress tests for now

will bring them back in a subsequent PR

* Rename submissions spec

* Rename cypress route subjects

* Don't clear entrypoint for cypress tests

* Execute "cypress run" in entrypoint

* Rename archived tests and exclude them from ESLint

* Temporarily disable docker compose cache

* Disable old migrations stemming from globalize gem

Along side this, the schema was automatically updated and got rid of
old translation tables originally created by the globalize gem.
They were still in here as remnant of #609.

* Add back docker compose cache to cypress workflow

* Revert "Disable old migrations stemming from globalize gem"

This reverts commit 88f369e.

* Use `db:schema:load` instead of `db:migrate`

See https://github.com/rails/rails/blob/d43ee2088118425e493766aeb20575e9ce7159d1/actionmailbox/test/dummy/db/schema.rb#L5-L9

We also abort the script if it is called from the production
environment. It's only intended for usage in the dev/test environment.
For production, we have the respective master and worker entrypoints.

* Wait for MaMpf before opening cypress UI
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.

Migrate to Rails 7.1
2 participants