don't break if WEB_CONCURRENCY is set to a blank string#3837
Conversation
joshuay03
left a comment
There was a problem hiding this comment.
Thanks! Can you add a test for this case please? I think we have a bunch for this env var config.
@nateberkopec FYI, this might conflict with #3827
done, and made this to make sure it fails in main: #3839 although both seem stuck on github actions, not sure why. i did get runs working before, maybe i'm being throttled, will come back and check later |
|
(i could not get tests running locally, contributing.md seems to be out of date - if this is not expected let me know and i'll write it up) |
|
i just realized something obvious, any setting could have this problem, so .strip should be invoked on all settings. that's probably fine but should we
|
dentarg
left a comment
There was a problem hiding this comment.
Would it make sense to handle anything that's not a number more gracefully?
$ echo 'app { [200, {}, ["OK"]] }' | WEB_CONCURRENCY=foo puma --config /dev/stdin
<internal:kernel>:288:in 'Kernel#Integer': invalid value for Integer(): "foo" (ArgumentError)
from /Users/dentarg/.arm64_rubies/3.4.8/lib/ruby/gems/3.4.0/gems/puma-7.1.0/lib/puma/configuration.rb:252:in 'Puma::Configuration#puma_options_from_env'
from /Users/dentarg/.arm64_rubies/3.4.8/lib/ruby/gems/3.4.0/gems/puma-7.1.0/lib/puma/configuration.rb:233:in 'Puma::Configuration#puma_default_options'
from /Users/dentarg/.arm64_rubies/3.4.8/lib/ruby/gems/3.4.0/gems/puma-7.1.0/lib/puma/configuration.rb:180:in 'Puma::Configuration#initialize'
from /Users/dentarg/.arm64_rubies/3.4.8/lib/ruby/gems/3.4.0/gems/puma-7.1.0/lib/puma/cli.rb:94:in 'Class#new'
from /Users/dentarg/.arm64_rubies/3.4.8/lib/ruby/gems/3.4.0/gems/puma-7.1.0/lib/puma/cli.rb:94:in 'Puma::CLI#setup_options'
from /Users/dentarg/.arm64_rubies/3.4.8/lib/ruby/gems/3.4.0/gems/puma-7.1.0/lib/puma/cli.rb:43:in 'Puma::CLI#initialize'
from /Users/dentarg/.arm64_rubies/3.4.8/lib/ruby/gems/3.4.0/gems/puma-7.1.0/bin/puma:8:in 'Class#new'
from /Users/dentarg/.arm64_rubies/3.4.8/lib/ruby/gems/3.4.0/gems/puma-7.1.0/bin/puma:8:in '<top (required)>'
from /Users/dentarg/.arm64_rubies/3.4.8/bin/puma:25:in 'Kernel#load'
from /Users/dentarg/.arm64_rubies/3.4.8/bin/puma:25:in '<main>'
Feel free to make a issue or discussion about this |
Sure, but that could be another PR, I think this addition is worthwhile |
Description
Thank you for contributing! You're the best.
We can read your code, so consider leaving some comments here that are more about your motivations and decision making. Some things that may be helpful to address in your description:
Your checklist for this pull request
[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.