-
-
Notifications
You must be signed in to change notification settings - Fork 932
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
Load framework defaults for Rails 7.2 #5305
Conversation
Removes the yjit initializer. YJIT is now enabled by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just notes for future reference on whether or not these are likely to cause any problems.
# backends that use the same database as Active Record as a queue, hence they | ||
# don't need this feature. | ||
#++ | ||
# Rails.application.config.active_job.enqueue_after_transaction_commit = :default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GoodJob defaults this to false
and since this config wasn't set until now, it was already using false within goodjob. It will continue to do so with this default in place.
# This is possible due to broad browser support for WebP, but older browsers and email clients may still not support | ||
# WebP. Requires imagemagick/libvips built with WebP support. | ||
#++ | ||
# Rails.application.config.active_storage.web_image_content_types = %w[image/png image/jpeg image/gif image/webp] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use active storage (a tests exists assert that its route is not loaded)
# Applications with existing timestamped migrations that do not adhere to the | ||
# expected format can disable validation by setting this config to `false`. | ||
#++ | ||
# Rails.application.config.active_record.validate_migration_timestamps = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a problem.
# | ||
# This query used to return a `String`. | ||
#++ | ||
# Rails.application.config.active_record.postgresql_adapter_decode_dates = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have no date fields.
# Enables YJIT as of Ruby 3.3, to bring sizeable performance improvements. If you are | ||
# deploying to a memory constrained environment you may want to set this to `false`. | ||
#++ | ||
# Rails.application.config.yjit = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already enable yjit ourselves.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5305 +/- ##
=======================================
Coverage 97.14% 97.14%
=======================================
Files 457 457
Lines 9564 9564
=======================================
Hits 9291 9291
Misses 273 273 ☔ View full report in Codecov by Sentry. |
The tests that fail here do not fail on the rails 8 branch. This is caused by a framework default change, almost certainly the job transaction/commit behavior. Something in rails 8 may have fixed the behavior in tests so that this no longer occurs. |
Given that these tests fail and rails 8 tests, with rails 8 defaults pass, I'm guessing something about how we run these failing tests was fixed in rails 8. I do not think it's worth it to try to fix this test now when I know my fix is unnecessary to get us to rails 8. |
Pull request was closed
Removes the yjit initializer. YJIT is now enabled by default.