Avoid suggesting very unlikely changes in rails app:update - #41083
Merged
Conversation
ghiculescu
force-pushed
the
less-choices-in-app-update
branch
2 times, most recently
from
January 12, 2021 01:17
82fb9a5 to
ced58a1
Compare
ghiculescu
force-pushed
the
less-choices-in-app-update
branch
2 times, most recently
from
January 12, 2021 01:23
ec10b1d to
1841d8b
Compare
rails app:updaterails app:update
ghiculescu
marked this pull request as ready for review
January 12, 2021 01:35
ghiculescu
force-pushed
the
less-choices-in-app-update
branch
2 times, most recently
from
February 9, 2021 18:10
dda9507 to
1afb63b
Compare
rafaelfranca
requested changes
Feb 9, 2021
While upgrading a Rails 6 app to Rails 6.1, I noticed that `rails app:update` asks you to review some file changes that you'd basically never want to accept. In this PR, I propose we make the update task do a bit less, by not offering to overwrite files where the developer has almost certainly changed them from the default. Specific changes: Don't replace the following files, as they change very rarely at the framework level, so if the user has changes they almost certainly intend to keep them: - [config/boot.rb](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config/boot.rb.tt) - [config/environment.rb](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config/environment.rb.tt) - [config/storage.yml](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config/storage.yml.tt) - [config/spring.rb](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config/spring.rb.tt) - [config/cable.yml](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config/cable.yml.tt) - [config/puma.rb](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config/puma.rb.tt) - [config.ru](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config.ru.tt) Don't overwrite the config/locales directory. There's basically no chance you'll want to replace your current `config/locales/en.yml` with a [hello world](https://github.com/rails/rails/blob/master/railties/lib/rails/generators/rails/app/templates/config/locales/en.yml). Don't replace config/routes.rb. It's so unlikely that the user will want to replace their routes file with [an empty file](https://github.com/rails/rails/blob/master/railties/lib/rails/generators/rails/app/templates/config/routes.rb.tt). With these changes, you will still be prompted to accept/decline changes to these files when doing an update: - config/application.rb - config/environments/{development|test|production}.rb - All the default [initializers](https://github.com/rails/rails/tree/master/railties/lib/rails/generators/rails/app/templates/config/initializers) that you already have a copy of. I go back and forth on also opting some of these out, specifically `assets.rb`, `filter_parameter_logging.rb`, and `inflections.rb` which seem pretty likely to have been changed. - All the default [binstubs](https://github.com/rails/rails/tree/master/railties/lib/rails/generators/rails/app/templates/bin) that you already have a copy of. I decided to still make the user review these, as bugs here are going to be annoying to debug, but it is good to use the latest versions if possible.
ghiculescu
force-pushed
the
less-choices-in-app-update
branch
from
February 9, 2021 19:21
1afb63b to
1d1c9f9
Compare
Member
Author
|
@rafaelfranca thanks! I reverted my changes for |
ghiculescu
added a commit
to ghiculescu/rails
that referenced
this pull request
Jun 18, 2021
Currently when you make a new Rails app, we generate a lot of initializers. For new users, I think we should try and include as few as possible - the less files, the less daunting a new app is. And for upgrades I'd like to [continue to simplify the update process](rails#41083), in this case by not bringing back initializers you have probably already dismissed or modified. In this PR I'm proposing we remove two initializers: `application_controller_renderer.rb` and `cookies_serializer.rb`: **`application_controller_renderer.rb`**. This configures [`ActionController::Renderer`](https://api.rubyonrails.org/classes/ActionController/Renderer.html), for rendering views outside of controller actions. I don't think this is something most Rails apps will need (certainly not on day 1); users can configure this feature when they need it. **`cookies_serializer.rb`**. This was added for [Rails 4.1](https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#cookies-serializer). The behaviour is: - For new apps, the initializer says `:json`. - For upgraded apps that don't have the initializer, it is added with value `:marshal`. - If there's no initializer, the [default value](https://github.com/rails/rails/blob/c9a89a4067834a095a41084030b8c9ccdbce77d5/actionpack/lib/action_dispatch/middleware/cookies.rb#L589) is `:marshal`. Since nobody should be upgrading direct from Rails 4.0 to Rails 7.0, we can simplify this by using new framework defaults. So the behavior will now be: - For new apps, `config.load_defaults("7.0")` sets the value to `:json`. - The `new_framework_defaults_7_0.rb` file explains this, and suggests using `:hybrid` to be upgrade to JSON cookies. - No changes to [the code](https://github.com/rails/rails/blob/c9a89a4067834a095a41084030b8c9ccdbce77d5/actionpack/lib/action_dispatch/middleware/cookies.rb#L589); the default value is `:marshal` if you don't set one. So if you were not setting a `cookies_serializer` previously and you want to keep using `:marshal`, you'll need to explicitly set this before using `config.load_defaults("7.0")`, otherwise it will switch to `:json`. The upside of this is you won't get the `cookies_serializer.rb` file created for you every time you upgrade.
jyoun-godaddy
pushed a commit
to jyoun-godaddy/activestorage
that referenced
this pull request
Jul 5, 2022
Currently when you make a new Rails app, we generate a lot of initializers. For new users, I think we should try and include as few as possible - the less files, the less daunting a new app is. And for upgrades I'd like to [continue to simplify the update process](rails/rails#41083), in this case by not bringing back initializers you have probably already dismissed or modified. In this PR I'm proposing we remove two initializers: `application_controller_renderer.rb` and `cookies_serializer.rb`: **`application_controller_renderer.rb`**. This configures [`ActionController::Renderer`](https://api.rubyonrails.org/classes/ActionController/Renderer.html), for rendering views outside of controller actions. I don't think this is something most Rails apps will need (certainly not on day 1); users can configure this feature when they need it. **`cookies_serializer.rb`**. This was added for [Rails 4.1](https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#cookies-serializer). The behaviour is: - For new apps, the initializer says `:json`. - For upgraded apps that don't have the initializer, it is added with value `:marshal`. - If there's no initializer, the [default value](https://github.com/rails/rails/blob/c9a89a4067834a095a41084030b8c9ccdbce77d5/actionpack/lib/action_dispatch/middleware/cookies.rb#L589) is `:marshal`. Since nobody should be upgrading direct from Rails 4.0 to Rails 7.0, we can simplify this by using new framework defaults. So the behavior will now be: - For new apps, `config.load_defaults("7.0")` sets the value to `:json`. - The `new_framework_defaults_7_0.rb` file explains this, and suggests using `:hybrid` to be upgrade to JSON cookies. - No changes to [the code](https://github.com/rails/rails/blob/c9a89a4067834a095a41084030b8c9ccdbce77d5/actionpack/lib/action_dispatch/middleware/cookies.rb#L589); the default value is `:marshal` if you don't set one. So if you were not setting a `cookies_serializer` previously and you want to keep using `:marshal`, you'll need to explicitly set this before using `config.load_defaults("7.0")`, otherwise it will switch to `:json`. The upside of this is you won't get the `cookies_serializer.rb` file created for you every time you upgrade.
4 tasks
y-yagi
added a commit
to y-yagi/rails
that referenced
this pull request
Jun 11, 2024
This is a partial revert of rails#41083. Rails has improved `puma.rb` sometimes, for example, 06d614a and f719787. To allow users to know those improvements, I think we should update `puma.rb` by `app:update`.
y-yagi
added a commit
to y-yagi/rails
that referenced
this pull request
Jun 11, 2024
This is a partial revert of rails#41083. `puma.rb` may update by users, but Rails has improved `puma.rb` sometimes. For example, 06d614a and f719787. To allow users to know those improvements, I think we should update `puma.rb` by `app:update`.
2 tasks
Set2005
pushed a commit
to Set2005/fix-association-initialize-order
that referenced
this pull request
Jul 8, 2024
This is a partial revert of rails#41083. `puma.rb` may update by users, but Rails has improved `puma.rb` sometimes. For example, 06d614a and f719787. To allow users to know those improvements, I think we should update `puma.rb` by `app:update`.
DanielaVelasquez
pushed a commit
to DanielaVelasquez/rails
that referenced
this pull request
Oct 3, 2024
This is a partial revert of rails#41083. `puma.rb` may update by users, but Rails has improved `puma.rb` sometimes. For example, 06d614a and f719787. To allow users to know those improvements, I think we should update `puma.rb` by `app:update`.
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While upgrading a Rails 6 app to Rails 6.1, I noticed that
rails app:updateasks you to review some file changes that you'd basically never want to accept. In this PR, I propose we make the update task do a bit less, by not offering to overwrite files where the developer has almost certainly changed them from the default.Specific changes:
config/locales/en.ymlwith a hello world.With these changes, you will still be prompted to accept/decline changes to these files when doing an update:
assets.rb,filter_parameter_logging.rb,cors.rb,inflections.rbpretty much have to be changed to be useful.While here, I also made some touchups to the upgrade guide: