Skip to content

feat: Allow an environment variable to specify config location #1074

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

Merged
merged 1 commit into from
Apr 16, 2020

Conversation

jeremycline
Copy link
Contributor

It can be useful (especially in scripts) to specify a configuration
location via an environment variable. If the "PYTHON_GITLAB_CFG"
environment variable is defined, treat its value as the path to a
configuration file and include it in the set of default configuration
locations.

@jeremycline jeremycline force-pushed the environment-variable branch from 20586a0 to 7a93a7e Compare April 15, 2020 13:21
@nejch nejch self-assigned this Apr 15, 2020
Copy link
Member

@nejch nejch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks a lot for this addition! Since this will work a lot like Ansible (which I assume you're familiar with :) ), I just added a comment about configuration precedence. I think users are more familiar with the "CLI options > env var file path > user config file > system config file" order.

Would you mind adding a small note in the docs to include this as an option?

PS note to self for future reference: at some point we might want to separate this out from the default files to get clearer messages for the user, currently we'll get this:

(.venv) [user@localhost python-gitlab]$ PYTHON_GITLAB_CFG="nonexistent.cfg" gitlab project list
Config file not found. 
Please create one in one of the following locations: /etc/python-gitlab.cfg, /home/user/.python-gitlab.cfg, nonexistent.cfg 
or specify a config file using the '-c' parameter.

gitlab/config.py Outdated
Comment on lines 28 to 31
_DEFAULT_FILES = [
"/etc/python-gitlab.cfg",
os.path.expanduser("~/.python-gitlab.cfg"),
] + _env_config()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we append the path from the env var but there are user or system-wide configuration files already, this won't be picked up. It might make sense for the environment variable to have precedence over default paths. I know the current order of default files goes against this (system-wide to user), which is maybe why you chose this, but how about this:

Suggested change
_DEFAULT_FILES = [
"/etc/python-gitlab.cfg",
os.path.expanduser("~/.python-gitlab.cfg"),
] + _env_config()
_DEFAULT_FILES = _env_config() + [
"/etc/python-gitlab.cfg",
os.path.expanduser("~/.python-gitlab.cfg"),
]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated! I know as a user that's the ordering I'd expect so I'm happy to get it closer to that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was quick :) Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review and accepting the patch!

It can be useful (especially in scripts) to specify a configuration
location via an environment variable. If the "PYTHON_GITLAB_CFG"
environment variable is defined, treat its value as the path to a
configuration file and include it in the set of default configuration
locations.
@jeremycline jeremycline force-pushed the environment-variable branch from 7a93a7e to 401e702 Compare April 16, 2020 23:01
@nejch nejch merged commit 0c3b717 into python-gitlab:master Apr 16, 2020
@jeremycline jeremycline deleted the environment-variable branch April 17, 2020 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants