cli: drop support for passing comma-separated template parameters through --param/--value #11539

mmilata · 2016-10-24T20:20:51Z

TODO:

Extend to --env parameters
Rebase pflag in kubernetes (Update github.com/spf13/pflag to latest version kubernetes/kubernetes#35700)
Docs update (Do not use comma-separated template parameters openshift-docs#3107 + Do not use comma-separated environment variables openshift-docs#3123)
Public notification after merge - mailing lists, release notes

The bump commit was created manually because I've been unable to make godep restore work for me. The bump is needed in order to have https://godoc.org/github.com/spf13/pflag#StringArray.

liggitt · 2016-10-24T21:20:29Z

can we bump to the same level as 1.4 upstream (1560c1005499d61b80f865c04d39ca7505bf7f0b)? https://github.com/kubernetes/kubernetes/blob/release-1.4/Godeps/Godeps.json#L2050-L2053

liggitt · 2016-10-24T21:20:52Z

or if this is post-1.4, to whatever is currently in master upstream

liggitt · 2016-10-24T21:22:03Z

ah, nevermind... looks like 1.4 didn't have stringarray

mmilata · 2016-10-25T11:12:45Z

@liggitt kubernetes master has c7e63cf4530bcd3ba943729cee0efeff2ebea63f which has StringArray but is missing a bunch of small fixes

mmilata · 2016-10-25T11:35:34Z

@bparees @smarterclayton PTAL

Docs update: openshift/openshift-docs#3107.

Does [test] run the cmd tests?

mmilata · 2016-10-25T13:21:12Z

Does [test] run the cmd tests?

It does. [test] again

liggitt · 2016-10-25T13:23:58Z

Please open an upstream PR bumping to the same version then

bparees

@mmilata does this also resolve the issue of the $(cat foo) syntax only capturing the first line? I can't remember if that as just a pflag update or something else.

bparees · 2016-10-25T17:37:17Z

pkg/cmd/cli/cmd/newapp.go

@@ -166,7 +166,7 @@ func NewCmdNewApplication(name, baseName string, f *clientcmd.Factory, out, erro
 	cmd.Flags().StringSliceVar(&config.Templates, "template", config.Templates, "Name of a stored template to use in the app.")
 	cmd.Flags().StringSliceVarP(&config.TemplateFiles, "file", "f", config.TemplateFiles, "Path to a template file to use for the app.")
 	cmd.MarkFlagFilename("file", "yaml", "yml", "json")
-	cmd.Flags().StringSliceVarP(&config.TemplateParameters, "param", "p", config.TemplateParameters, "Specify a list of key value pairs (e.g., -p FOO=BAR,BAR=FOO) to set/override parameter values in the template.")
+	cmd.Flags().StringArrayVarP(&config.TemplateParameters, "param", "p", config.TemplateParameters, "Specify a key-value pair (e.g., -p FOO=BAR) to set/override parameter value in the template.")


"to set/override a parameter value"

bparees · 2016-10-25T17:37:44Z

pkg/cmd/cli/cmd/process.go

 			kcmdutil.CheckErr(err)
 		},
 	}
 	cmd.Flags().StringP("filename", "f", "", "Filename or URL to file to read a template")
 	cmd.MarkFlagFilename("filename", "yaml", "yml", "json")
-	cmd.Flags().StringSliceP("value", "v", nil, "Specify a list of key-value pairs (eg. -v FOO=BAR,BAR=FOO) to set/override parameter values")
+	cmd.Flags().StringArrayP("value", "v", nil, "Specify a key-value pair (eg. -v FOO=BAR) to set/override parameter value in the template.")


"to set/override a parameter value"

bparees · 2016-10-25T17:44:42Z

pkg/cmd/cli/cmd/newapp.go

@@ -166,7 +166,7 @@ func NewCmdNewApplication(name, baseName string, f *clientcmd.Factory, out, erro
 	cmd.Flags().StringSliceVar(&config.Templates, "template", config.Templates, "Name of a stored template to use in the app.")
 	cmd.Flags().StringSliceVarP(&config.TemplateFiles, "file", "f", config.TemplateFiles, "Path to a template file to use for the app.")
 	cmd.MarkFlagFilename("file", "yaml", "yml", "json")
-	cmd.Flags().StringSliceVarP(&config.TemplateParameters, "param", "p", config.TemplateParameters, "Specify a list of key value pairs (e.g., -p FOO=BAR,BAR=FOO) to set/override parameter values in the template.")
+	cmd.Flags().StringArrayVarP(&config.TemplateParameters, "param", "p", config.TemplateParameters, "Specify a key-value pair (e.g., -p FOO=BAR) to set/override parameter value in the template.")
 	cmd.Flags().StringSliceVar(&config.Groups, "group", config.Groups, "Indicate components that should be grouped together as <comp1>+<comp2>.")
 	cmd.Flags().StringSliceVarP(&config.Environment, "env", "e", config.Environment, "Specify key-value pairs of environment variables to set into each container. This doesn't apply to objects created from a template, use parameters instead.")


we should do the same thing for env (drop comma support, add a warning)

mmilata · 2016-10-27T11:39:38Z

@bparees updated to handle --env arguments as well. Multiline values shouldn't be a problem, StringArray should handle them fine, StringSlice does not. There's a [test] case for this in test/cmd/process.sh.

bparees · 2016-10-27T14:19:31Z

pkg/bootstrap/docker/up.go

@@ -139,7 +139,7 @@ func NewCmdUp(name, fullName string, f *osclientcmd.Factory, out io.Writer) *cob
 	cmd.Flags().StringVar(&config.HostDataDir, "host-data-dir", "", "Directory on Docker host for OpenShift data. If not specified, etcd data will not be persisted on the host.")
 	cmd.Flags().BoolVar(&config.PortForwarding, "forward-ports", config.PortForwarding, "Use Docker port-forwarding to communicate with origin container. Requires 'socat' locally.")
 	cmd.Flags().IntVar(&config.ServerLogLevel, "server-loglevel", 0, "Log level for OpenShift server")
-	cmd.Flags().StringSliceVarP(&config.Environment, "env", "e", config.Environment, "Specify key value pairs of environment variables to set on OpenShift container")
+	cmd.Flags().StringArrayVarP(&config.Environment, "env", "e", config.Environment, "Specify a key value pair of environment variable to set on OpenShift container")


"Specify a key value pair of environment variable"->"Specify a key value pair for an environment variable"

bparees · 2016-10-27T14:20:53Z

pkg/cmd/cli/cmd/newapp.go

 	cmd.Flags().StringSliceVar(&config.Groups, "group", config.Groups, "Indicate components that should be grouped together as <comp1>+<comp2>.")
-	cmd.Flags().StringSliceVarP(&config.Environment, "env", "e", config.Environment, "Specify key-value pairs of environment variables to set into each container. This doesn't apply to objects created from a template, use parameters instead.")
+	cmd.Flags().StringArrayVarP(&config.Environment, "env", "e", config.Environment, "Specify a key-value pair of environment variable to set into each container. This doesn't apply to objects created from a template, use parameters instead.")


Specify a key value pair of environment variable -> Specify a key value pair for an environment variable

bparees · 2016-10-27T14:21:08Z

pkg/cmd/cli/cmd/newbuild.go

@@ -128,7 +128,7 @@ func NewCmdNewBuild(name, baseName string, f *clientcmd.Factory, in io.Reader, o
 	cmd.Flags().StringVar(&config.Name, "name", "", "Set name to use for generated build artifacts.")
 	cmd.Flags().StringVar(&config.To, "to", "", "Push built images to this image stream tag (or Docker image repository if --to-docker is set).")
 	cmd.Flags().BoolVar(&config.OutputDocker, "to-docker", false, "Have the build output push to a Docker repository.")
-	cmd.Flags().StringSliceVarP(&config.Environment, "env", "e", config.Environment, "Specify key value pairs of environment variables to set into resulting image.")
+	cmd.Flags().StringArrayVarP(&config.Environment, "env", "e", config.Environment, "Specify a key value pair of environment variable to set into resulting image.")


Specify a key value pair of environment variable -> Specify a key value pair for an environment variable

bparees · 2016-10-27T14:22:06Z

pkg/cmd/cli/cmd/set/env.go

@@ -93,7 +93,7 @@ func NewCmdEnv(fullName string, f *clientcmd.Factory, in io.Reader, out, errout
 	cmd.Flags().StringP("containers", "c", "*", "The names of containers in the selected pod templates to change - may use wildcards")
 	cmd.Flags().StringP("from", "", "", "The name of a resource from which to inject enviroment variables")
 	cmd.Flags().StringP("prefix", "", "", "Prefix to append to variable names")
-	cmd.Flags().StringSliceVarP(&env, "env", "e", env, "Specify key value pairs of environment variables to set into each container.")
+	cmd.Flags().StringArrayVarP(&env, "env", "e", env, "Specify a key value pair of environment variable to set into each container.")


Specify a key value pair of environment variable -> Specify a key value pair for an environment variable

bparees · 2016-10-27T14:22:21Z

pkg/cmd/cli/cmd/startbuild.go

@@ -90,7 +90,7 @@ func NewCmdStartBuild(fullName string, f *clientcmd.Factory, in io.Reader, out,
 		},
 	}
 	cmd.Flags().StringVar(&o.LogLevel, "build-loglevel", o.LogLevel, "Specify the log level for the build log output")
-	cmd.Flags().StringSliceVarP(&o.Env, "env", "e", o.Env, "Specify key value pairs of environment variables to set for the build container.")
+	cmd.Flags().StringArrayVarP(&o.Env, "env", "e", o.Env, "Specify a key value pair of environment variable to set for the build container.")


Specify a key value pair of environment variable -> Specify a key value pair for an environment variable

Right, that sounds better. BTW some of the descriptions say key value pair while others have key-value pair, I suppose it would be nice to be consistent. Which one is correct?

i like key-value

bparees · 2016-10-27T14:22:56Z

pkg/cmd/util/cmd.go

+func WarnAboutCommaSeparation(errout io.Writer, values []string, flag string) {
+	for _, value := range values {
+		if strings.ContainsRune(value, ',') {
+			fmt.Fprintf(errout, "warning: %s no longer accepts comma-separated list of values. %q will be treated as a single key-value pair.\n", flag, value)


s/list/lists/

we should only warn if the value looks like "key1=value1,key2=value2". In other words, we have to see both a command and an equal after key1= for it to be a problem. There's nothing wrong with "key1=a,b,c"

bparees · 2016-10-27T14:27:00Z

@mfojtik you will need to review this also since it's changing oc set for deploymentconfigs.

mmilata · 2016-10-27T17:23:49Z

@bparees fixed the descriptions, changed the warning functions to match against .*=.*,.*=.*.

bparees

couple more test requests and then lgtm.

bparees · 2016-10-27T18:24:31Z

test/cmd/newapp.sh

+os::cmd::expect_success_and_not_text 'oc new-app ruby-helloworld-sample --param MYSQL_PASSWORD=com,ma -o yaml' 'no longer accepts comma-separated list'
+# check that warning is not printed when parameters/env are passed positionally
+os::cmd::expect_success_and_not_text 'oc new-app php PASS=one,two=three -o yaml' 'no longer accepts comma-separated list'
+os::cmd::expect_success_and_not_text 'oc new-app ruby-helloworld-sample MYSQL_PASSWORD=one,two=three -o yaml' 'no longer accepts comma-separated list'


these two tests should also confirm that the parameter/env is not split.

bparees · 2016-10-27T18:25:31Z

test/cmd/newapp.sh

+os::cmd::expect_success_and_text 'oc new-build --binary php --env X=Y,Z=W -o yaml' 'no longer accepts comma-separated list'
+os::cmd::expect_success_and_not_text 'oc new-build --binary php --env X=Y,Z,W -o yaml' 'no longer accepts comma-separated list'
+os::cmd::expect_success_and_not_text 'oc new-build --binary php --env X=Y -o yaml' 'no longer accepts comma-separated list'
+os::cmd::expect_success_and_not_text 'oc new-build --binary php X=Y,Z=W -o yaml' 'no longer accepts comma-separated list'


confirm this test does not split the env into two.

bparees · 2016-10-27T18:26:26Z

test/cmd/process.sh

+os::cmd::expect_success_and_not_text "oc process -f '${required_params}' --value=required_param=a_b_c_d" 'no longer accepts comma-separated list'
+os::cmd::expect_success_and_not_text "oc process -f '${required_params}' --value=required_param=a,b,c,d" 'no longer accepts comma-separated list'
+# warning is not printed for template values passed as positional arguments
+os::cmd::expect_success_and_not_text "oc process -f '${required_params}' required_param=a,b=c,d" 'no longer accepts comma-separated list'


confirm argument is not split.

bparees · 2016-10-27T18:28:29Z

test/cmd/templates.sh

-os::cmd::expect_success_and_text 'oc process ruby-helloworld-sample MYSQL_USER=myself MYSQL_PASSWORD=my,1%pass'  '"myself"'
-os::cmd::expect_success_and_text 'oc process MYSQL_USER=myself MYSQL_PASSWORD=my,1%pass ruby-helloworld-sample'  '"my,1%pass"'
+os::cmd::expect_success_and_text 'oc process ruby-helloworld-sample MYSQL_USER=myself MYSQL_PASSWORD=my,1%pa=s'        '"myself"'
+os::cmd::expect_success_and_text 'oc process MYSQL_USER=myself MYSQL_PASSWORD=my,1%pa=s ruby-helloworld-sample'        '"my,1%pa=s"'


looks like this covers at least one of the test suggestions i made above.

I wonder if cmd/templates.sh and cmd/process.sh should be combined into a single test script so we can do some de-duping.

Not for this PR though.

mmilata · 2016-10-28T12:13:34Z

Following assertion failed because oc new-app never supported template parameters as positional arguments (unlike oc process):

os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample MYSQL_PASSWORD=one,two=three -o yaml' 'value: one,two=three'

Pushed new version with the test removed.

danmcp · 2016-10-28T14:39:18Z

Flaked on #10988

Automatic merge from submit-queue Update github.com/spf13/pflag to latest version  **What this PR does / why we need it**: This PR updates `github.com/spf13/pflag` to the latest version. * OpenShift needs pflag bump in order to use [`StringArray`](spf13/pflag#89), and * Latest revision contains couple of bugfixes that I'd like to have there, and * Kubernetes should have the same version, which is the reason for this PR **Which issue this PR fixes**: None, related to openshift/origin#11539. **Special notes for your reviewer**: `hack/verify-godeps.sh` fails with following output even though I haven't touched that package: https://gist.github.com/mmilata/3cb84b754f9b9e7ff371a7b6c5131b09 Am I using the wrong golang/godep version perhaps? Any hint what am I doing wrong? **Release note**:  ```release-note NONE ```

mmilata · 2016-10-31T11:43:33Z

[test]

mmilata · 2016-10-31T17:22:19Z

The test got stuck on

[k8s.io] EmptyDir volumes
/data/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/test/e2e/framework/framework.go:793
  should support (root,0644,default) [Conformance]
  /data/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/test/e2e/common/empty_dir.go:97

Should be reported as a flake?

[test] again?

smarterclayton · 2016-10-31T17:40:17Z

report the test run as a flake in an issue (might be a new docker failure
mode)

[test]

On Mon, Oct 31, 2016 at 1:29 PM, OpenShift Bot [email protected]
wrote:

The Origin test job could not be run again for this pull request.

If the proposed changes in this pull request caused the job to fail,
update this pull request with new code to fix the issue(s).

If flaky tests caused the job to fail, leave a comment with links to
the GitHub issue(s) in the openshift/origin repository with the
kind/test-flake label
https://github.com/openshift/origin/issues?q=label%3Akind%2Ftest-flake
that are tracking the flakes. If no issue already exists for the flake you
encountered, create one
https://github.com/openshift/origin/issues/new.

If something else like CI system downtime or maintenance caused the
job to fail, contact a member of the Team Project Committers
https://github.com/orgs/openshift/teams/team-project-committers
group to trigger the job again.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11539 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p7K0PydX0ObkOSHDQDT95gCGOcpGks5q5iVygaJpZM4KfPRX
.

mmilata · 2016-10-31T17:49:48Z

Filed #11671

mmilata · 2016-10-31T19:27:59Z

Flake #11406

mmilata · 2016-10-31T22:07:19Z

[test]

mmilata · 2016-11-01T10:21:47Z

Flake #10988 or #10773 or both. [test]

openshift-bot · 2016-11-01T10:29:20Z

Evaluated for origin test up to d3df274

openshift-bot · 2016-11-01T11:53:12Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10904/) (Base Commit: 76dc98f)

mmilata · 2016-11-01T11:57:49Z

@bparees, modified the tests as you suggested, PTAL

bparees · 2016-11-01T18:46:44Z

lgtm [merge]

openshift-bot · 2016-11-01T18:53:25Z

Evaluated for origin merge up to d3df274

openshift-bot · 2016-11-01T20:05:57Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10904/) (Base Commit: 16306a6) (Image: devenv-rhel7_5291)

mmilata · 2016-11-03T15:10:09Z

@smarterclayton you requested a public notification for this change in #10952 (comment)

Where do I add a release note? I'm planning to send an email to users@ list as well.

smarterclayton · 2016-11-03T17:13:52Z

In openshift-docs (there should be a 3.4 release note list)

On Thu, Nov 3, 2016 at 11:10 AM, Martin Milata [email protected]
wrote:

@smarterclayton https://github.com/smarterclayton you requested a
public notification for this change in #10952 (comment)
#10952 (comment)

Where do I add a release note? I'm planning to send an email to users@
list as well.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11539 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p8u1mWMz_GHDWPM5_nVhZf7eWFyHks5q6flUgaJpZM4KfPRX
.

mmilata mentioned this pull request Oct 24, 2016

Support loading template parameter values from file #10952

Closed

mmilata mentioned this pull request Oct 25, 2016

Do not use comma-separated template parameters openshift/openshift-docs#3107

Merged

mmilata force-pushed the disallow-multiple-template-params branch from 9312063 to 5f0e901 Compare October 25, 2016 13:20

bparees requested changes Oct 25, 2016

View reviewed changes

bparees self-assigned this Oct 25, 2016

bump(github.com/spf13/pflag): 5ccb023bc27df288a957c5e994cd44fd19619465

8fd5cd2

mmilata force-pushed the disallow-multiple-template-params branch 2 times, most recently from 2ef8bd0 to 7618ef3 Compare October 27, 2016 11:38

mmilata mentioned this pull request Oct 27, 2016

Do not use comma-separated environment variables openshift/openshift-docs#3123

Merged

bparees requested changes Oct 27, 2016

View reviewed changes

mmilata mentioned this pull request Oct 27, 2016

Update github.com/spf13/pflag to latest version kubernetes/kubernetes#35700

Merged

mmilata force-pushed the disallow-multiple-template-params branch from 7618ef3 to 145dcb7 Compare October 27, 2016 17:20

bparees requested changes Oct 27, 2016

View reviewed changes

mmilata force-pushed the disallow-multiple-template-params branch from 145dcb7 to b39d554 Compare October 28, 2016 00:19

cli: remove support for comma-separated template values

d3df274

mmilata force-pushed the disallow-multiple-template-params branch from b39d554 to d3df274 Compare October 28, 2016 12:09

bparees mentioned this pull request Nov 1, 2016

Merging jenkins plugin test templates into one #11711

Merged

openshift-bot merged commit d7c6243 into openshift:master Nov 1, 2016

mmilata mentioned this pull request Nov 3, 2016

Support using files as values for template parameters #10687

Closed

mmilata mentioned this pull request Nov 4, 2016

OpenShift 1.4/3.4 Release Notes Tracker openshift/openshift-docs#2933

Closed

php-coder mentioned this pull request Nov 30, 2016

docs/cli.md: drop support for passing comma-separated parameters #12076

Closed

php-coder mentioned this pull request Dec 14, 2016

Env var with comma in it doesn't work on new-app #12181

Closed

jfchevrette mentioned this pull request Jun 7, 2017

OpenShift: need to use the -p parameter multiple time when creating the app RocketChat/docs-old#303

Merged

cli: drop support for passing comma-separated template parameters through --param/--value #11539

cli: drop support for passing comma-separated template parameters through --param/--value #11539

Conversation

mmilata commented Oct 24, 2016 • edited Loading

liggitt commented Oct 24, 2016

liggitt commented Oct 24, 2016

liggitt commented Oct 24, 2016

mmilata commented Oct 25, 2016

mmilata commented Oct 25, 2016

mmilata commented Oct 25, 2016 • edited Loading

liggitt commented Oct 25, 2016

bparees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmilata commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Oct 27, 2016

mmilata commented Oct 27, 2016

bparees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmilata commented Oct 28, 2016

danmcp commented Oct 28, 2016

mmilata commented Oct 31, 2016

mmilata commented Oct 31, 2016

smarterclayton commented Oct 31, 2016

mmilata commented Oct 31, 2016

mmilata commented Oct 31, 2016

mmilata commented Oct 31, 2016

mmilata commented Nov 1, 2016

openshift-bot commented Nov 1, 2016

openshift-bot commented Nov 1, 2016

mmilata commented Nov 1, 2016

bparees commented Nov 1, 2016

openshift-bot commented Nov 1, 2016

openshift-bot commented Nov 1, 2016 • edited Loading

mmilata commented Nov 3, 2016

smarterclayton commented Nov 3, 2016

mmilata commented Oct 24, 2016 •

edited

Loading

mmilata commented Oct 25, 2016 •

edited

Loading

openshift-bot commented Nov 1, 2016 •

edited

Loading