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 windows service to set arbitrary environment variables #6648

Merged
merged 1 commit into from Jun 17, 2019

Conversation

@mwrock
Copy link
Contributor

commented Jun 12, 2019

fixes #6647

Now users can create any application setting prefixed with ENV_ and that setting (sans the ENV_) will be added to the environment of the service.

This also deprecates the debug setting which we will remove in a future release.

Signed-off-by: mwrock matt@mattwrock.com

allow windows service to set arbitrary environment variables
Signed-off-by: mwrock <matt@mattwrock.com>
@chef-expeditor

This comment has been minimized.

Copy link

commented Jun 12, 2019

Hello mwrock! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

{
var envPrefix = "ENV_";
foreach(var key in ConfigurationManager.AppSettings.AllKeys) {
var i = key.Trim().ToUpper();

This comment has been minimized.

Copy link
@markan

markan Jun 13, 2019

Contributor

Curious about when whitespace can appear (and Trim is needed). Is there always whitespace, or is that only when someone is doing something wrong? The effect here is to fold "ENV_WIERD " (with space) and "ENV_WIERD" (without space) together; which takes effect depends on the order from AllKeys.

There's a similar effect from the ToUpper; if both "env_case" and "ENV_CASE" are present only one will passed.

Would it make sense to be more restrictive, and only pass 'well formed' ENV_* variables (no whitespace, all upper, etc.)

It's been a while since I've done C#/windows, so very well could be missing something.

This comment has been minimized.

Copy link
@mwrock

mwrock Jun 13, 2019

Author Contributor

The trim is really defensive handling of a config file content like:

<add key="   ENV_HAB_FEAT_IGNORE_SIGNALS   " value="true" />

If some one had multiple entries for the same key. For example one with spaces and the other without, it seems like it would be unintentional. However even if we omitted the trim, windows would effectively trim the key when we added it to the environment.

Same holds true for the ToUpper. On Windows, environment var keys are case insensitive. So it would likely be unintentional to add 2 keys that only vary in casing.

The main benefit of trimmimg and upper casing the key is it just makes it simpler to do the StartsWith("ENV_") check.

This comment has been minimized.

Copy link
@markan

markan Jun 13, 2019

Contributor

Thanks, makes sense to me.

@markan

markan approved these changes Jun 13, 2019

@chefsalim

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Will this change require updates to the Windows build worker setup?

@mwrock

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

no this should not require any builder changes @chefsalim

@mwrock mwrock merged commit ce3c363 into master Jun 17, 2019

5 checks passed

DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #2294 passed (28 minutes, 56 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #2606 passed (35 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

@mwrock mwrock deleted the win_svc_env branch Jun 17, 2019

chef-ci added a commit that referenced this pull request Jun 17, 2019

Update CHANGELOG.md with details from pull request #6648
Obvious fix; these changes are the result of automation not creative thinking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.