Skip to content

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

Merged
nejch merged 1 commit into
python-gitlab:masterfrom
jeremycline:environment-variable
Apr 16, 2020
Merged

feat: Allow an environment variable to specify config location#1074
nejch merged 1 commit into
python-gitlab:masterfrom
jeremycline:environment-variable

Conversation

@jeremycline
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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