set env vars on oc new-app of template #12048

gabemontero · 2016-11-28T21:03:04Z

(the simplicity of the change seems to good to be true ... let me know what if anything I am missing - thx)

csrwng

Just a couple of comments but looks good otherwise

csrwng · 2016-11-28T21:09:51Z

pkg/generate/app/cmd/newapp.go

+	// before returning, if this is a template run, and environment variables were passed in, let's apply to the environment variables
+	// to every pod template object
+	if len(templateObjects) > 0 {
+		for i := range objects {


you should iterate over templateObjects since generated objects already have the environment applied to them.

Yeah, in the spirit of this, I moved (and will soon push) the loop into the buildTemplate method. It seems to work correctly based on local testing, but let me know if I'm missing some ramification of such a move.

csrwng · 2016-11-28T21:14:11Z

test/cmd/newapp.sh

@@ -62,6 +62,7 @@ os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'MY
 os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'ADMIN_USERNAME'
 os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'ADMIN_PASSWORD'
 os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample --param MYSQL_PASSWORD=hello -o yaml' 'hello'
+os::cmd::expect_success 'oc new-app -e FOO=BAR -f examples/jenkins/jenkins-ephemeral-template.json -o json | grep FOO'


Not sure if it's possible, but ideally we should prove to ourselves that for generated objects the environment is applied only once.

When looking for text in output prefer os::cmd::expect_success_and_(not_)text rather than grep.

When looking for specific output, prefer to inspect the created object in the server (create it, then oc get ... -o jsonpath ... so you can be certain you are not picking up some other unrelated FOO.

OK, switched to expect_success_and_text.

I could not get -o jsonpath to work as we needed. I'm punting on that one unless someone can enlighten me.

By introducing along with expect_success_and_text the use of grep and wc (saw other examples of this in test/cmd/*.sh), I can confirm that the env var is applied only once.

That said, my initial assumption is that the use of -e should override any pre-existing env vars set in the template. Is that correct @csrwng? If so, the current implementation is not overriding.

I'll push the existing changes up for review momentarily and then investigate the "to override or not override" angle.

stevekuznetsov · 2016-11-29T14:24:55Z

test/cmd/newapp.sh

@@ -62,6 +62,7 @@ os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'MY
 os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'ADMIN_USERNAME'
 os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'ADMIN_PASSWORD'
 os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample --param MYSQL_PASSWORD=hello -o yaml' 'hello'
+os::cmd::expect_success 'oc new-app -e FOO=BAR -f examples/jenkins/jenkins-ephemeral-template.json -o json | grep FOO'


When looking for text in output prefer os::cmd::expect_success_and_(not_)text rather than grep.

When looking for specific output, prefer to inspect the created object in the server (create it, then oc get ... -o jsonpath ... so you can be certain you are not picking up some other unrelated FOO.

gabemontero · 2016-11-30T16:09:12Z

Intermediate responses to comments pushed ... in the interim will dive into override question I noted earlier.

gabemontero · 2016-11-30T16:21:40Z

Have override sorted out ... will push momentarily

gabemontero · 2016-11-30T16:26:38Z

Latest pushed ... PTAL

stevekuznetsov · 2016-11-30T17:34:39Z

test/cmd/newapp.sh

@@ -62,6 +62,8 @@ os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'MY
 os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'ADMIN_USERNAME'
 os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'ADMIN_PASSWORD'
 os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample --param MYSQL_PASSWORD=hello -o yaml' 'hello'
+os::cmd::expect_success_and_text  'oc new-app -e FOO=BAR -f examples/jenkins/jenkins-ephemeral-template.json -o json | grep FOO | wc -l' '1'
+os::cmd::expect_success_and_text  'oc new-app -e OPENSHIFT_ENABLE_OAUTH=false -f examples/jenkins/jenkins-ephemeral-template.json -o json | grep -A 1 OPENSHIFT_ENABLE_OAUTH | grep false | wc -l' '1'


I think a better approach here would be (wrapped in the correct os::cmd invocations):

oc new-app -e OPENSHIFT_ENABLE_OAUTH=false -f examples/jenkins/jenkins-ephemeral-template.json oc get <whatever_object> -o jsonpath=<path_to_environment_field>

Then you could directly check that whatever part of whatever object you are creating has the field you need. The grep -A1 check is understandably very fragile.

If you want to do counting, etc, use a Golang template.

As an example, I manually run oc new-app on the jenkins template. It creates a DC named jenkins. I then run:

oc get dc/jenkins -o jsonpath='.spec.replicas'

When using -o json, the value of replicas is clearly 1. However, running the above -o jsonpath command simply returns:

.spec.replicas

with nothing about the actual value being 1. How do you get the actual value?

You'll need to surround the JSONPath with braces:

oc get dc/jenkins -o jsonpath='{.spec.replicas}'

See the k8s documentation for more detail.

Ah ... it needs to be oc get dc/jenkins -o jsonpath='{.spec.replicas}'

Figured it out as @stevekuznetsov was posting :-) ... thanks @stevekuznetsov

gabemontero · 2016-11-30T18:06:18Z

On Wed, Nov 30, 2016 at 12:54 PM, Gabe Montero ***@***.***> wrote: On Wed, Nov 30, 2016 at 12:34 PM, Steve Kuznetsov < ***@***.***> wrote: > ***@***.**** requested changes on this pull request. > ------------------------------ > > In test/cmd/newapp.sh > <#12048 (review)> > : > > > @@ -62,6 +62,8 @@ os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'MY > os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'ADMIN_USERNAME' > os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'ADMIN_PASSWORD' > os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample --param MYSQL_PASSWORD=hello -o yaml' 'hello' > +os::cmd::expect_success_and_text 'oc new-app -e FOO=BAR -f examples/jenkins/jenkins-ephemeral-template.json -o json | grep FOO | wc -l' '1' > +os::cmd::expect_success_and_text 'oc new-app -e OPENSHIFT_ENABLE_OAUTH=false -f examples/jenkins/jenkins-ephemeral-template.json -o json | grep -A 1 OPENSHIFT_ENABLE_OAUTH | grep false | wc -l' '1' > > I think a better approach here would be (wrapped in the correct os::cmd > invocations): > > oc new-app -e OPENSHIFT_ENABLE_OAUTH=false -f examples/jenkins/jenkins-ephemeral-template.json > > When actually creating the objects, from what I can tell from the test

scripts, those objects will be present (and potentially impact) subsequent commands ... so I'm somewhat hesitant of that approach. In theory you can supply `-o jsonpath` to `oc new-app` as well, but circumnavigating that morass appeared more problematic than doing it for `oc get` when I experimented with it :-) ... but if I could get it working for even `oc get` manually, maybe I'll see what's additionally needed. I'll contemplate / experiment some more.

…

> oc get <whatever_object> -o jsonpath=<path_to_environment_field> > > Then you could directly check that whatever part of whatever object you > are creating has the field you need. The grep -A1 check is > understandably very fragile. > > If you want to do counting, etc, use a Golang template. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#12048 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ADbadEO9oCuHHmKPjfdZ9e1-6eUlhSyEks5rDbO7gaJpZM4K-Qik> > . >

gabemontero · 2016-11-30T19:11:06Z

OK pushed update .... now using -o jsonpath ... thx @stevekuznetsov again for the help

fyi ... was able to leverage it with oc new-app, so no actual creation of objects.

Not sure if jsonpath filter along with os::cmd::expect_success_and_text will complain if more than one entry matches the search criteria per @csrwng 's request (unlike the grep/wc version), but I agree this is cleaner.

PTAL

stevekuznetsov · 2016-11-30T20:25:40Z

test/cmd/newapp.sh

@@ -62,6 +62,8 @@ os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'MY
 os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'ADMIN_USERNAME'
 os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample -o yaml' 'ADMIN_PASSWORD'
 os::cmd::expect_success_and_text 'oc new-app ruby-helloworld-sample --param MYSQL_PASSWORD=hello -o yaml' 'hello'
+os::cmd::expect_success_and_text  'oc new-app -e FOO=BAR -f examples/jenkins/jenkins-ephemeral-template.json -o jsonpath="{.items[?(@.kind==\"DeploymentConfig\")].spec.template.spec.containers[0].env[?(@.name==\"FOO\")].value}" ' 'BAR'


As you mention this will output more than one value if you match more than one environment variable. I would suggest expecting the text '^BAR$' to combat that. Other than that, these look good to me.

Ah, I was trying to refer to 2 instances of FOO=BAR in the env[], but based on what you are saying is also a concern FOO1=BAR1 and FOO2=BAR2

Will push the update momentarily - thx.

Update pushed

stevekuznetsov

Test look good to me. Deferring to @csrwng for the rest.

csrwng · 2016-11-30T21:11:53Z

pkg/generate/app/cmd/newapp.go

+				}
+			}
+		}
+	*/


@gabemontero pls remove

daahhhhhh - forgot that :-)

update pushed

csrwng · 2016-12-01T02:47:23Z

LGTM [merge]

openshift-bot · 2016-12-01T02:53:25Z

Evaluated for origin merge up to 53215d9

openshift-bot · 2016-12-01T02:53:25Z

[Test]ing while waiting on the merge queue

openshift-bot · 2016-12-01T03:05:22Z

Evaluated for origin test up to 53215d9

openshift-bot · 2016-12-01T04:38:47Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11843/) (Base Commit: 166273b)

openshift-bot · 2016-12-01T11:13:28Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11851/) (Base Commit: 3929053) (Image: devenv-rhel7_5454)

csrwng reviewed Nov 28, 2016

View reviewed changes

stevekuznetsov suggested changes Nov 29, 2016

View reviewed changes

gabemontero force-pushed the newAppEnvs branch from fcdd379 to b238e9e Compare November 30, 2016 16:08

gabemontero force-pushed the newAppEnvs branch from b238e9e to 838fc79 Compare November 30, 2016 16:26

stevekuznetsov suggested changes Nov 30, 2016

View reviewed changes

gabemontero force-pushed the newAppEnvs branch from 838fc79 to 75ac180 Compare November 30, 2016 19:07

stevekuznetsov suggested changes Nov 30, 2016

View reviewed changes

gabemontero force-pushed the newAppEnvs branch from 75ac180 to 18c54db Compare November 30, 2016 20:37

stevekuznetsov approved these changes Nov 30, 2016

View reviewed changes

csrwng reviewed Nov 30, 2016

View reviewed changes

gabemontero force-pushed the newAppEnvs branch from 18c54db to 21600bd Compare November 30, 2016 21:47

set env vars on oc new-app of template

53215d9

gabemontero force-pushed the newAppEnvs branch from 21600bd to 53215d9 Compare November 30, 2016 22:18

gabemontero mentioned this pull request Dec 1, 2016

set env vars on oc new-app of template openshift/openshift-docs#3304

Merged

openshift-bot merged commit 91f6636 into openshift:master Dec 1, 2016

gabemontero deleted the newAppEnvs branch December 1, 2016 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set env vars on oc new-app of template #12048

set env vars on oc new-app of template #12048

gabemontero commented Nov 28, 2016

csrwng left a comment

csrwng Nov 28, 2016

gabemontero Nov 30, 2016

csrwng Nov 28, 2016

stevekuznetsov Nov 29, 2016

gabemontero Nov 30, 2016

stevekuznetsov Nov 29, 2016

gabemontero commented Nov 30, 2016

gabemontero commented Nov 30, 2016

gabemontero commented Nov 30, 2016

stevekuznetsov Nov 30, 2016

gabemontero Nov 30, 2016

stevekuznetsov Nov 30, 2016

gabemontero Nov 30, 2016

gabemontero Nov 30, 2016

gabemontero commented Nov 30, 2016 via email

gabemontero commented Nov 30, 2016

stevekuznetsov Nov 30, 2016

gabemontero Nov 30, 2016

gabemontero Nov 30, 2016

stevekuznetsov left a comment

csrwng Nov 30, 2016

gabemontero Nov 30, 2016

gabemontero Nov 30, 2016

csrwng commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016 •

edited

Loading

set env vars on oc new-app of template #12048

set env vars on oc new-app of template #12048

Conversation

gabemontero commented Nov 28, 2016

csrwng 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero commented Nov 30, 2016

gabemontero commented Nov 30, 2016

gabemontero commented Nov 30, 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

gabemontero commented Nov 30, 2016 via email

gabemontero commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov 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

csrwng commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016 • edited Loading

openshift-bot commented Dec 1, 2016 •

edited

Loading