-
Notifications
You must be signed in to change notification settings - Fork 671
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
Conversation
20586a0
to
7a93a7e
Compare
There was a problem hiding this 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
_DEFAULT_FILES = [ | ||
"/etc/python-gitlab.cfg", | ||
os.path.expanduser("~/.python-gitlab.cfg"), | ||
] + _env_config() |
There was a problem hiding this comment.
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:
_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"), | |
] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was quick :) Thanks!
There was a problem hiding this comment.
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.
7a93a7e
to
401e702
Compare
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.