-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 support for Env in remote shell provisioners #11819
Conversation
As mentioned in issue #11670, the Shell provider did not support Env variable declarations in the HCL templates. This commit adds the capability to the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me Lucas, nice work!
Can you resolve the linting failure before merging?
e2fddc0
to
9ddf8a8
Compare
@@ -439,6 +439,11 @@ func (p *Provisioner) createFlattenedEnvVars(elevated bool) (flattened string) { | |||
envVars[keyValue[0]] = escapedEnvVarValue | |||
} | |||
|
|||
for k, v := range p.config.Env { | |||
// No need to interpolate.Render as the Env map is only available in HCL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting find. I believe this is a because we made the decision that new template changes will be optimized for HCL. Env
is actually available for both HCL and JSON templates. See an example below. In HCL we don't encourage the use of Go template interpolation. So interpolate is not really a thing. But for JSON some folks may get confused. I think it is okay to let it be as is 🤔
{
"variables": {
"sensitive": "I am soooo sensitive"
},
"sensitive-variables": ["sensitive"],
"builders": [
{
"type": "null",
"name": "consul",
"communicator": "none"
}
],
"provisioners": [
{
"type": "shell-local",
"env": {
"TESTNAME": "\"hope\"life"
},
"env_var_format": "%s='foo%s' ",
"inline": ["echo the {{build_name}} and {{build_type}} for $TESTNAME, sensitive var: {{user `sensitive`}}"]
}
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh OK my bad! I assumed that this wouldn't be supported in the legacy JSON templates as they're deprecated and marked as not receiving updates.
If that's not the case, that comment can be removed, and we should definitely interpolate the strings in case that's needed.
Though, and that may be me missing something, I could only see the keys/values interpolated for the Powershell provisioner, and not the shell and windows-shell ones.
Although I'd imagine that'll be fairly rare, I imagine someone may add those, I'd think we should probably fix this in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no need for apologies. Your assumption is correct, and I think it might be confusing to users as they don't have the code insight we have. That's why its an interesting find.
I agree lets hold for a separate PR. It gives us time to play around and squash and confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so we have a trace for this, shell
and windows-shell
both interpolate the commands, including the environment definitions, before they upload the resulting script, hence why they don't need to interpolate the values directly in the createFlattenedEnvVars
function, as they will be rendered before they are executed in the Provision
method.
The powershell
provider however does not before it uploads the final script, hence why they're interpolated directly during the flatten phase.
I imagine the rationale is because the approach is different as the variables for Powershell scripts are first uploaded as a script, and then sourced from subsequent scripts instead of being integrated as commands in the prelude.
Note that the rendering could be done in uploadEnvVars
from what I see, maybe there's a good reason that it's not done here, we could discuss that later.
9ddf8a8
to
2126a4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. Appreciate you adding the additional context on the interpolation flow.
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Add the support for defining environment variables through the
Env
map for both: windows-shell, powershell and shell.Closes #11670