Implement user_supplied_options behavior. #1234
Merged
Conversation
This file contains 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
schneems
force-pushed
the
schneems/puma-port-problem-patched-per-prior-parley-possibly
branch
from
e3bd773
to
0e519a8
Compare
nateberkopec
reviewed
Mar 6, 2017
test/test_helper.rb
Outdated
| @@ -4,9 +4,11 @@ | |||
| begin | |||
| require "bundler/setup" | |||
| rescue LoadError | |||
There was a problem hiding this comment.
Aha, yeah, this is probably the correct fix.
|
@schneems is this done now? |
|
Yes, please review. Ping me with any questions. |
nateberkopec
reviewed
Mar 8, 2017
lib/puma/configuration.rb
Outdated
| @@ -151,14 +143,17 @@ def initialize(options={}, &blk) | |||
| attr_reader :options, :plugins | |||
|
|
|||
| def configure(&blk) | |||
| @options.shift | |||
| DSL.new(@options, self)._run(&blk) | |||
| blk.call(@user_dsl, @file_dsl, @default_dsl) | |||
There was a problem hiding this comment.
Sure if you prefer that.
There was a problem hiding this comment.
no biggie, I just have a habit of using yield
nateberkopec
reviewed
Mar 8, 2017
test/test_rack_handler.rb
Outdated
| end | ||
|
|
||
| def test_default_port_when_no_config_file | ||
| options = {} |
There was a problem hiding this comment.
That was a copy pasta error. Will fix
nateberkopec
reviewed
Mar 8, 2017
test/test_rack_handler.rb
Outdated
|
|
||
| def test_config_wins_over_default | ||
| file_port = 6001 | ||
| @options = {} |
nateberkopec
reviewed
Mar 8, 2017
test/test_rack_handler.rb
Outdated
| def test_user_port_wins_over_config | ||
| user_port = 5001 | ||
| file_port = 6001 | ||
| @options = {} |
The RackHandler for Puma has configuration options that I want to test without booting up a server. By separating out into a new method we can do this easily.
When options are passed to the Puma rack handler it is unknown if the options were set via a framework as a default or via a user. Puma currently has 3 different sources of configuration, the user via command line, the config files, and defaults. Rails 5.1+ will record the values actually specified by the user versus the values specified by the frameworks. It passes these values to the Rack handler and now it's up to Puma to do something with that information. When only framework defaults are passed it will set ``` options[:user_supplied_options] = [] ``` When one or more options are specified by the user such as `:Port` then those keys will be in the array. In that example it will look like this ``` options[:user_supplied_options] = [:Port] ``` This change is 100% backwards compatible. If the framework is older and does not pass this information then the `user_supplied_options` will not be set, in that case we assume all values are user supplied. Internally we accomplish this separation by replacing `LeveledOptions` which was a generic way of specifying options with different priorities with a more explicit `UserFileDefaultOptions` this assumes only 3 levels of options and it will use them in the order supplied (user config wins over file based config wins over defaults). Now instead of using 1 dsl to set all values, we use 3. A user dsl, a file dsl and a Configuration.new` will return all 3 DSLs to the block. It's up to the person using the block to use the correct dsl corresponding to the source of data they are getting.
schneems
force-pushed
the
schneems/puma-port-problem-patched-per-prior-parley-possibly
branch
from
4e892e7
to
852f52f
Compare
|
@schneems Hi Richard, I think this commit broke the correct loading of puma environment configuration file. Before 3.8.0, I was able to run On 3.8.0, it's not picking up the Might be related? #1269 |
|
I'm not sure. Can you try with 3.9.0 and if it's still broken open a new issue with an example app? |
|
Yes 3.9.0 is still broken. I can open a new issue for sure !
…--
Mathieu Allaire
Le 2 juin 2017 16:46 -0400, Richard Schneeman <notifications@github.com>, a écrit :
I'm not sure. Can you try with 3.9.0 and if it's still broken open a new issue with an example app?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
7 tasks
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.
This is the Puma side of rails/rails#28137
When options are passed to the Puma rack handler it is unknown if the options were set via a framework as a default or via a user. Puma currently has 3 different sources of configuration, the user via command line, the config files, and defaults.
Rails 5.1+ will record the values actually specified by the user versus the values specified by the frameworks. It passes these values to the Rack handler and now it's up to Puma to do something with that information.
When only framework defaults are passed it will set
When one or more options are specified by the user such as
:Portthen those keys will be in the array. In that example it will look like thisThis change is 100% backwards compatible. If the framework is older and does not pass this information then the
user_supplied_optionswill not be set, in that case we assume all values are user supplied.Internally we accomplish this separation by replacing
LeveledOptionswhich was a generic way of specifying options with different priorities with a more explicitUserFileDefaultOptionsthis assumes only 3 levels of options and it will use them in the order supplied (user config wins over file based config wins over defaults).Now instead of using 1 dsl to set all values, we use 3. A user dsl, a file dsl and a
Configuration.newwill return all 3 DSLs to the block. It's up to the person using the block to use the correct dsl corresponding to the source of data they are getting.