oc new-app/new-build/process: add --env-file/--param-file parameters #12164

mmilata · 2016-12-06T22:46:19Z

Trello card: https://trello.com/c/BMuLvOny/968-5-supply-oc-new-app-and-oc-process-parameter-list-env-vars-via-a-file-app-creation

Follow-up of #10766

mmilata · 2016-12-06T22:46:44Z

[test]

stevekuznetsov · 2016-12-06T23:45:39Z

test/cmd/newapp.sh

@@ -82,6 +82,22 @@ os::cmd::expect_success_and_not_text 'oc new-app ruby-helloworld-sample --param
 os::cmd::expect_success_and_text 'oc new-app php PASS=one,two=three -o yaml' 'value: one,two=three'
 os::cmd::expect_success_and_not_text 'oc new-app php PASS=one,two=three -o yaml' 'no longer accepts comma-separated list'

+# check that we can populate template parameters from file
+param_file="${OS_ROOT}/test/testdata/test-cmd-newapp-params.env"
+os::cmd::expect_success_and_text "oc new-app ruby-helloworld-sample --param-file ${param_file} -o yaml" 'value: thisisapassword'


Use jsonpath to query for the specific value you want to check instead of checking that some small string exists in the broader YAML output. See https://github.com/openshift/origin/blob/master/test/cmd/newapp.sh#L65

mmilata · 2016-12-07T15:04:54Z

@bparees PTAL

bparees · 2016-12-07T17:53:42Z

flake #8571
[test]

bparees · 2016-12-07T17:55:00Z

pkg/cmd/cli/cmd/newapp.go

@@ -168,8 +165,12 @@ func NewCmdNewApplication(name, baseName string, f *clientcmd.Factory, out, erro
 	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().StringArrayVarP(&config.TemplateParameters, "param", "p", config.TemplateParameters, "Specify a key-value pair (e.g., -p FOO=BAR) to set/override a parameter value in the template.")
+	cmd.Flags().StringVar(&config.TemplateParametersFile, "param-file", "", "File containing environment parameter values to set/override in the template.")


s/environment//

bparees · 2016-12-07T17:58:49Z

pkg/generate/app/cmd/newapp.go

-	env, duplicateEnv, envErrs := cmdutil.ParseEnvironmentArguments(c.Environment)
-	for _, s := range duplicateEnv {
-		fmt.Fprintf(c.ErrOut, "warning: The environment variable %q was overwritten", s)
+func parseAndCombine(what string, cmdline []string, file string, stdin io.Reader, errout io.Writer) (app.Environment, []error) {


bparees · 2016-12-07T18:01:12Z

pkg/generate/app/env.go

-func ParseEnvironment(vals ...string) Environment {
+// ParseEnvironmentAllowEmpty converts the provided strings in key=value form
+// into environment entries. If case there's no equals sign in a string, it's
+// considered as a key with empty


"considered as a key with empty value"

bparees · 2016-12-07T18:02:18Z

pkg/generate/app/env.go

@@ -44,6 +70,22 @@ func (e Environment) Add(envs ...map[string]string) {
 	}
 }

+// Join adds the environment variables to the current environment. In case of
+// key confilct the old value is kept. Conflicting keys are returned as a


Will edit go files with :set spell from now.

bparees · 2016-12-07T18:19:46Z

@mmilata what was the ultimate resolution to the "how to handle multiline values in files" question? i see you're using a library, what does it support as far as multiline values? did @smarterclayton agree to it?

bparees · 2016-12-07T18:20:53Z

@openshift/api-review needs review for new command line args (support providing a file for list of env/parameter key/values) and also agreement on the multiline value syntax supported within the files.

mmilata · 2016-12-07T18:51:11Z

@bparees Clayton suggested this one. It supports \n and \\ replacement when the right hand side is enclosed in quotes. There's some bash incompatibility but that could easily be improved.

bparees · 2016-12-07T18:53:28Z

@bparees Clayton suggested this one. It supports \n and \ replacement when the right hand side is enclosed in quotes. There's some bash incompatibility but that could easily be improved.

cool. (and thanks @smarterclayton)

bparees · 2016-12-07T19:59:29Z

test/cmd/process.sh

@@ -6,7 +6,7 @@ trap os::test::junit::reconcile_output EXIT
 (
  set +e
  oc delete all,templates --all
-  oc delete user someval
+  oc delete user someval someval=moreval someval=moreval2 someval=moreval3


A test in that file creates these users but doesn't delete them thus you can't run that file twice in a row. Not related to the envfile feature at all.

i'm a little surprised "=" is even a valid character in a username, but ok cool.

bparees · 2016-12-07T20:00:15Z

test/cmd/process.sh

@@ -88,6 +88,9 @@ os::cmd::expect_success_and_not_text "oc process -f '${required_params}' require
 os::cmd::expect_success_and_text "oc process -f '${required_params}' --value=required_param='`cat ${OS_ROOT}/test/testdata/multiline.txt`'" 'also,with=commas'
 os::cmd::expect_success_and_text "oc process -f '${required_params}' --param=required_param='`cat ${OS_ROOT}/test/testdata/multiline.txt`'" 'also,with=commas'

+# set template parameters from parameter file with multiline values
+param_file="${OS_ROOT}/test/testdata/template_required_params.env"
+os::cmd::expect_success_and_text "oc process -f '${required_params}' --param-file=${param_file} -o yaml" 'first$'


test from stdin?

i see it's covered in test/cmd/templates.sh

i wonder if we shouldn't combine test/cmd/templates.sh and test/cmd/process.sh, seems like we probably have overlap and it's not like process can do anything except work with templates.

Sure, makes sense. Should I do that as part of this PR or open a new one?

let's just do it here.

smarterclayton · 2016-12-07T20:11:51Z

pkg/cmd/cli/cmd/newapp.go

@@ -168,8 +165,12 @@ func NewCmdNewApplication(name, baseName string, f *clientcmd.Factory, out, erro
 	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().StringArrayVarP(&config.TemplateParameters, "param", "p", config.TemplateParameters, "Specify a key-value pair (e.g., -p FOO=BAR) to set/override a parameter value in the template.")
+	cmd.Flags().StringVar(&config.TemplateParametersFile, "param-file", "", "File containing parameter values to set/override in the template.")


Accept multiple files, i.e. StringArrayVar

For all of these

smarterclayton · 2016-12-07T20:12:10Z

pkg/cmd/cli/cmd/newapp.go

@@ -168,8 +165,12 @@ func NewCmdNewApplication(name, baseName string, f *clientcmd.Factory, out, erro
 	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().StringArrayVarP(&config.TemplateParameters, "param", "p", config.TemplateParameters, "Specify a key-value pair (e.g., -p FOO=BAR) to set/override a parameter value in the template.")
+	cmd.Flags().StringVar(&config.TemplateParametersFile, "param-file", "", "File containing parameter values to set/override in the template.")


For all of these

smarterclayton · 2016-12-07T20:12:46Z

pkg/generate/app/cmd/newapp.go

+//
+//If "file" is "-" the file contents will be read from argument "stdin" (unless
+//it's nil).
+func parseAndCombine(what string, cmdline []string, filename string, stdin io.Reader, errout io.Writer) (app.Environment, []error) {


Extract this into common library code (app or something even higher)

smarterclayton · 2016-12-07T20:13:39Z

pkg/generate/app/env.go

+// Join adds the environment variables to the current environment. In case of
+// key conflict the old value is kept. Conflicting keys are returned as a
+// slice.
+func (e Environment) Join(more Environment) []string {


Probably more consistent to call this AddIfNotPresent(...)

smarterclayton · 2016-12-07T21:00:36Z

Ldap.

…

On Wed, Dec 7, 2016 at 3:22 PM, Ben Parees ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/cmd/process.sh <#12164>: > @@ -6,7 +6,7 @@ trap os::test::junit::reconcile_output EXIT ( set +e oc delete all,templates --all - oc delete user someval + oc delete user someval someval=moreval someval=moreval2 someval=moreval3 i'm a little surprised "=" is even a valid character in a username, but ok cool. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12164>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p8autM8m4q_uEBNdArRCJIH7VQLzks5rFxWDgaJpZM4LF-vU> .

mmilata · 2016-12-08T15:12:12Z

Pushed version that addresses problems found in review so far. test/cmd/process.sh has been merged into test/cmd/templates.sh, the individual tests were left as-is.
PTAL

bparees · 2016-12-08T15:20:20Z

@openshift/cli-review ptal

mmilata · 2016-12-08T16:33:12Z

Flake #8571 & outdated man pages which are regenerated now. [test]

mmilata · 2016-12-08T17:49:14Z

Documentation: openshift/openshift-docs#3347

mmilata · 2016-12-08T18:21:44Z

Still flake #8571.

fabianofranz · 2016-12-08T18:48:32Z

pkg/generate/app/env.go

+// ParseEnvironmentAllowEmpty converts the provided strings in key=value form
+// into environment entries. In case there's no equals sign in a string, it's
+// considered as a key with empty value.
+func ParseEnvironmentAllowEmpty(vals ...string) Environment {


In case there's no equals sign in a string, it's considered as a key with empty value

Is this only for docker compose?

fabianofranz · 2016-12-08T18:53:12Z

pkg/generate/app/env.go

@@ -24,6 +30,26 @@ func ParseEnvironment(vals ...string) Environment {
 	return env
 }

+func ParseEnvironment(vals ...string) (Environment, []string, []error) {


This looks like it would accepts invalid things for env keys. What happens if you do for example

--env="S P A C E S=test"

or have that pair in the file? Env vars are very strict about what's accepted for names, since you still have validArgumentEnvironment, make use of it.

Also godoc.

Added IsValidArgumentEnvironment check to ParseEnvironment as well as LoadEnvironmentFile because godotenv library allows such keys.

fabianofranz · 2016-12-08T18:55:26Z

pkg/generate/app/env.go

+	return env, nil
+}
+
+// ParseAndCombineEnvironment parses key-value records from the command line


"... parses key=value records ..." for clarity.

fabianofranz · 2016-12-08T19:00:59Z

pkg/generate/app/env.go

+			errs = append(errs, fileErr)
+		}
+
+		duplicates = vars.AddIfNotPresent(fileVars)


Make it clear in the godoc for this function that env pairs coming in the first argument takes precedence over the ones in files.

fabianofranz · 2016-12-08T19:03:10Z

pkg/generate/app/env.go

+//
+// If a file is "-" the file contents will be read from argument stdin (unless
+// it's nil).
+func ParseAndCombineEnvironment(cmdline []string, filenames []string, stdin io.Reader, dupfn func(string, string) error) (Environment, []error) {


Just call it env []string or envs []string instead of cmdline []string, for proper separation of concerns. This function doesn't understand anything about cmd line stuff, and it's exported.

fabianofranz · 2016-12-08T19:06:01Z

pkg/generate/app/env.go

+// If a file is "-" the file contents will be read from argument stdin (unless
+// it's nil).
+func ParseAndCombineEnvironment(cmdline []string, filenames []string, stdin io.Reader, dupfn func(string, string) error) (Environment, []error) {
+	vars, duplicates, errs := ParseEnvironment(cmdline...)


godoc should also mention that this function doesn't return parsing errors right away, but accumulates it with the ones returned by the provided duplicates function.

fabianofranz · 2016-12-08T19:10:06Z

pkg/generate/app/env.go

+	}
+
+	for _, fname := range filenames {
+		fileVars, fileErr := LoadEnvironmentFile(fname, stdin)


godoc should also mention that it doesn't fail right away for errors loading the provided file. In fact, it feels like it should, if I do something completely wrong like --env-file=/nonexistent.

Fair enough, I'll change the function to return after encountering first error.

fabianofranz · 2016-12-08T19:16:25Z

test/cmd/newapp.sh

@@ -82,6 +82,20 @@ os::cmd::expect_success_and_not_text 'oc new-app ruby-helloworld-sample --param
 os::cmd::expect_success_and_text 'oc new-app php PASS=one,two=three -o yaml' 'value: one,two=three'
 os::cmd::expect_success_and_not_text 'oc new-app php PASS=one,two=three -o yaml' 'no longer accepts comma-separated list'

+# check that we can populate template parameters from file
+param_file="${OS_ROOT}/test/testdata/test-cmd-newapp-params.env"
+os::cmd::expect_success_and_text "oc new-app ruby-helloworld-sample --param-file ${param_file} -o jsonpath='{.items[?(@.kind==\"DeploymentConfig\")].spec.template.spec.containers[0].env[?(@.name==\"MYSQL_PASSWORD\")].value}'" 'thisisapassword'


--param-file needs failure test cases. Things like

--param-file invalid --param-file /path/to/empty-params-file --param-file /path/to/malformed-params-file (keys with invalid chars like *&*$%&=test, lines without a KEY=VALUE pattern, etc) --param-file /path/to/access-denied-file

Will add failure test cases. How can I cause access denied if the tests are run as root? Should empty file really cause an error?

How can I cause access denied if the tests are run as root?

Ok, I'm ok with skipping this one. :)

Should empty file really cause an error?

No, it's ok to just interpret that as "no env vars to set", but still should have a test.

Also

--param-file directory

Good point, passing a directory does nothing, silently. Opened joho/godotenv#22 and updated the branch with a workaround that Stats the file first.

fabianofranz · 2016-12-08T19:16:48Z

test/cmd/newapp.sh

+
+# check that we can set environment variables from env file
+env_file="${OS_ROOT}/test/testdata/test-cmd-newapp-env.env"
+os::cmd::expect_success_and_text "oc new-app php --env-file ${env_file} -o jsonpath='{.items[?(@.kind==\"DeploymentConfig\")].spec.template.spec.containers[0].env[?(@.name==\"SOME_VAR\")].value}'" 'envvarfromfile'


Same for --env-file failure test cases.

mmilata · 2016-12-09T18:49:50Z

Updated, PTAL @fabianofranz

mmilata · 2016-12-12T16:56:27Z

flake #8502 [test]

mmilata · 2016-12-12T17:55:54Z

still #8502 [test]

mmilata · 2016-12-13T13:26:55Z

flake #12235 [test]

mmilata · 2016-12-13T14:52:20Z

flake #9886, posibly #11747

bparees · 2016-12-14T02:24:34Z

lgtm but needs a rebase.

New dependency.

test/cmd/process.sh has been merged into test/cmd/templates.sh since they test the same functionality.

openshift-bot · 2016-12-14T11:21:40Z

Evaluated for origin test up to 53e4af7

mmilata · 2016-12-14T11:22:05Z

@bparees rebased.

openshift-bot · 2016-12-14T12:37:35Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12372/) (Base Commit: 41702ed)

bparees · 2016-12-14T13:27:56Z

[merge]

openshift-bot · 2016-12-14T13:29:15Z

Evaluated for origin merge up to 53e4af7

openshift-bot · 2016-12-14T13:29:24Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12373/) (Base Commit: ec9ff39) (Image: devenv-rhel7_5544)

stevekuznetsov reviewed Dec 6, 2016

View reviewed changes

mmilata force-pushed the env-file branch from d1751e3 to 8b7e176 Compare December 7, 2016 15:04

mmilata force-pushed the env-file branch from 8b7e176 to a525da1 Compare December 7, 2016 18:03

bparees requested changes Dec 7, 2016

View reviewed changes

bparees added the needs-api-review label Dec 7, 2016

mmilata force-pushed the env-file branch from a525da1 to d670e2d Compare December 7, 2016 18:38

bparees self-assigned this Dec 7, 2016

bparees reviewed Dec 7, 2016

View reviewed changes

smarterclayton reviewed Dec 7, 2016

View reviewed changes

mmilata force-pushed the env-file branch from d670e2d to c47146e Compare December 8, 2016 15:07

mmilata force-pushed the env-file branch from c47146e to 9ebcda4 Compare December 8, 2016 16:26

mmilata mentioned this pull request Dec 8, 2016

Document --param-file and --env-file oc arguments openshift/openshift-docs#3347

Merged

fabianofranz reviewed Dec 8, 2016

View reviewed changes

mmilata force-pushed the env-file branch from 9ebcda4 to 9ccbf31 Compare December 9, 2016 18:27

mmilata force-pushed the env-file branch from 9ccbf31 to d5ddc53 Compare December 12, 2016 15:03

mmilata force-pushed the env-file branch from d5ddc53 to f70fdf7 Compare December 12, 2016 20:51

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2016

mmilata added 2 commits December 14, 2016 12:17

bump(github.com/joho/godotenv): 4ed13390c0acd2ff4e371e64d8b97c8954138243

a803c00

New dependency.

new-app/new-build/process: read envvars/params from file

53e4af7

test/cmd/process.sh has been merged into test/cmd/templates.sh since they test the same functionality.

mmilata force-pushed the env-file branch from f70fdf7 to 53e4af7 Compare December 14, 2016 11:19

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2016

openshift-bot merged commit 4c96ea4 into openshift:master Dec 14, 2016

mmilata mentioned this pull request Jan 17, 2017

Add option to use file with environment variables in new-app #7585

Closed

bparees mentioned this pull request Jan 21, 2017

PR for using a env file in oc cmdline #10766

Closed

oc new-app/new-build/process: add --env-file/--param-file parameters #12164

oc new-app/new-build/process: add --env-file/--param-file parameters #12164

Conversation

mmilata commented Dec 6, 2016 • edited Loading

mmilata commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmilata commented Dec 7, 2016

bparees commented Dec 7, 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

bparees commented Dec 7, 2016

bparees commented Dec 7, 2016

mmilata commented Dec 7, 2016

bparees commented Dec 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Dec 7, 2016 via email

mmilata commented Dec 8, 2016

bparees commented Dec 8, 2016

mmilata commented Dec 8, 2016

mmilata commented Dec 8, 2016

mmilata commented Dec 8, 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

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

mmilata commented Dec 9, 2016

mmilata commented Dec 12, 2016

mmilata commented Dec 12, 2016

mmilata commented Dec 13, 2016

mmilata commented Dec 13, 2016

bparees commented Dec 14, 2016

openshift-bot commented Dec 14, 2016

mmilata commented Dec 14, 2016

openshift-bot commented Dec 14, 2016

bparees commented Dec 14, 2016

openshift-bot commented Dec 14, 2016

openshift-bot commented Dec 14, 2016 • edited Loading

mmilata commented Dec 6, 2016 •

edited

Loading

openshift-bot commented Dec 14, 2016 •

edited

Loading