Skip to content

[IMP] Expand env vars in config#8

Merged
lmignon merged 3 commits into
acsone:masterfrom
LasLabs:feature/yml-env-expansion
Feb 1, 2017
Merged

[IMP] Expand env vars in config#8
lmignon merged 3 commits into
acsone:masterfrom
LasLabs:feature/yml-env-expansion

Conversation

@lasley

@lasley lasley commented Jan 21, 2017

Copy link
Copy Markdown
Contributor
  • Add optional environment variable expansion for the config file

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

coveralls commented Jan 21, 2017

Copy link
Copy Markdown

Coverage Status

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

@yajo yajo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Simply great!

@yajo yajo left a comment

Copy link
Copy Markdown
Contributor

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

lmignon commented Jan 23, 2017

Copy link
Copy Markdown
Member

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

@lasley

lasley commented Jan 23, 2017

Copy link
Copy Markdown
Contributor Author

Oops I knew I was forgetting something! Documentation updated

@coveralls

Copy link
Copy Markdown

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

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

Coverage Status

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

@lasley

lasley commented Jan 23, 2017

Copy link
Copy Markdown
Contributor Author

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

@lmignon

lmignon commented Jan 24, 2017

Copy link
Copy Markdown
Member

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

@sbidoul

sbidoul commented Jan 24, 2017

Copy link
Copy Markdown
Member

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

@lasley

lasley commented Jan 24, 2017

Copy link
Copy Markdown
Contributor Author

@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

sbidoul commented Jan 24, 2017

Copy link
Copy Markdown
Member

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

Comment thread git_aggregator/main.py
help='Set the logging output level. {0}'.format(_LOG_LEVEL_STRINGS))

main_parser.add_argument(
'-e', '--expand-env',

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.

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

this is an incorrect use of parse arguments.

Copy link
Copy Markdown

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

coveralls commented Jan 31, 2017

Copy link
Copy Markdown

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