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

Rails 7.1 config and deprecation -- step 1 #2439

Merged
merged 13 commits into from
Nov 27, 2023

Conversation

jrochkind
Copy link
Contributor

While #2431 upgraded to Rails 7.1, and was green in CI -- it did not update any configuration defaults to match Rails 7.1 yet, and still produced some Rails 7.1 deprecation warnings.

This PR is separated for ease of review, but it should be merged and deployed simultaneous with #2431.

It eliminates any deprecation warnings when run under Rails 7.1, and also updates much configuration to match Rails 7.1 defaults.

How we found what to update

First we checked railsdiff.com, to reivew how a newly generated app under Rails 7.1 might differ from Rails 7.0. We manually reviewed, and updated some local files to match. See for instance https://railsdiff.org/7.0.8/7.1.2

THEN we used the new_framework_defaults_7_1.rb file generated by Rails to opt into new defaults one by one, and see which ones if any broke our tests. https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#configure-framework-defaults. You can see this file in this PR.

We did NOT opt into any defaults that would keep our app from going back to 7.0 -- mainly ones that change serialization formats to ones that are not backwards compat and could not be readalbe by a 7.0 app.

At some LATER point, when we've been running 7.1 several weeks and know we won't need to go back -- we'll have another PR to fully take all 7.1 defaults, with load_defaults in application.rb instead of the new_framework_defaults... file.

Actual problem with run_after_transaction_callbacks_in_order_defined

So... ONE piece of config, run_after_transaction_callbacks_in_order_defined, actually did cause our tests to fail -- and it was catching real problem.

To make things more confusing -- at FIRST we didn't catch this, becuase it turned out enabling this in new_framework_defaults_7_1.rb didn't actually enable it in time to effect our Asset class. I noticed the problem when trying out the future branc that would have load_defaults 7.1 -- THAT one triggered the problem.

I realized that for reasons I won't go into here, we load our Asset classes during initialization, so we had to opt into this config before the initializer -- in application.rb. I did so to demonstrate the problem, then set about solving it.

It turned out to be pretty confusing -- we had some after_commit hooks in Asset, that need to run before shrine promotion, so they can look at file_data_previously_changed and see the delta that caused the shrine promotion, rather than the delta of the promotion.

Rails 7.1 defaults changed the order of after_commit hooks so they were broken... and Rails 7.1 didn't actually give us any tools to alter this! I was pretty stymied, but figured out using other Rails public API (I think!), I could easily implement a method that did let us insert after_commit hooks where we needed. That is implemented in kithe at sciencehistory/kithe#178

Then this PR right here upgrades to a version of kithe with the new function, and uses the new function to register the after_commit hooks so they will run before shrine promotion, and have access to the right previously changed deltas.

Phew!

…ure why yet

It was an oversight we were still on 6.1 and not 7.0 when we moved to Rails 7.0
That's all we were using secrets.yml from, we can just set the config from ENV directly, since we aren't putting key in source anyway. In fact, Rails would just pick it up from SECRET_KEY_BASE env even without this, by default? But for consistency with what we did before and clarity, we're going to move it over like this.
…ally take effect

In config/initializers, it was happening too late to actually affect our Asset class, so we didn't actually see the new behavior that ended up cauisng us problems
…er_commits to run early enough that we can still work in presence of Rails 7.1 run_after_transaction_callbacks_in_order_defined
@jrochkind
Copy link
Contributor Author

Wow, weird this is failing in CI -- it passed for me locally!

Based on the error, may have something to do with the app setup step, setting up the database etc, that we don't normally do in development... will have to spend more time with this.

Previously on CI, somehow our DB schema _was_ getting loaded... but after moving to Rails 7.1 it wasn't and was causing an error. Not sure what was going on before, but in CI setup script changing 'db:create' to 'db:prepare` tells rails to create the DB _and_ load the schema, and fixes our build
@jrochkind
Copy link
Contributor Author

in CI change db:create to db:prepare

Previously on CI, somehow our DB schema was getting loaded... but after moving to Rails 7.1 it wasn't and was causing an error. Not sure what was going on before, but in CI setup script changing 'db:create' to 'db:prepare` tells rails to create the DB and load the schema, and fixes our build

@jrochkind jrochkind marked this pull request as ready for review November 27, 2023 15:27
@jrochkind jrochkind merged commit d172acc into rails_7.1 Nov 27, 2023
1 check passed
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.

2 participants