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 dash in environment variable names #6080

Merged
merged 7 commits into from
Sep 11, 2019
Merged

Allow dash in environment variable names #6080

merged 7 commits into from
Sep 11, 2019

Conversation

lchayoun
Copy link
Contributor

@lchayoun lchayoun commented Aug 6, 2019

Fixes #6079

@hashicorp-cla
Copy link

hashicorp-cla commented Aug 6, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@langmartin langmartin left a comment

Choose a reason for hiding this comment

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

Thanks so much for the pr :) I think we may need to split apart the sanitizing code path a bit in order to allow hyphenated variables without changing existing behavior.

client/taskenv/env_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@langmartin langmartin left a comment

Choose a reason for hiding this comment

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

Thanks so much for taking another pass at this! I'm going to get one more set of eyes on this to make sure the NOMAD_ namespace for environment variables is sufficiently baked in that it covers all the generate cases.

helper/funcs.go Outdated Show resolved Hide resolved
@langmartin langmartin requested a review from notnoop August 12, 2019 20:44
@lchayoun
Copy link
Contributor Author

@langmartin after thinking about it a bit more, I think only generated environment variables should be cleaned.
I think use specified ones should be provided as specified.
Any issues with it?

Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this. Fully agree that user set environment variables as set verbatim with no processing. Had a question about NOMAD_ prefix check heuristic but lgtm otherwise.

client/taskenv/env.go Outdated Show resolved Hide resolved
client/taskenv/env.go Outdated Show resolved Hide resolved
@lchayoun
Copy link
Contributor Author

@notnoop I've added a specific list of the prefixes that needs to be cleaned, my only concern is for newly added generated environment variables prefixes that will need to be added to this list.
any ideas?

@lchayoun
Copy link
Contributor Author

@notnoop any update on this PR?

@notnoop notnoop merged commit 8177d45 into hashicorp:master Sep 11, 2019
@notnoop
Copy link
Contributor

notnoop commented Sep 11, 2019

Thanks @lchayoun , sorry for taking too long to merge.

I'm less concerned about newly generated environment variables, considering that they don't have backward compatibility constraints like current ones. I'd follow up and add a comment for future contributors which I suspect is sufficient now.

@lchayoun lchayoun deleted the bug-6079 branch November 3, 2019 14:44
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow dash in environment variable names
4 participants