Conversation
anarchivist
left a comment
There was a problem hiding this comment.
generally looks good - rw+c. most of my comments are minor. you might want to ask Anna to look at this, too.
awilfox
left a comment
There was a problem hiding this comment.
This looks mostly good, just a few nits. However, it looks like rails app:update wasn't run, similar to what happened at the beginning of BerkeleyLibrary/UCBEARS#21. Can you make sure that's run and the configuration files are updated appropriately? I described how to do that in the UCBEARS MR.
lib/tasks/db.rake
Outdated
| ActiveRecord::Base.connection_pool.with_connection(&:active?) | ||
| rescue PG::ConnectionBad |
There was a problem hiding this comment.
When we upgraded Framework to 8.0, the suggested way to do it was to use verify! on the connection object. We also needed to add ActiveRecord::DatabaseConnectionError to the rescue block, but I'm not sure if that's because of verify! or because in general Rails 8 can raise that.
| ActiveRecord::Base.connection_pool.with_connection(&:active?) | |
| rescue PG::ConnectionBad | |
| ActiveRecord::Base.connection.verify! | |
| rescue PG::ConnectionBad, ActiveRecord::DatabaseConnectionError |
I don't mind doing it the way already here; I just haven't seen it before. (will this raise if there are no active connections, or just return nil?)
There was a problem hiding this comment.
Nice catch - I've updated to use verify! instead of active!
Gemfile.lock
Outdated
|
|
||
| RUBY VERSION | ||
| ruby 3.3.9p170 | ||
| ruby 3.3.9p170 |
There was a problem hiding this comment.
Should we be moving this to 3.3.11 as well?
There was a problem hiding this comment.
So do you think it's worth pinning the base image to 3.3.11?
Right now:
FROM ruby:3.3-slim AS base
I could pin it to 3.3.11, but then future bumps would need to be manually updated.
There was a problem hiding this comment.
This is a little complicated, but we shouldn't change the Dockerfile.
In practice, we should be updating the Ruby version in the Gemfile.lock (or ship with a .ruby-version file), but it doesn't really have too much of an effect here. IIRC, this specifies the version that bundle was run under.
If we rebuild the image (e.g., after a merged new pull request), it will fetch the latest tag of ruby:3.3-slim, which updates the Ruby patch version under the hood.
There was a problem hiding this comment.
Weird, yeah, my container was already running 3.3.11, but the lockfile wasn't updating. Finally did with this command:
docker compose run --rm app bundle update --ruby
Gemfile
Outdated
| gem 'rack-cors' | ||
| gem 'rails', '~> 7.0.4' | ||
| gem 'ransack', '~> 2.6' | ||
| gem 'rails', '~> 8.0.4' |
There was a problem hiding this comment.
They've already snuck out another release while this was being worked on!
| gem 'rails', '~> 8.0.4' | |
| gem 'rails', '~> 8.0.5' |
There was a problem hiding this comment.
Those sneaky guys... Updated to 8.0.5!
.github/workflows/build.yml
Outdated
| - name: Validate database migrations | ||
| env: | ||
| RAILS_ENV: production | ||
| SECRET_KEY_BASE: dummy |
There was a problem hiding this comment.
Do we want to test the Rails secrets stuff here like we do in Framework?
See BerkeleyLibrary/framework@6cf75564ef, specifically .github/workflows/build.yml and docker-compose.ci.yml. This makes a "real" (throwaway) secret, and that ensures the app is going to read it from the correct place in production.
There was a problem hiding this comment.
I tried to shoe-horn that kind of test stage into the build.yml, but looks like it would take a little more work, galc-ui setup is a bit different than framework... so reverted back to the secret_key_base->dummy.
awilfox
left a comment
There was a problem hiding this comment.
We're quite close! Luckily there aren't too many new configurations it found for 8.0. I'm not sure about 7.1 and 7.2; you may need to step each one of those as well.
You don't need to do each one, since some of them are obviously not going to affect us (old IE support and the like). The main ones I'm concerned about from 7.1 and 7.2 are the Postgres adaptor settings and YJIT.
| @@ -26,6 +26,10 @@ | |||
| module GalcApi | |||
| class Application < Rails::Application | |||
| config.load_defaults 7.0 | |||
There was a problem hiding this comment.
It would be preferable to use the initialiser to test each new configuration, and assuming each one works fine, to change this to 8.0.
| config.load_defaults 7.0 | |
| config.load_defaults 8.0 |
| # Uncomment each configuration one by one to switch to the new default. | ||
| # Once your application is ready to run with all new defaults, you can remove | ||
| # this file and set the `config.load_defaults` to `8.0`. |
|
To be clear, you can download the 7.1 and 7.2 files I linked above and put them in |
Upgrade Rails and dependencies to 8.0.x