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.

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

Support Dictionary Config setting. #1602

Merged
merged 5 commits into from Oct 30, 2017

Conversation

Projects
None yet
5 participants
@tilgovi
Copy link
Collaborator

tilgovi commented Sep 18, 2017

Taking over #1110 as per the OP request.

@tilgovi tilgovi changed the title Feature/1087 dict config Support Dictionary Config on the command line. Sep 18, 2017

@tilgovi tilgovi changed the title Support Dictionary Config on the command line. Support Dictionary Config setting. Sep 18, 2017

if dictConfig and cfg.logconfig_dict:
config = CONFIG_DEFAULTS.copy()
config.update(cfg.logconfig_dict)
dictConfig(config)

This comment has been minimized.

@berkerpeksag

berkerpeksag Sep 18, 2017

Collaborator

We probably need to catch the following exceptions and raise RuntimeError(message_from_catched_exception) like we already did for logconfig in line 247:

If an error is encountered during configuration, this function will raise a ValueError, TypeError, AttributeError or ImportError with a suitably descriptive message.

validator = validate_dict
default = {}
desc = """\
The log config dictionary to use, using the standard Python logging

This comment has been minimized.

@berkerpeksag

berkerpeksag Sep 18, 2017

Collaborator

We need to add a versionadded label.

default = {}
desc = """\
The log config dictionary to use, using the standard Python logging
module's dictConfig format added in python 2.7.

This comment has been minimized.

@berkerpeksag

berkerpeksag Sep 18, 2017

Collaborator

dictConfig -> ``dictConfig``

This comment has been minimized.

desc = """\
The log config dictionary to use, using the standard Python logging
module's dictConfig format added in python 2.7.
If available, this takes precedence over logconfig, which uses the older

This comment has been minimized.

@berkerpeksag

berkerpeksag Sep 18, 2017

Collaborator

logconfig -> :ref:`--log-config `

The log config dictionary to use, using the standard Python logging
module's dictConfig format added in python 2.7.
If available, this takes precedence over logconfig, which uses the older
fileConfig format.

This comment has been minimized.

@berkerpeksag

berkerpeksag Sep 18, 2017

Collaborator

fileConfig -> ``fileConfig``

@@ -1309,6 +1309,19 @@ class LogConfig(Setting):
"""


class LogConfigDict(Setting):
name = "logconfig_dict"

This comment has been minimized.

@berkerpeksag

berkerpeksag Sep 18, 2017

Collaborator

Do we need to add cli = ["--log-config-dict"]?

default = {}
desc = """\
The log config dictionary to use, using the standard Python logging
module's dictConfig format added in python 2.7.

This comment has been minimized.

@berkerpeksag

berkerpeksag Sep 18, 2017

Collaborator

It might be better to remove "added in python 2.7" and print a warning if dictConfig is None in gunicorn/glogging.py.

@tilgovi tilgovi referenced this pull request Oct 27, 2017

Closed

release 19.8.0 #1634

@tilgovi tilgovi force-pushed the feature/1087-dict-config branch from a12b878 to 783c9be Oct 30, 2017

@tilgovi

This comment has been minimized.

Copy link
Collaborator Author

tilgovi commented Oct 30, 2017

@berkerpeksag I think I addressed all your comments. Thanks for the review.

@berkerpeksag
Copy link
Collaborator

berkerpeksag left a comment

I left two comments but they aren't blockers and can be addressed in separate PRs. Thanks!

section = "Logging"
cli = ["--log-config-dict"]
validator = validate_dict
default = {}

This comment has been minimized.

@berkerpeksag

berkerpeksag Oct 30, 2017

Collaborator

This might be a little bit misleading since we use glogging.CONFIG_DEFAULTS as default. Should we document this or keep it as implementation detail?

This comment has been minimized.

@tilgovi

tilgovi Oct 30, 2017

Author Collaborator

Maybe we should set the default as None? Would that be more "unset" / "undefined" / "default" to people?

Maybe it doesn't matter much either way.

desc = """\
The log config dictionary to use, using the standard Python
logging module's dictionary configuration format. This option
takes precedence over the :ref:`logconfig` option, which uses the

This comment has been minimized.

@berkerpeksag

berkerpeksag Oct 30, 2017

Collaborator

This is a minor issue, but I'd say let's deprecate --log-config documentation-only and tell users to switch to --log-config-dict.

This comment has been minimized.

@tilgovi

tilgovi Oct 30, 2017

Author Collaborator

If you can explain a bit more, I will do a PR. Or I will review a PR. Thanks!

@tilgovi tilgovi merged commit 0e63307 into master Oct 30, 2017

2 of 9 checks passed

couverture-io/py26 Code coverage has worsened
Details
couverture-io/py27 Code coverage has worsened
Details
couverture-io/py34 Code coverage has worsened
Details
couverture-io/py35 Code coverage has worsened
Details
couverture-io/py36 Code coverage has worsened
Details
couverture-io/py36-dev Code coverage has worsened
Details
couverture-io/py37 Code coverage has worsened
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tilgovi tilgovi deleted the feature/1087-dict-config branch Oct 30, 2017

@wking wking referenced this pull request Nov 3, 2017

Closed

dictConfig in logconfig #1087

fofanov pushed a commit to fofanov/gunicorn that referenced this pull request Mar 16, 2018

class LogConfigDict(Setting):
name = "logconfig_dict"
section = "Logging"
cli = ["--log-config-dict"]

This comment has been minimized.

@rthille

rthille Oct 29, 2018

This doesn't seem to work, as there doesn't seem to be any conversion from the string supplied by the shell in some ARGV[x] to an actual dict:

bash-3.2$ env/bin/gunicorn --version
env/bin/gunicorn --version
gunicorn (version 19.9.0)
bash-3.2$ env/bin/gunicorn --log-config-dict {} 'flask.main:app'
env/bin/gunicorn --log-config-dict {} 'flask.main:app'

Error: Value is not a dictionary: {} 

This comment has been minimized.

@javabrett

javabrett Oct 29, 2018

Contributor

Right, maybe cli should have been left off this option until someone JSON-parses it. Can you use config file?

Also https://stackoverflow.com/a/18609361/452210 re argparse capabilities in this area.

This comment has been minimized.

@rthille

rthille Oct 30, 2018

I think we can extend our config we're passing via --config=python:cariumlib.djangolib.gunicorn_config
It's funny, I tried hacking type=json.loads in, but couldn't get it to work.

This comment has been minimized.

@javabrett

javabrett Oct 30, 2018

Contributor

Maybe you could convert your PR comment into a new issue along the lines of advertised CLI option --log-config-dict cannot work.

This comment has been minimized.

@rthille
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.