Skip to content
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

Allow environment variable substitution in conf file #1343

Closed
wants to merge 4 commits into from
Closed

Allow environment variable substitution in conf file #1343

wants to merge 4 commits into from

Conversation

delitescere
Copy link

  • Enabled with the --enable-env command line option
  • Opt-in ensures current conf files don't get into ConfigParser
    recursion issues, e.g. home: %(HOME)s
  • Provide nicer error on exit if the conf file has bad interpolation
  • New unit tests with and without command line option present

- Enabled with the `--enable-env` command line option
- Opt-in ensures current conf files don't get into ConfigParser
  recursion issues, e.g. `home: %(HOME)s`
- Provide nicer error on exit if the conf file has bad interpolation
- New unit tests with and without command line option present
@delitescere
Copy link
Author

The build failure is because it is claimed in the README that python 2.7 is ok to use, but CI is building with python 2.6 which doesn't support the assertRaises as a context manager (see http://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises).

@delitescere
Copy link
Author

Wow this is one flakey CI. It can't make up it's mind if it's running python 2.6 or 2.7, and randomly fails anyway.

Suffice it to say, this patch is clean.

Cheers,
Josh.

@LeoCavaille
Copy link
Member

Thanks @delitescere
We will review this change for the 5.3 release! (5.2 is now frozen for imminent release)
Regarding the CI, it's running BOTH python 2.6 and 2.7 ! Because non-EOL'd OS still run python 2.6, and anyone should be able to install from source the agent..

We have made huge steps towards non-flakiness, but I cannot agree more it sucks. Relying on so many 3rd party mirrors and services for integration testing is hard, next up will be some more caching to avoid that.

@LeoCavaille LeoCavaille added this to the 5.3.0 milestone Feb 3, 2015
@LeoCavaille
Copy link
Member

Cross reference #1243

@vincenzovitale
Copy link

+1

@LeoCavaille
Copy link
Member

This is a really nice feature, we'll work on it soon for the future release we didn't have time to really work on it yet. And we want to add some more work on top so that we revamp the config.py code and the check config code in the agent to allow using the environment for more things. Setting milestone to 5.4.

@LeoCavaille
Copy link
Member

Still some more work to do, working a leo/configcleanup branch. Will submit new PR for 5.5.

@LeoCavaille LeoCavaille modified the milestones: 5.5.0, 5.4.0 Jun 9, 2015
@mattes
Copy link

mattes commented Jun 26, 2015

+1

@LeoCavaille LeoCavaille modified the milestones: 6.0.0, 5.5.0 Jul 31, 2015
@irabinovitch
Copy link
Contributor

@LeoCavaille Should we close this in favor of the branch you are working on?

@remh remh modified the milestones: 5.7.0, 6.0.0 Nov 19, 2015
@remh remh added this to the 5.8.0 milestone Dec 22, 2015
@remh remh removed this from the 5.7.0 milestone Dec 22, 2015
@irabinovitch
Copy link
Contributor

@LeoCavaille Should we close this in favor of the branch you mentioned in #1343 (comment) ?

@remh remh modified the milestones: 6.0.0, 5.8.0 Apr 25, 2016
@irabinovitch
Copy link
Contributor

@LeoCavaille checking back in?

@LeoCavaille
Copy link
Member

@irabinovitch wow somehow I missed that thread 3 times in a row. I will coordinate with the rest of the team (cc @remh) to decide what to do next for this feature and re-update the PR this week.
I think this has been milestoned for 6.X as that code has been refactored quite a lot since that PR was opened. My former branch with updates to this initial patch cannot easily be merged either.

config = ConfigParser.ConfigParser()
if options is not None and options.enable_env:
log.info("Interpolation of environment variables in config is enabled")
config = ConfigParser.ConfigParser(os.environ)
Copy link

@jonhosmer jonhosmer Aug 19, 2016

Choose a reason for hiding this comment

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

@delitescere @LeoCavaille
You may want to add ConfigParser.ConfigParser.optionxform = str before config = ConfigParser.ConfigParser(os.environ) so that the keys in os.environ are treated as case sensitive.

For example:

>>> from ConfigParser import ConfigParser
>>> import os
>>> os.environ['XY'] = 'foo'
>>> os.environ['Xy'] = 'bar'
>>> os.environ['xY'] = 'baz'
>>> print '\n'.join(sorted([':'.join((k, v)) for k, v in os.environ.items() if k.lower() == 'xy']))
XY:foo
Xy:baz
xY:bar

>>> cfgp = ConfigParser(os.environ)
>>> print '\n'.join(sorted([':'.join((k, v)) for (k, v) in cfgp.defaults().items() if k.lower() == 'xy']))
xy:foo

>>> ConfigParser.optionxform = str
>>> cfgp_cs = ConfigParser(os.environ)
>>> print '\n'.join(sorted([':'.join((k, v)) for (k, v) in cfgp_cs.defaults().items() if k.lower() == 'xy']))
XY:foo
Xy:baz
xY:bar

>>> assert([(k, v) for (k, v) in cfgp_cs.defaults().items() if k.lower() == 'xy'] == [(k, v) for (k, v) in os.environ.items() if k.lower() == 'xy'])
>>> assert([(k, v) for (k, v) in cfgp.defaults().items() if k.lower() == 'xy'] == [(k, v) for (k, v) in os.environ.items() if k.lower() == 'xy'])
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-43-1f97baeb1e97> in <module>()
----> 1 assert([(k, v) for (k, v) in cfgp.defaults().items() if k.lower() == 'xy'] == [(k, v) for (k, v) in os.environ.items() if k.lower() == 'xy'])

AssertionError:

@delitescere
Copy link
Author

/unsubscribe

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

Successfully merging this pull request may close these issues.

7 participants