-
Notifications
You must be signed in to change notification settings - Fork 261
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
Supports setting comma separated env vars #347
Conversation
`--env ENV1=VAL1 --env ENV2=VAL2,ENV3=VAL3` should work specifying by separate `--env` or specifying comma separated values to `--env` or combination of both.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: navidshaikh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
@navidshaikh: 0 warnings.
In response to this:
/lint
Proposed Changes
--env ENV1=VAL1 --env ENV2=VAL2,ENV3=VAL3
should work specifying by separate
--env
or specifying comma separated
values to--env
or combination of both.Release Note
Supports specifying comma separated environment variables as `--env KEY1=VAL1,KEY2=VAL2`
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@navidshaikh don't forget to add to CHANGELOG.adoc, too |
@rhuss : I thought about that, separating the values by comma for Without comma
With comma
I think this should just work fine without any additional processing. |
No strong opinion either way, but do people have a guess as to how often someone may use a comma in an env var? Is there a way to escape it via cobra? |
@navidshaikh: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Supporting this would disallow users from setting commas in env var values, need to check about escaping but single/double quotes on only values or even whole pair, back/forward slash didnt work.
The current forma though support it (check last service update command above). |
I did some digging and found this: And this does seem to work:
So if we document this then I think we're good |
We should add a test for it too :-) |
@duglin : Nice! I can do this as well
|
|
||
servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") | ||
|
||
action, updated, _, err := fakeServiceUpdate(orig, []string{ |
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.
Add test for quoting (to have a comma in an env var), and for repeating the flag, to make sure we cover all the bases.
LGTM |
repeat --env flag to specify multiple env vars, escaping might create more confusion. |
images job might run on OCP 3.11 (api.ci) and we'd need to create dir before referncing it via WORKDIR. (issue with imagebuilder on OCP 3.11)
/lint
Proposed Changes
--env ENV1=VAL1 --env ENV2=VAL2,ENV3=VAL3
should work specifying by separate
--env
or specifying comma separatedvalues to
--env
or combination of both.Release Note