Do not update load_defaults version when running app:update
#31951
Conversation
| FileUtils.cd(destination_root) do | ||
| config = "config/application.rb" | ||
| content = File.read(config) | ||
| File.write(config, content.gsub(/config\.load_defaults #{Rails::VERSION::STRING.to_f}/, "config.load_defaults 5.2")) |
There was a problem hiding this comment.
Let's use 5.1, so this test still applies when backported to 5.2
| @@ -130,6 +130,9 @@ def config_when_updating | |||
| assets_config_exist = File.exist?("config/initializers/assets.rb") | |||
| csp_config_exist = File.exist?("config/initializers/content_security_policy.rb") | |||
|
|
|||
| load_defaults = File.read("config/application.rb").match(/config\.load_defaults (\d\.\d)/) | |||
There was a problem hiding this comment.
I think we also need a test that this matches the generated content (e.g., right now it would stop working for version 10.0, but no test would fail). Do you have any ideas how we could do that?
There was a problem hiding this comment.
Two other thoughts on the regex:
- Start with
/^\s*, to ignore commented lines. - Technically, the argument is allowed to be a string (because
6.10.to_sand"6.10".to_sare very different)... but maybe that's not worth worrying about for now.
| @@ -130,6 +130,9 @@ def config_when_updating | |||
| assets_config_exist = File.exist?("config/initializers/assets.rb") | |||
| csp_config_exist = File.exist?("config/initializers/content_security_policy.rb") | |||
|
|
|||
| load_defaults = File.read("config/application.rb").match(/config\.load_defaults (\d\.\d)/) | |||
| @config_target_version = load_defaults[1] if load_defaults | |||
There was a problem hiding this comment.
If there's no load_defaults in there, but we're upgrading... maybe this should be:
@config_target_version = load_defaults[1] || "5.0"
There was a problem hiding this comment.
If there's no config.load_defaults line in the file, and we regenerate it with config.load_defaults 5.0, we've opted the app into a set of defaults that they weren't getting before - which is what we're trying to avoid here.
I think we should skip the load_defaults line in the template if we didn't find it in the current file.
There was a problem hiding this comment.
Yeah, it's a fair point. But since I would like to recommend using load_defaults, want to avoid skip.
So, I added version 4.2 which does not affect existing configs and used it there.
@matthewd What do you think?
There was a problem hiding this comment.
I think omitting it for upgrades is fine... people should really be doing a stepwise upgrade anyway, so it will be rare for 5.2 to see an app that was never upgraded to 5.0 or 5.1. (That's also why I went for 5.0... it's not correct, but seemed less bother than making the whole line optional just for a corner case.)
Offering 4.2 seems more likely to be confusing, because it's not really true... IIRC we changed a number of the 'real'/baseline defaults in 5.0?
There was a problem hiding this comment.
My concern was belongs_to_required_by_default. It effects to the whole application.
But I fixed it to 5.0 in order to agree that 4.2 is confusing and that it seems to have responded somewhat already with the update from 5.0 to 5.1.
(I also do not agree much to make the whole line an option for that corner case)
|
The other option, instead of parsing the file, would be to remember the |
y-yagi
force-pushed
the
do_not_update_load_defaults_in_app_update
branch
from
dff4a8a
to
a4b9457
Compare
👍 Yeah, that is a good approach. I updated. |
y-yagi
force-pushed
the
do_not_update_load_defaults_in_app_update
branch
from
a4b9457
to
0defd1b
Compare
Incompatible settings are included in the settings set by `load_defaults`. So, I think that target version should be updated by a user when becomes available, and should not be updated with `app:update`.
y-yagi
force-pushed
the
do_not_update_load_defaults_in_app_update
branch
from
0defd1b
to
66b4752
Compare
Incompatible settings are included in the settings set by `load_defaults`. So, I think that target version should be updated by a user when becomes available, and should not be updated with `app:update`.
Incompatible settings are included in the settings set by
load_defaults.So, I think that target version should be updated by a user when becomes available, and should not be updated with
app:update.