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 6.1.4.7 #50511
Rails 6.1.4.7 #50511
Conversation
…tions format in rails 6.1
…ructures it's modifying
Specifically, update the signatures of some method overrides to reflect the new signatures of the overridden methods. Also add a new override for `drop_table` to reflect some changes (possibly a bug?) which broke temporary tables.
# service: S3 | ||
# access_key_id: <%= Rails.application.credentials.dig(:aws, :access_key_id) %> | ||
# secret_access_key: <%= Rails.application.credentials.dig(:aws, :secret_access_key) %> | ||
# region: us-east-1 |
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.
Can we grab the region dynamically? If the CDO
object has been initialized at this point, then we could set it from CDO.aws_region
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 possibly could if we want to, but this is a commented-out section of an auto-generated configuration file which we don't actually use anywhere. It's just a default placeholder which Rails automatically provides for new projects as of v6.0 and which it expects to always be in place as of v6.1, it's not something we need to personalize until we decide to actually start using it.
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.
oh, my. I need to improve my reading comprehension. 😂
Yup. these placeholders are fine.
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.
LGTM! impressively small diff (if you ignore schema.rb). nice job getting lots of work done ahead in other PRs.
@@ -534,7 +534,7 @@ def embed_level | |||
# Reference links should be stored as an array. | |||
if params[:level][:reference_links].is_a? String | |||
params[:level][:reference_links] = params[:level][:reference_links].split("\r\n") | |||
params[:level][:reference_links].delete_if(&:blank?) | |||
params[:level][:reference_links].compact_blank! |
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.
I am just curious, is this being forced by a newly enabled lint rule?
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.
Yep: Rails/CompactBlank
In addition to the required dependency updates, also apply a few changes to the codebase that can't effectively be applied in advance:
dashboard/config/storage.yml
withrails app:update
. We aren't actually using anyActiveStorage
functionality anywhere, but Rails 6.1 still expects this configuration file to be presentschema.rb
:create_table
invocations to match new expected options format in Rails 6.1no_transform_paths
customization to reflect changes toActionDispatch::FileHandler
serve
methodfile_readable?
method in order to create a mock test route instead of the removedmatch?
methodActiveRecord::ConnectionAdapters::Mysql2Adapter
to reflect changes to its internal structureLinks
cancancan
Gem to 3.2 #50693factory_girl_rails
to 4.9.0 #50767FactoryGirl
toFactoryBot
#50768Cache-Control
Header #50829where.not
Calls with Multiple Predicates #50910rake
Gem to v12 #50908Testing story
Largely relying on existing tests to ensure that this change doesn't disrupt any existing functionality. Also read through the major changes in this version (linked above) to confirm that none of them are immediately relevant for our usage.
Follow-up work
After this has been deployed, we can remove this transitional framework configuration and start working on preparing our configuration for Rails 7.0