-
Notifications
You must be signed in to change notification settings - Fork 7
Rename some configuration variables #223
base: master
Are you sure you want to change the base?
Conversation
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 agree in general with the PR, except of changing PORT
to HTTP_PORT
since it breaks implicit compatibility with Foreman.
I am okay with renaming the variable as long Foreman works. We can pass port from Procfile to Puma through -p $PORT
argument, but that will be overriden by the config file – so there needs to be some check that the port was given externally via CLI.
config/puma.rb
Outdated
@@ -1,6 +1,6 @@ | |||
require './config/config' | |||
environment Config.rack_env | |||
port Config.port | |||
port Config.http_port |
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.
$PORT
is a special env variable handled by Foreman, so this is a breaking change.
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.
Okay, I’ve removed this change.
@@ -26,22 +26,22 @@ module Config | |||
|
|||
# Override -- value is returned or the set default. | |||
override :database_timeout, 10, int | |||
override :db_pool, 5, int | |||
override :db_pool_max_size, 3, int |
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.
Any explanation for this?
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.
We already use 3 on production for a long time, because 5 is too many – this value is per process and Puma spawns multiple processes.
I’ve rebased against master and removed rename of |
No description provided.