add ability to pass in all config values as environment variables. #1385

Merged
merged 4 commits into from Jan 8, 2017

Conversation

Projects
None yet
5 participants
@hramezani
Contributor

hramezani commented Nov 6, 2016

add environment variables that starts with GUNICORN_ like GUNICORN_WORKERS as settings.
skip variables that does not start with GUNICORN_, and also skip variable that not in settings like GUNICORN_FOO

@tilgovi

Overall, I like the idea, but interested to hear others' thoughts.

gunicorn/config.py
+ for k,v in self.env_orig.items():
+ if k.startswith('GUNICORN_'):
+ try:
+ name = k.split('GUNICORN_', 1)[1].lower()

This comment has been minimized.

@tilgovi

tilgovi Dec 20, 2016

Collaborator

I don't think this can ever fail if k.startswith('GUNICORN_').

@tilgovi

tilgovi Dec 20, 2016

Collaborator

I don't think this can ever fail if k.startswith('GUNICORN_').

gunicorn/app/base.py
@@ -154,6 +154,11 @@ def load_config(self):
if default_config is not None:
self.load_config_from_file(default_config)
+ # Load up environment configuration
+ env_vars = self.cfg.parse_env_vars()
+ for k,v in env_vars.items():

This comment has been minimized.

@tilgovi

tilgovi Dec 20, 2016

Collaborator

small linting nit here, space between ,v.

@tilgovi

tilgovi Dec 20, 2016

Collaborator

small linting nit here, space between ,v.

gunicorn/config.py
@@ -69,6 +69,19 @@ def set(self, name, value):
raise AttributeError("No configuration setting for: %s" % name)
self.settings[name].set(value)
+ def parse_env_vars(self):
+ env_vars = {}
+ for k,v in self.env_orig.items():

This comment has been minimized.

@tilgovi

tilgovi Dec 20, 2016

Collaborator

Same nitpick here about spacing.

@tilgovi

tilgovi Dec 20, 2016

Collaborator

Same nitpick here about spacing.

gunicorn/config.py
+ name = k.split('GUNICORN_', 1)[1].lower()
+ except:
+ continue
+ if not name or name not in self.settings:

This comment has been minimized.

@tilgovi

tilgovi Dec 20, 2016

Collaborator

name not in self.settings should be sufficient, no?

@tilgovi

tilgovi Dec 20, 2016

Collaborator

name not in self.settings should be sufficient, no?

@hramezani

This comment has been minimized.

Show comment
Hide comment
@hramezani

hramezani Dec 21, 2016

Contributor

@tilgovi thanks, i fix problems.

Contributor

hramezani commented Dec 21, 2016

@tilgovi thanks, i fix problems.

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Dec 22, 2016

Collaborator

I also like the idea, thanks! I haven't reviewed the code yet, but we should document all GUNICORN_ env variables before merging this.

Collaborator

berkerpeksag commented Dec 22, 2016

I also like the idea, thanks! I haven't reviewed the code yet, but we should document all GUNICORN_ env variables before merging this.

tests/test_config.py
+ os.environ["GUNICORN_FOO"] = "BAR"
+ with AltArgs():
+ app = NoConfigApp()
+ try:

This comment has been minimized.

@berkerpeksag

berkerpeksag Dec 22, 2016

Collaborator

This can be written as:

with pytest.raises(AttributeError):
    app.cfg.too
@berkerpeksag

berkerpeksag Dec 22, 2016

Collaborator

This can be written as:

with pytest.raises(AttributeError):
    app.cfg.too
tests/test_config.py
@@ -285,3 +285,25 @@ def test_always_use_configured_logger():
c.set('statsd_host', 'localhost:12345')
# still uses custom logger over statsd
assert c.logger_class == MyLogger
+
+def test_load_enviroment_variables_config():
+ os.environ["GUNICORN_WORKERS"] = "4"

This comment has been minimized.

@berkerpeksag

berkerpeksag Dec 22, 2016

Collaborator

We need to unset these environment variables. I don't know what's the best practice to do this in pytest though (I'd suggest writing a context manager if we were using unittest -- perhaps you need a fixture?)

@berkerpeksag

berkerpeksag Dec 22, 2016

Collaborator

We need to unset these environment variables. I don't know what's the best practice to do this in pytest though (I'd suggest writing a context manager if we were using unittest -- perhaps you need a fixture?)

@hramezani

This comment has been minimized.

Show comment
Hide comment
@hramezani

hramezani Dec 22, 2016

Contributor

@berkerpeksag Thanks,
this PR accept KNOWN_SETTINGS in this format GUNICORN_setting_name. known setting names are in Settings. for example GUNICORN_bind, GUNICORN_worker_class, ...
Is this a good idea that a write a paragraph in settings page and mention that we can change all of settings in this page by setting environment variables with abov format?

Contributor

hramezani commented Dec 22, 2016

@berkerpeksag Thanks,
this PR accept KNOWN_SETTINGS in this format GUNICORN_setting_name. known setting names are in Settings. for example GUNICORN_bind, GUNICORN_worker_class, ...
Is this a good idea that a write a paragraph in settings page and mention that we can change all of settings in this page by setting environment variables with abov format?

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 22, 2016

Owner

What is the use case for this change? Can you elaborate about it?

Owner

benoitc commented Dec 22, 2016

What is the use case for this change? Can you elaborate about it?

@hramezani

This comment has been minimized.

Show comment
Hide comment
@hramezani

hramezani Dec 23, 2016

Contributor

@benoitc i saw Ability to pass in all config values as environment variables in issue list and @tilgovi commented:

I don't see any reason not to support it. Pull requests welcome.

finally i decide to do this.

with this change we can change gunicorn settings with environment variables.

Contributor

hramezani commented Dec 23, 2016

@benoitc i saw Ability to pass in all config values as environment variables in issue list and @tilgovi commented:

I don't see any reason not to support it. Pull requests welcome.

finally i decide to do this.

with this change we can change gunicorn settings with environment variables.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Dec 23, 2016

Collaborator

There are times, especially in these days of containerization, where it is much easier to change an environment variable than to change the command line invocation or a configuration file.

Collaborator

tilgovi commented Dec 23, 2016

There are times, especially in these days of containerization, where it is much easier to change an environment variable than to change the command line invocation or a configuration file.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Dec 23, 2016

Collaborator

We can do this in a separate PR, but I'm wondering if there is any reason to clear these keys from the environment when we spawn workers. I'm just wondering if any settings might contain information that we should try to prevent from leaking to workers and possibly then into a response.

Collaborator

tilgovi commented Dec 23, 2016

We can do this in a separate PR, but I'm wondering if there is any reason to clear these keys from the environment when we spawn workers. I'm just wondering if any settings might contain information that we should try to prevent from leaking to workers and possibly then into a response.

@hramezani

This comment has been minimized.

Show comment
Hide comment
@hramezani

hramezani Dec 25, 2016

Contributor

@benoitc @tilgovi
Is there anything that I do?

Contributor

hramezani commented Dec 25, 2016

@benoitc @tilgovi
Is there anything that I do?

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Dec 26, 2016

Collaborator

Is this a good idea that a write a paragraph in settings page and mention that we can change all of settings in this page by setting environment variables with abov format?

It would be nice to add something like "Environment variable: GUNICORN_FOO" (or just plain GUNICORN_FOO) to every setting. For example, let's take a look at bind: http://docs.gunicorn.org/en/stable/settings.html#bind With "-b ADDRESS, --bind ADDRESS", we know that we can pass -b or --bind to gunicorn. We can document the env variable in the same way:

bind

* -b ADDRESS, --bind ADDRESS (CLI)
* GUNICORN_BIND (env variable)
* ['127.0.0.1:8000'] (default value)
Collaborator

berkerpeksag commented Dec 26, 2016

Is this a good idea that a write a paragraph in settings page and mention that we can change all of settings in this page by setting environment variables with abov format?

It would be nice to add something like "Environment variable: GUNICORN_FOO" (or just plain GUNICORN_FOO) to every setting. For example, let's take a look at bind: http://docs.gunicorn.org/en/stable/settings.html#bind With "-b ADDRESS, --bind ADDRESS", we know that we can pass -b or --bind to gunicorn. We can document the env variable in the same way:

bind

* -b ADDRESS, --bind ADDRESS (CLI)
* GUNICORN_BIND (env variable)
* ['127.0.0.1:8000'] (default value)
@hramezani

This comment has been minimized.

Show comment
Hide comment
@hramezani

hramezani Dec 26, 2016

Contributor

@berkerpeksag thanks for reply, I add documents.

Contributor

hramezani commented Dec 26, 2016

@berkerpeksag thanks for reply, I add documents.

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Dec 26, 2016

Collaborator

Thanks, docs/source/settings.rst is automatically generated. I prefer modifying https://github.com/benoitc/gunicorn/blob/master/docs/gunicorn_ext.py#L39 instead of manually adding these names. You can then run make -C docs html to update settings.rst (note that make html will make some unrelated changes in settings.rst -- please discard them before committing your changes)

Collaborator

berkerpeksag commented Dec 26, 2016

Thanks, docs/source/settings.rst is automatically generated. I prefer modifying https://github.com/benoitc/gunicorn/blob/master/docs/gunicorn_ext.py#L39 instead of manually adding these names. You can then run make -C docs html to update settings.rst (note that make html will make some unrelated changes in settings.rst -- please discard them before committing your changes)

docs/source/settings.rst
@@ -17,6 +17,7 @@ config
~~~~~~
* ``-c CONFIG, --config CONFIG``
+* ``GUNICORN_CONFIG (env variable)``

This comment has been minimized.

@berkerpeksag

berkerpeksag Dec 26, 2016

Collaborator

"(env variable)" can be removed. I used it as an annotation in my example :)

@berkerpeksag

berkerpeksag Dec 26, 2016

Collaborator

"(env variable)" can be removed. I used it as an annotation in my example :)

@hramezani

This comment has been minimized.

Show comment
Hide comment
@hramezani

hramezani Dec 26, 2016

Contributor

@berkerpeksag Thanks, whta is your opinion about adding a property like cli with name env_var and type boolean to Setting class? this can be used for following purpose:

  1. add variables to config if this propery = True.
  2. disable this functionality for some setting.
  3. generate document based on this property.
Contributor

hramezani commented Dec 26, 2016

@berkerpeksag Thanks, whta is your opinion about adding a property like cli with name env_var and type boolean to Setting class? this can be used for following purpose:

  1. add variables to config if this propery = True.
  2. disable this functionality for some setting.
  3. generate document based on this property.
@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 27, 2016

Owner

I'm not sure i'm comfortable with this change, it introduces an extra level of complexity in the configuration, having to document the variables to use is a sign. What about instead having a simple GUNICORN_CMD_ARGS (or whatever its name) where you pass the command line arguments that could be parsed like usual? Ie:

GUNICORN_CMD_ARGS ="--bind=127.0.0.1 --workers=3" ...

So we wouldn't have to document each changes and could reuse the current code to handle it. Thoughts?

Owner

benoitc commented Dec 27, 2016

I'm not sure i'm comfortable with this change, it introduces an extra level of complexity in the configuration, having to document the variables to use is a sign. What about instead having a simple GUNICORN_CMD_ARGS (or whatever its name) where you pass the command line arguments that could be parsed like usual? Ie:

GUNICORN_CMD_ARGS ="--bind=127.0.0.1 --workers=3" ...

So we wouldn't have to document each changes and could reuse the current code to handle it. Thoughts?

@hramezani

This comment has been minimized.

Show comment
Hide comment
@hramezani

hramezani Dec 27, 2016

Contributor

@benoitc Thanks, I change PR as your previous comment.

Contributor

hramezani commented Dec 27, 2016

@benoitc Thanks, I change PR as your previous comment.

@decal

This comment has been minimized.

Show comment
Hide comment
@decal

decal Jan 6, 2017

Looks even better than the Apache 2.4 envvars support..

decal commented Jan 6, 2017

Looks even better than the Apache 2.4 envvars support..

@benoitc benoitc self-requested a review Jan 8, 2017

@benoitc

benoitc approved these changes Jan 8, 2017

@berkerpeksag

The PR looks pretty good to me, thanks! Just left some trivial comments.

We also need to document the new env variable.

gunicorn/config.py
@@ -69,6 +69,12 @@ def set(self, name, value):
raise AttributeError("No configuration setting for: %s" % name)
self.settings[name].set(value)
+ def get_gunicorn_env_var(self):
+ env_vars = []

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Unless I'm missing something obvious, env_vars is not needed here:

if 'GUNICORN_CMD_ARGS' in self.env_orig:
    return self.env_orig['GUNICORN_CMD_ARGS'].split()
return []
@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Unless I'm missing something obvious, env_vars is not needed here:

if 'GUNICORN_CMD_ARGS' in self.env_orig:
    return self.env_orig['GUNICORN_CMD_ARGS'].split()
return []
gunicorn/config.py
+ def get_gunicorn_env_var(self):
+ env_vars = []
+ if 'GUNICORN_CMD_ARGS' in self.env_orig:
+ env_vars = self.env_orig['GUNICORN_CMD_ARGS'].split()

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Did you consider using shlex.split() here?

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Did you consider using shlex.split() here?

gunicorn/config.py
@@ -69,6 +69,12 @@ def set(self, name, value):
raise AttributeError("No configuration setting for: %s" % name)
self.settings[name].set(value)
+ def get_gunicorn_env_var(self):

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

I'm not a native English speaker, but perhaps we can rename it to something like get_cmd_args_from_env? get_gunicorn_env_var sounds too generic to me. Ping @tilgovi :)

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

I'm not a native English speaker, but perhaps we can rename it to something like get_cmd_args_from_env? get_gunicorn_env_var sounds too generic to me. Ping @tilgovi :)

gunicorn/app/base.py
+ env_vars = self.cfg.get_gunicorn_env_var()
+ if env_vars:
+ env_args = parser.parse_args(env_vars)
+ for k, v in env_args.__dict__.items():

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Wouldn't vars(env_args).items() be better?

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Wouldn't vars(env_args).items() be better?

tests/test_config.py
+ app = NoConfigApp()
+ assert app.cfg.workers == 4
+
+def test_cli_overrides_enviroment_variables_module():

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Tests are look great, thank you! :) Could you also add a test that passes an invalid config option (e.g os.environ["GUNICORN_CMD_ARGS"] = "--monty-python"?

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Tests are look great, thank you! :) Could you also add a test that passes an invalid config option (e.g os.environ["GUNICORN_CMD_ARGS"] = "--monty-python"?

@hramezani

This comment has been minimized.

Show comment
Hide comment
@hramezani

hramezani Jan 8, 2017

Contributor

@berkerpeksag Thanks, i fix all your comments.

Contributor

hramezani commented Jan 8, 2017

@berkerpeksag Thanks, i fix all your comments.

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Looks great, thanks! I think you forgot to address two of my previous comments:

  • We need to cleanup the env variables we set in tests. We can do this in the following way:

    def test_cli_overrides_enviroment_variables_module(monkeypatch):
        monkeypatch.setenv("GUNICORN_CMD_ARGS", "--workers=4")
        # test code
  • GUNICORN_CMD_ARGS needs to be documented. We can mention it in docs/source/run.rst and docs/gunicorn_ext.py. We can leave this to a separate PR if you want.

Collaborator

berkerpeksag commented Jan 8, 2017

Looks great, thanks! I think you forgot to address two of my previous comments:

  • We need to cleanup the env variables we set in tests. We can do this in the following way:

    def test_cli_overrides_enviroment_variables_module(monkeypatch):
        monkeypatch.setenv("GUNICORN_CMD_ARGS", "--workers=4")
        # test code
  • GUNICORN_CMD_ARGS needs to be documented. We can mention it in docs/source/run.rst and docs/gunicorn_ext.py. We can leave this to a separate PR if you want.

tests/test_config.py
+ os.environ["GUNICORN_CMD_ARGS"] = "--foo=bar"
+ with AltArgs():
+ with pytest.raises(SystemExit):
+ app = NoConfigApp()

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Style nit: No need to set NoConfigApp() to app. Just call NoConfigApp().

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Style nit: No need to set NoConfigApp() to app. Just call NoConfigApp().

@@ -285,3 +285,21 @@ def test_always_use_configured_logger():
c.set('statsd_host', 'localhost:12345')
# still uses custom logger over statsd
assert c.logger_class == MyLogger
+

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Style nit: Please add two empty lines after each tests.

@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Style nit: Please add two empty lines after each tests.

@hramezani

This comment has been minimized.

Show comment
Hide comment
@hramezani

hramezani Jan 8, 2017

Contributor

@berkerpeksag thanks!, comments fixed.
can you help me for documentation? is this ok to add a note or paragraph in Settings and Running Gunicorn page, and describe that we can use all command line config in env vars in bellow format?
GUNICORN_CMD_ARGS ="--bind=127.0.0.1 --workers=3" ...

Contributor

hramezani commented Jan 8, 2017

@berkerpeksag thanks!, comments fixed.
can you help me for documentation? is this ok to add a note or paragraph in Settings and Running Gunicorn page, and describe that we can use all command line config in env vars in bellow format?
GUNICORN_CMD_ARGS ="--bind=127.0.0.1 --workers=3" ...

@berkerpeksag berkerpeksag merged commit 3f9eace into benoitc:master Jan 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Jan 8, 2017

Collaborator

Thanks for the PR, @hramezani! :)

is this ok to add a note or paragraph in Settings and Running Gunicorn page, and describe that we can use all command line config in env vars in bellow format?
GUNICORN_CMD_ARGS ="--bind=127.0.0.1 --workers=3"

Yes, that sounds good to me. Let's open another pull request for documentation.

Collaborator

berkerpeksag commented Jan 8, 2017

Thanks for the PR, @hramezani! :)

is this ok to add a note or paragraph in Settings and Running Gunicorn page, and describe that we can use all command line config in env vars in bellow format?
GUNICORN_CMD_ARGS ="--bind=127.0.0.1 --workers=3"

Yes, that sounds good to me. Let's open another pull request for documentation.

@hramezani

This comment has been minimized.

Show comment
Hide comment
@hramezani

hramezani Jan 8, 2017

Contributor

@berkerpeksag thanks very much for your help. i open another pull request for documentation soon.

Contributor

hramezani commented Jan 8, 2017

@berkerpeksag thanks very much for your help. i open another pull request for documentation soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment