added capability to take JSON as log's config #1917
Conversation
There was a problem hiding this comment.
Looks good to me! Thank you. Would you mind squashing the commits? @berkerpeksag @benoitc how does this look to you?
kingkuong
force-pushed
the
1775-support-log-config-json
branch
from
a0b87d8
to
e98f6a0
Compare
|
Thanks to take a look, I did squash the commits. |
gunicorn/config.py
Outdated
|
|
||
| Format: https://docs.python.org/3/library/logging.config.html#logging.config.dictConfig | ||
|
|
||
| .. versionadded:: 19.9 |
There was a problem hiding this comment.
I think "19.9" should be "20.0" now.
There was a problem hiding this comment.
is there a plan to upgrade to 20.0? The current is 19.9 and this could be going to either 19.10 or 20.0.
There was a problem hiding this comment.
The master branch doesn't support Python 2 anymore and since we can't drop Python 2 support in the next 19.x release, we can safely say that the master branch is now 20.0. If you want to update gunicorn.__version__, feel free to submit a separate PR :)
gunicorn/config.py
Outdated
| validator = validate_string | ||
| default = None | ||
| desc = """\ | ||
| The log config JSON reads config from a JSON file |
There was a problem hiding this comment.
Perhaps using something shorter like "The log config to read from a JSON file." would be better?
gunicorn/config.py
Outdated
| @@ -1368,15 +1368,31 @@ class LogConfigDict(Setting): | |||
| 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 | |||
| older file configuration format. | |||
| takes precedence over the :ref:`logconfig` and :ref:`logConfigJson` options, which uses the | |||
There was a problem hiding this comment.
Style nit: Please wrap lines at 80 chars.
gunicorn/config.py
Outdated
| @@ -627,11 +627,11 @@ class WorkerClass(Setting): | |||
| A string referring to one of the following bundled classes: | |||
|
|
|||
| * ``sync`` | |||
| * ``eventlet`` - Requires eventlet >= 0.9.7 (or install it via | |||
| * ``eventlet`` - Requires eventlet >= 0.9.7 (or install it via | |||
There was a problem hiding this comment.
I know that your editor did this automatically, but it would be nice not to include cosmetic fixes into this PR. Feel free to submit them as a separate PR.
gunicorn/glogging.py
Outdated
| config_json = json.load(open(cfg.logconfig_json)) | ||
| config.update(config_json) | ||
| dictConfig(config) | ||
| except( |
There was a problem hiding this comment.
Style nit: Add a space after except.
|
@tilgovi @berkerpeksag Any thoughts on when this can get merged and released? |
docs/source/settings.rst
Outdated
| @@ -1173,11 +1184,11 @@ libraries may be installed using setuptools' ``extra_require`` feature. | |||
| A string referring to one of the following bundled classes: | |||
|
|
|||
| * ``sync`` | |||
| * ``eventlet`` - Requires eventlet >= 0.9.7 (or install it via | |||
| * ``eventlet`` - Requires eventlet >= 0.9.7 (or install it via | |||
There was a problem hiding this comment.
You probably forgot to rerun make -C docs html :)
|
@cuongtranx I today ran into an issue where i was basically trying to use do you have any interest in continuing this PR? |
|
oh wow thanks for merging! This was a looong time ago and I didn't rmb putting this PR at all 😬 |
Added feature discussed #1775
What changed is that I added the option to take in a JSON file for log's config using the
log-config-jsonparam.