You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Support overriding config options via environment variables #48
I first wrote this as a comment in #35, but I'm creating a separate issue for this so it doesn't get forgotten. I will volunteer to implement it. This is slightly higher priority now that I'm getting somewhere with PyFITS implementation, and I would like to hook it into the eventual config system.
It would be good to have the ability to override any config options via environment variables. These environment variables would follow some standard naming convention like ASTROPY_SUB_PACKAGE_OPTION. There could also be some way to register shorter aliases for commonly used options, but I don't have any thoughts yet on the specifics of that.
In order to support these environment variables transparently, the easiest approach I see (which I usually end up doing anyways) is to provide a simple wrapper around ConfigObj objects. I like to use something simple like Configuration. This is a much simpler object than the ConfigObj so that fewer details of reading/writing the configuration are exposed to subpackage/affiliated package authors. It would still provide a dictionary-like interface and many of the methods of ConfigObj. Though rather than write() it has a save() method which I think is a little more obvious. If a user wants to update the configuration they simply call config.save(). This method would prevent saving any values that were overridden by environment variables.
Using a wrapper can have other eventual benefits, since it allows us to provide some abstractions around the configuration object that aren't specific to any one backend implementation (not that I would suggest we move away from ConfigObj or anything--it's just nice to have additional flexibility).
I've started poking at this a bit in iguananaut/astropy/config-envvars, but hit a small snag. From my last commit message:
When a config is written to a file I don't want it to save the value that was set by an environment variable. This could be implemented through the save_config function. But if a user calls the .write() method directly on the ConfigObj object it won't work correctly. I could be fine with that as long as we make sure the user always uses our API...
Another possibility is that there's a recent patch for ConfigObj that makes it possible to subclass the Section class throughout ConfigObj. This would allow us to use a simple Section subclass to implement this functionality consistently.
I don't fully understand the problem... why would the user be calling ConfObj.write()? As the ConfigurationItem class is written, they should always either call ConfigurationItem.save() on a particular item or astropy.config.configs.save_config(). The idea I have here is to make it so the user doesn't even know ConfigObj is actually even present - all they know about is the astropy.config.configs stuff. So you could add a method to the ConfigurationItem that checks if the current item setting came from the environment variable or not, and if it does, temporarily put back in whatever it's "supposed" to be when saving. Not terribly elegant, but that was part of the reason I thought we wanted an interface where we control all the entry points...
@eteq I already have ConfigurationItem doing as you suggested, and that works. So you're right, as long as they only use the API we give them I guess there's no problem.
The only place that there is a problem is that get_config() still returns a ConfigObj object. That's where I was running into issues... I don't really mind that it does, though I suppose if we really wanted we could convert it to a normal dict--that would control things even more.
Ahh, now I see what you're saying. I think it's good to have access to the actual ConfgObj just in case it's needed. But given what you've said here, we should probably add the following to the get_config docstring:
.. warning::
Calling :meth:`ConfigObj.write` directly may incorrectly save your configuration files. Instead, always use `ConfigurationItem.save` or `save_config` to save configuration settings.
(note that I'm doing the :meth: thing in ConfigObj becaue configobj doesn't seem to do intersphinx, so sphinx won't know it's a function and do the () thing without explicit marking)
Ok, cool. Once you've gotten this updated, maybe convert this issue to a pull request (if you don't know how to do this, see #98 (comment) or the comment above that one by @mdboom) so we can review and merge that way?
I've thought the same thing - I mean, it's not really that hard to do it once you dig into the API, but it would be so simple to just add a button... I imagine they'll add it sooner or later.
@mdboom - I was re-visiting some random old issues... do you think this still makes sense in the context of the new config system? Or should we just close this?
Activity
embray commentedon Dec 7, 2011
I've started poking at this a bit in iguananaut/astropy/config-envvars, but hit a small snag. From my last commit message:
When a config is written to a file I don't want it to save the value that was set by an environment variable. This could be implemented through the save_config function. But if a user calls the .write() method directly on the ConfigObj object it won't work correctly. I could be fine with that as long as we make sure the user always uses our API...
Another possibility is that there's a recent patch for ConfigObj that makes it possible to subclass the Section class throughout ConfigObj. This would allow us to use a simple Section subclass to implement this functionality consistently.
eteq commentedon Dec 8, 2011
I don't fully understand the problem... why would the user be calling
ConfObj.write()? As theConfigurationItemclass is written, they should always either callConfigurationItem.save()on a particular item orastropy.config.configs.save_config(). The idea I have here is to make it so the user doesn't even know ConfigObj is actually even present - all they know about is theastropy.config.configsstuff. So you could add a method to theConfigurationItemthat checks if the current item setting came from the environment variable or not, and if it does, temporarily put back in whatever it's "supposed" to be when saving. Not terribly elegant, but that was part of the reason I thought we wanted an interface where we control all the entry points...embray commentedon Dec 8, 2011
@eteq I already have ConfigurationItem doing as you suggested, and that works. So you're right, as long as they only use the API we give them I guess there's no problem.
The only place that there is a problem is that get_config() still returns a ConfigObj object. That's where I was running into issues... I don't really mind that it does, though I suppose if we really wanted we could convert it to a normal dict--that would control things even more.
eteq commentedon Dec 8, 2011
Ahh, now I see what you're saying. I think it's good to have access to the actual
ConfgObjjust in case it's needed. But given what you've said here, we should probably add the following to the get_config docstring:(note that I'm doing the
:meth:thing inConfigObjbecaue configobj doesn't seem to do intersphinx, so sphinx won't know it's a function and do the () thing without explicit marking)embray commentedon Dec 8, 2011
Sounds fine. Also, I usually use :meth:, :func:, and friends even when they can be otherwise be inferred, just in case...
eteq commentedon Dec 8, 2011
Ok, cool. Once you've gotten this updated, maybe convert this issue to a pull request (if you don't know how to do this, see #98 (comment) or the comment above that one by @mdboom) so we can review and merge that way?
embray commentedon Dec 8, 2011
Cool, so there is a way! I don't know why they don't just add this to the UI already--it seems like a commonly requested feature.
eteq commentedon Dec 9, 2011
I've thought the same thing - I mean, it's not really that hard to do it once you dig into the API, but it would be so simple to just add a button... I imagine they'll add it sooner or later.
Here's an initial implementation for astropy#48. However, it's not qu…
Implement use of the astropy config system for PyFITS' handful of set…
astrofrog commentedon May 3, 2014
Re-scheduling to 1.0 since we should wait until the new APE3 changes are settled.
astrofrog commentedon Jan 14, 2015
Removing the 1.0 milestone since this is low priority and there has been no work on it recently.
Merge pull request astropy#48 from mdboom/edit_on_github_fix
eteq commentedon May 8, 2015
@mdboom - I was re-visiting some random old issues... do you think this still makes sense in the context of the new config system? Or should we just close this?
pllim commentedon May 11, 2017
I say we just close this.