-
Notifications
You must be signed in to change notification settings - Fork 4
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
Bump rails from 7.2.2 to 8.0.0 #999
base: main
Are you sure you want to change the base?
Conversation
|
A newer version of rails exists, but since this PR has been edited by someone other than Dependabot I haven't updated it. You'll get a PR for the updated version as normal once this PR is merged. |
377f03c
to
5e000ce
Compare
Bumps [rails](https://github.com/rails/rails) from 7.2.2 to 8.0.0. - [Release notes](https://github.com/rails/rails/releases) - [Commits](rails/rails@v7.2.2...v8.0.0) --- updated-dependencies: - dependency-name: rails dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
This was introduced in the rails 8.0 upgrade because of deprecation warnings. Without the new default file we no longer get the warnings so i've removed the line from the application config.
5e000ce
to
9229aa2
Compare
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've left some questions and comments. I haven't got very far into the review, but thought it might be more productive to leave the comments now and start another review when you've responded because I anticipated more of the same kind of "why is this being changed?" etc etc
@@ -439,7 +439,7 @@ GEM | |||
nokogiri (>= 1.6) | |||
rails-html-sanitizer (1.6.2) | |||
loofah (~> 2.21) | |||
nokogiri (~> 1.14) | |||
nokogiri (>= 1.15.7, != 1.16.7, != 1.16.6, != 1.16.5, != 1.16.4, != 1.16.3, != 1.16.2, != 1.16.1, != 1.16.0.rc1, != 1.16.0) |
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.
Did this change really result from running rails app:update
?
if Rails.root.join("tmp/caching-dev.txt").exist? | ||
config.action_controller.perform_caching = true | ||
config.action_controller.enable_fragment_cache_logging = true | ||
|
||
config.cache_store = :memory_store |
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.
So are these conditional cache_store
settings just old Rails default config? I wasn't sure when I was running my own updates
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/rails@9c8716a
# Print deprecation notices to the Rails logger. | ||
config.active_support.deprecation = :log | ||
|
||
# Raise exceptions for disallowed deprecations. | ||
config.active_support.disallowed_deprecation = :raise |
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.
Is this setting now the default in development or something?
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 don't think the underlying default changed. It looks like the setting was just removed because DHH thought it wouldn't be useful for new apps: rails/rails@506d728
|
||
config.hosts += [ | ||
"authenticating-proxy.dev.gov.uk", | ||
] |
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.
What difference does this change make?
|
||
# Disable serving static files from `public/`, relying on NGINX/Apache to do so instead. | ||
config.public_file_server.enabled = ENV["RAILS_SERVE_STATIC_FILES"].present? |
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.
What's the thinking behind removing this?
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.
This is also a changed default that we skipped updating in 7.1/7.2:
|
||
# Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies. | ||
# config.force_ssl = true | ||
config.force_ssl = 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.
Is this correct? I thought our infrastructure resulted in our Rails apps only ever working with HTTP requests (because SSL is terminated at the edge of our network)?
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'm less familiar with assume_ssl
, so the same reasoning might not apply there)
config.log_level = ENV.fetch("RAILS_LOG_LEVEL", "info") | ||
|
||
# Use a different cache store in production. | ||
# Prevent health checks from clogging up the logs. | ||
config.silence_healthcheck_path = "/up" |
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 /up..?
# Do not dump schema after migrations. | ||
config.active_record.dump_schema_after_migration = false | ||
|
||
# Only use :id for inspections in production. | ||
config.active_record.attributes_for_inspect = [:id] |
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.
Is this a change to the existing production behaviour? If so, is it useful?
Bumps rails from 7.2.2 to 8.0.0.
Release notes
Sourced from rails's releases.
... (truncated)
Commits
dd8f718
Preparing for 8.0.0 releasef88e6ae
Merge pull request #53550 from tysongach/devcontainer-links43425c8
Bump deprecation message to 8.138bf52d
Add yarn.lock to allowed dirty files3de9afc
Merge pull request #53546 from matthewd/dst_deprecation_fixebcb66e
Merge pull request #53542 from Uaitt/remove-redundant-period-in-security-guides4f042a8
Merge pull request #53520 from Earlopain/fix-backtrace-env-gem-paths74608e5
Merge pull request #53533 from Earlopain/no-docs-for-rackup8ee2d3e
Merge pull request #53504 from SleeplessByte/fix/anchor-scroll-mobile473f2b2
Merge pull request #53515 from k-tsuchiya-jp/fix-53467You can trigger a rebase of this PR by commenting
@dependabot rebase
.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebase
will rebase this PR@dependabot recreate
will recreate this PR, overwriting any edits that have been made to it@dependabot merge
will merge this PR after your CI passes on it@dependabot squash and merge
will squash and merge this PR after your CI passes on it@dependabot cancel merge
will cancel a previously requested merge and block automerging@dependabot reopen
will reopen this PR if it is closed@dependabot close
will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditions
will show all of the ignore conditions of the specified dependency@dependabot ignore this major version
will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor version
will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependency
will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)