-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Fix #3044] Set conditional config defaults after CLI options are parsed and config files are loaded #3297
base: master
Are you sure you want to change the base?
Conversation
f977233
to
3ac0f65
Compare
@@ -352,7 +352,6 @@ def test_environment_app_env | |||
ENV['APP_ENV'] = 'test' | |||
|
|||
cli = Puma::CLI.new [] | |||
cli.send(:setup_options) |
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 think these are unnecessary, options are set up when initialised.
cli = Puma::CLI.new ['-w 2'] | ||
config = Puma.cli_config | ||
|
||
assert_equal true, config.options[:preload_app] |
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.
3ac0f65
to
d4f9f5d
Compare
conf.load | ||
preload = Puma.forkable? | ||
|
||
assert_equal preload, conf.options.default_options[:preload_app] |
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.
0b9d9ed
to
3caaef8
Compare
Are the test failures related to (these) tests now using preloading (and that causes some problem)? While #3044 is classed as a bug, it is quite the change in behaviour... so maybe the fix should be shipped in a major version? |
I think some of the phased restart failures are because of this clause. We should just make Edit: I think I misunderstood that condition. It seems that
I agree, and yes the issue was added to the |
3caaef8
to
dbb42e6
Compare
4b7d90d
to
fe5dec5
Compare
… parsed and config files are loaded
fe5dec5
to
ace3c3f
Compare
Description
Closes #3044
CLI
options are parsed duringCLI
initialisation, but afterConfiguration
initialisation. This PR changes this so that the parsing takes place duringConfiguration
initialisation. This ensures thatCLI
options are also used when some config defaults are conditionally set during initialisation.Note: Even with the changes in this PRpreload_app
isn't enabled by default if using#workers
in a config file. I'm happy to address that here too if prefered. Else we have an inconsistency with the cli and config file.Addressed in these changes. We now ensure that conditional defaults are also set when config files are loaded.
Note: The side effect of these changes which fixes the linked issue is that
preload_app
is now consistently enabled by default regardless of configuration method if these conditions are met.Your checklist for this pull request
[ci skip]
to the title of the PR.#issue
" to the PR description or my commit messages.