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

[IMP] Expand env vars in config #8

Merged
merged 3 commits into from
Feb 1, 2017

Conversation

lasley
Copy link
Contributor

@lasley lasley commented Jan 21, 2017

  • Add optional environment variable expansion for the config file

* Add optional environment variable expansion for the config file
@coveralls
Copy link

coveralls commented Jan 21, 2017

Coverage Status

Coverage increased (+0.2%) to 89.199% when pulling 50467a3 on LasLabs:feature/yml-env-expansion into 115f7e9 on acsone:master.

Copy link
Contributor

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Simply great!

Copy link
Contributor

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Lacks proper docs in README.

@lmignon
Copy link
Member

lmignon commented Jan 23, 2017

@lasley Thank you for this improvement! As stated by @yajo an entry into the documentation for this new feature will be greatly appreciated.

@lasley
Copy link
Contributor Author

lasley commented Jan 23, 2017

Oops I knew I was forgetting something! Documentation updated

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 88.153% when pulling 30561dc on LasLabs:feature/yml-env-expansion into 115f7e9 on acsone:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 88.153% when pulling 30561dc on LasLabs:feature/yml-env-expansion into 115f7e9 on acsone:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 88.153% when pulling 30561dc on LasLabs:feature/yml-env-expansion into 115f7e9 on acsone:master.

@lasley
Copy link
Contributor Author

lasley commented Jan 23, 2017

Coverage drop is due to more lines being added in the ReadMe it seems. I submitted #9 for that.

@lmignon
Copy link
Member

lmignon commented Jan 24, 2017

@lasley IMO Variable expansion should be the default. A plus should be to keep a parameter to disable the expansion.

@sbidoul
Copy link
Member

sbidoul commented Jan 24, 2017

@lmignon @lasley are there security implications when enabling env var substitution?

@lasley
Copy link
Contributor Author

lasley commented Jan 24, 2017

@lmignon Making it the default sounds good to me - not being a core contributor to this repo, I was trying to make the change as silently as possible for the proposal.

@sbidoul Yeah there could be some security implications here, as with most things using environment vars. An example of a harmful act that could be performed with this is the using a Travis Secret env var in a test & dumping it to console.

This would require a merge to master though, because the Travis secures are not exposed to forks. I'm sure some other dangerous things could be thought of as well, but that's the main one I watch out for in PRs with systems that include env expansion.

As with most security holes, the tradeoff is usability. In my specific instance, these env vars enhance usability of this application quite a bit in the context of Docker.

@sbidoul
Copy link
Member

sbidoul commented Jan 24, 2017

I prefer to keep the explicit activation of the substitution mode.

@@ -89,6 +89,16 @@ def get_parser():
type=_log_level_string_to_int,
nargs='?',
help='Set the logging output level. {0}'.format(_LOG_LEVEL_STRINGS))

main_parser.add_argument(
'-e', '--expand-env',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per @pedrobaeza in Tecnativa@50467a3#commitcomment-20682952:

this is an incorrect use of parse arguments.

Choose a reason for hiding this comment

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

Correct use is:

    main_parser.add_argument(
        '-e', '--expand-env',
        dest='expand_env',
        action='store_true',
        help='Expand environment variables in configuration file',
)

@coveralls
Copy link

coveralls commented Jan 31, 2017

Coverage Status

Coverage increased (+0.8%) to 89.803% when pulling a4a8b5a on LasLabs:feature/yml-env-expansion into 115f7e9 on acsone:master.

@lmignon lmignon merged commit d13a104 into acsone:master Feb 1, 2017
@lasley lasley deleted the feature/yml-env-expansion branch February 1, 2017 17:12
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.

6 participants