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

Fix #1576 - config file/module in GUNICORN_CMD_ARGS #1581

Merged
merged 1 commit into from Sep 16, 2017

Conversation

Projects
None yet
3 participants
@Code0x58
Contributor

Code0x58 commented Sep 2, 2017

I also sorted THANKS.md to follow CONTRIBUTING.md.

@tilgovi

Looks mostly good. Thank you for writing tests, too! My only question is whether we should override env_args.config with args.config so that we're consistent about precedence. But maybe config files are special and you could specify both? It seems overkill maybe, since you could always import one config from another.

self.load_config_from_file(args.config)
env_args = parser.parse_args(self.cfg.get_cmd_args_from_env())
if args.config or env_args.config:

This comment has been minimized.

@tilgovi

tilgovi Sep 2, 2017

Collaborator

Should one override the other rather than allowing both of these branches?

This comment has been minimized.

@berkerpeksag

berkerpeksag Sep 3, 2017

Collaborator

I agree that env_args.config should override args.config. We should probably document this at http://docs.gunicorn.org/en/latest/settings.html#settings

@Code0x58

This comment has been minimized.

Contributor

Code0x58 commented Sep 2, 2017

I can kind of understand why overriding would make sense, but inheriting feels more natural to me.

After looking at this more I've realised the hierarchy feels a bit weird (as it was before the patch) as options from the config file on the command line are overridden by arguments from the environment:
app config ← ((env config file ← arg config file) | default config file) ← env options ← arg options

This also gives the issue that applications don't get augments from the environment, so for applications to get configured properly Application.init has to be after all of the config loading.

Blerg.

@berkerpeksag

This comment has been minimized.

Collaborator

berkerpeksag commented Sep 3, 2017

Some comments:

  • Please leave changes to THANKS file for another PR. You can add your name in this PR.
  • There is no need to rename tests/config/test_cfg.py to tests/config/test_cfg1.py.
@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Sep 16, 2017

I think this looks good to merge! Thank you so much, @Code0x58!

@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Sep 16, 2017

And thanks for your patience. I had my personal laptop melt down last weekend, so I've been a little bit offline more than usually.

@tilgovi tilgovi merged commit 886b5e9 into benoitc:master Sep 16, 2017

5 of 8 checks passed

couverture-io/py27 Not all changes are covered
Details
couverture-io/py35 Not all changes are covered
Details
couverture-io/py36 Not all changes are covered
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
couverture-io/py26 Code coverage OK
Details
couverture-io/py34 Code coverage OK
Details
couverture-io/py36-dev Code coverage OK
Details
couverture-io/py37 Code coverage OK
Details
@Code0x58

This comment has been minimized.

Contributor

Code0x58 commented Sep 16, 2017

Don't worry, I was delayed too as I was on a mobile for 5 days.

Is it worth me putting in a separate PR to either sort the THANKS, or remove the sorting part from CONTRIBUTING?

@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Sep 16, 2017

I'm fine to keep it sorted.

@Code0x58 Code0x58 deleted the Code0x58:fix/1576-env-config-file branch Sep 16, 2017

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

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