fix: Update multienv_step_runner Env Var Parsing Logic (#2351) #2354
Conversation
* Update multienv_step_runner.go to split only on the first = character * Add affirmative and failing test cases
| @@ -22,7 +22,7 @@ func (r *MultiEnvStepRunner) Run(ctx command.ProjectContext, command string, pat | |||
| var sb strings.Builder | |||
| sb.WriteString("Dynamic environment variables added:\n") | |||
| for _, item := range envVars { | |||
| nameValue := strings.Split(item, "=") | |||
| nameValue := strings.SplitN(item, "=", 2) | |||
| if len(nameValue) == 2 { | |||
| envs[nameValue[0]] = nameValue[1] | |||
| sb.WriteString(nameValue[0]) | |||
There was a problem hiding this comment.
I'm also finding that this list of dynamic env vars added is quite noisy, and doesn't really add much value to plans/applies. Would there be any openness to suppressing this?
There was a problem hiding this comment.
@tapaszto is the original developer, maybe he can chime in why this is here?
There was a problem hiding this comment.
We use multienv step to add dynamic number of environment variables coming from the result of a webservice call.
Originally we proposed to have a json string which can be parsed safely and handling the errors coming from the runstep result but later the reviewer suggested to have a simple key/value list. We modified accordingly implementing the simplest solution for our requirements and did not assume having "=" in the envvar values.
Here is the PR containing more details of our use case:
#1793
There was a problem hiding this comment.
Thanks for the feedback @tapaszto.
@jamengual As it stands, is this a reasonable bug fix to incorporate into the mainline? I think it's probably a pretty common case for env vars to contain =, as in my experience they're often base64 encoded.
There was a problem hiding this comment.
if @tapaszto agrees that this is ok to add and does not change the use case I think it will be ok.
There was a problem hiding this comment.
and maybe we should update the docs if they do not explain well the use case and or the key=value structure.
There was a problem hiding this comment.
if @tapaszto agrees that this is ok to add and does not change the use case I think it will be ok.
It's ok
There was a problem hiding this comment.
@austinsherron check the docs and see if they will need adjustment due to this change, please.
We can merge it after
|
@austinsherron did you check the docs? do you think we need to update them? |
jamengual
added
waiting-on-response
|
@austinsherron are you still around? |
) (runatlantis#2354) * Update multienv_step_runner Env Var Parsing Logic * Update multienv_step_runner.go to split only on the first = character * Add affirmative and failing test cases * Run gofmt
Fixes #2351.