Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Fix #3044] Set conditional config defaults after CLI options are parsed and config files are loaded #3297

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Fix #3044] Set conditional config defaults after CLI options are parsed and config files are loaded #3297

wants to merge 1 commit into from
+74 −20

Conversation

joshuay03
Copy link
Contributor

@joshuay03 joshuay03 commented Dec 24, 2023 •
edited

Description

Closes #3044

CLI options are parsed during CLI initialisation, but after Configuration initialisation. This PR changes this so that the parsing takes place during Configuration initialisation. This ensures that CLI options are also used when some config defaults are conditionally set during initialisation.

Note: Even with the changes in this PR preload_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

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@joshuay03 joshuay03 force-pushed the set-conditional-config-defaults-after-cli-options-parse branch from f977233 to 3ac0f65 Compare December 24, 2023 09:49
joshuay03
joshuay03 commented Dec 24, 2023
View reviewed changes
test/test_cli.rb
@@ -352,7 +352,6 @@ def test_environment_app_env
ENV['APP_ENV'] = 'test'

cli = Puma::CLI.new []
cli.send(:setup_options)
Copy link
Contributor Author

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.

joshuay03
joshuay03 commented Dec 24, 2023
View reviewed changes
test/test_cli.rb
cli = Puma::CLI.new ['-w 2']
config = Puma.cli_config

assert_equal true, config.options[:preload_app]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before patch:
Screenshot 2023-12-24 at 7 50 33 pm

@joshuay03 joshuay03 force-pushed the set-conditional-config-defaults-after-cli-options-parse branch from 3ac0f65 to d4f9f5d Compare December 24, 2023 10:17
joshuay03
joshuay03 commented Dec 24, 2023
View reviewed changes
test/test_config.rb
conf.load
preload = Puma.forkable?

assert_equal preload, conf.options.default_options[:preload_app]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before patch:
Screenshot 2023-12-24 at 8 19 44 pm

@joshuay03 joshuay03 changed the title [Fix #3044] Set conditional config defaults after CLI options are parsed [Fix #3044] Set conditional config defaults after CLI options are parsed and config files are loaded Dec 24, 2023
@joshuay03 joshuay03 force-pushed the set-conditional-config-defaults-after-cli-options-parse branch 2 times, most recently from 0b9d9ed to 3caaef8 Compare December 24, 2023 10:29
@dentarg
Copy link
Member

dentarg commented Dec 24, 2023

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?

@joshuay03
Copy link
Contributor Author

joshuay03 commented Dec 25, 2023 •
edited

Are the test failures related to (these) tests now using preloading (and that causes some problem)?

I think some of the phased restart failures are because of this clause. We should just make fork_worker one of the conditions to not default preload_app to true if they conflict. Addressed in these changes. I'll come back and figure out the remaining failures after the break. Merry Christmas 🎄

Edit:

I think I misunderstood that condition. It seems that preload_app is compatible fork_worker, but incompatible with pure phased restart. So the real fix is to make preload_app disabled explicitly in these tests. Addressed in:

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 agree, and yes the issue was added to the 7.0.0 milestone.

@joshuay03 joshuay03 force-pushed the set-conditional-config-defaults-after-cli-options-parse branch from 3caaef8 to dbb42e6 Compare December 25, 2023 00:52
@joshuay03 joshuay03 force-pushed the set-conditional-config-defaults-after-cli-options-parse branch 8 times, most recently from 4b7d90d to fe5dec5 Compare January 2, 2024 22:54
@dentarg dentarg added waiting-for-review Waiting on review from anyone v7 Breaking changes for v7 labels Apr 8, 2024
@joshuay03 joshuay03 force-pushed the set-conditional-config-defaults-after-cli-options-parse branch from fe5dec5 to ace3c3f Compare April 16, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
v7 Breaking changes for v7 waiting-for-review Waiting on review from anyone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

preload_app doesn't default to true if more than 1 worker
2 participants
@joshuay03 @dentarg