Allow build --env option to be comma separated list.

[kramarz]

Summary

According to help
(https://buildpacks.io/docs/tools/pack/cli/pack_build/) it should
already be this way.

Before

only --env KEY1=VAL1 --env KEY2=VAL2 syntax is allowed

After

also --env KEY1=VAL1,KEY2=VAL2 syntax is allowed

Documentation

• Should this change be documented?
  • Yes, see #___
  • [ x] No

[dfreilich]

Thanks for the fix!

[kramarz]

So acceptance test have detected some inconsistency. Currently syntax like this one works:
--env "ENV1_CONTENTS=\"Env1 Layer Contents From Command Line\"" and after the change it will no longer work.

On the other hand it really differs only if user pass quotation marks to the command (escaped them in shell) so they would get stripped by flags library - they could just omit quotes in that case.

--env "ENV1_CONTENTS=Env1 Layer Contents From Command Line" and
--env "\"ENV1_CONTENTS=Env1 Layer Contents From Command Line\"" would still work.

I have changed acceptance test to avoid that first syntax.

[jromero]

I'm a little concerned by this new change. I can't see this as expected behaviour from a user's perspective. What's the purpose of quoting the key and value?

I would have expected the following to work.

--env 'KEY_1=VAL1,KEY_2="VAL 2"'

key	value
KEY_1	VAL1
KEY_2	VAL 2

[kramarz]

Ok, I have tried implementing that, but this is way more complicated then I thought it will be - library does not support it so we have to parse it on our own.

[jromero]

@kramarz Unfortunately this implementation is clashing with being able to provide Windows paths in environment variable hence why tests are failing.

Here are the relevant logs from the test outputs:

[kramarz]

Right, we don't want to break such use cases. What about different approach to escaping?

Original solution with using StringSliceVarP was using CSV syntax for escaping commas and quotation marks, but you have suggested to not use include keys in quotes. I have now changed that so we would keep CSV escaping rules, but only for the value.

--env 'KEY_1=VAL1,KEY_2="Super, ""luxurious"" truck"'

key	value
KEY_1	VAL1
KEY_2	Super, "luxurious" truck

Please let me know if you have any suggestions for this issue.

[dfreilich]

I'm a bit confused by this - why is it matching

`VALUE"` and not `VALUE""`

?

[kramarz]

"" escapes " - it is the same as in CSV specification. The same escaping is used for other flags: buildpack and tag since they use StringSliceVarP. I just wanted to be consistent with what is already being used.

Sorry for delay - I was on holiday. I see there are some errors about missing tests - I ll try to add them this week.

[jromero]

@kramarz, seems like we have some use cases that were broken by this change. We had to revert it. The use cases brought up the fact that this may be harder to resolve than originally expected. I believe that the right path forward would be not to support (and therefore not mention) env vars to be listed together in a single flag.

See #1288 and #1289