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

Add environment variable replacement to GetStringValueOrDefault #57

Merged
merged 5 commits into from
Mar 7, 2019
Merged

Add environment variable replacement to GetStringValueOrDefault #57

merged 5 commits into from
Mar 7, 2019

Conversation

abbotware
Copy link
Contributor

@abbotware abbotware commented Jan 20, 2019

Description of changes:
Allows a config file to use $(VARIABLE) that will be replaced with environment variables at run-time.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@abbotware
Copy link
Contributor Author

I only use config files.. but having to put 'relative pathing' (....\ notation) everywhere is painful. I would much rather just use a token that gets expanded at runtime

@abbotware
Copy link
Contributor Author

I can confirm this works perfectly for me.. as long as the {varaible} token is not typically used in any existing parameter/config this should be fine for everyone else too. If somehow a token appeared, it would get replaced with an empty string as the environment variable won't exist

Copy link
Member

@normj normj left a comment

Choose a reason for hiding this comment

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

I like the feature. I am worried that opening and closing brackets is not unique enough to be sure the intention was an environment variable. I'm wondering if we should follow MSBuild precedence and use $(VARIABLE) instead.

https://docs.microsoft.com/en-us/visualstudio/msbuild/how-to-use-environment-variables-in-a-build?view=vs-2017

Also can you add some unit tests for this feature? You should be able to write tests with by setting environment variable in the test, create a command and then call GetStringValueOrDefault().

@abbotware
Copy link
Contributor Author

what the token is - doesnt matter - its a regex. I just need any form of token replacement so I don't have to make convoluted config files. I'll change to $(VARIABLE) and make some unit tests

@normj normj merged commit f994046 into aws:master Mar 7, 2019
@normj
Copy link
Member

normj commented Mar 7, 2019

This has been released as part of version 3.1.0 of Amazon.ElasticBeanstalk.Tools. Thanks for the PR and patience to get this out.

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.

None yet

2 participants