Merging jenkins plugin test templates into one #11711

jupierce · 2016-11-01T18:46:30Z

When running locally on a developer's system, the previous Jenkins plugin test would conditionally fork and load:
jenkins-ephemeral-template-test-new-plugin.json
or
jenkins-ephemeral-template-no-oauth.json

depending on whether the developer had built an image using the OpenShift Jenkins plugin PR-Testing instructions.

This implementation required that both templates be maintained -- more importantly, failure to do so was undetectable by CI testing. This problem already manifested once when the new-plugin template was not updated to specify the "jenkins" service account.

Instructing the extended tests to target the local image will also now require an environment variable to be specified before running the tests: USE_SNAPSHOT_JENKINS_IMAGE=1 . This makes explicit the developer's desire to target the local (versus official) jenkins image. A corresponding PR in openshift/jenkins-plugin will be opened to update the README.

@bparees @gabemontero ptal

gabemontero

Only 1 idea / potential follow up PR

Generally, IGTM either way

gabemontero · 2016-11-01T18:56:54Z

test/extended/image_ecosystem/plugin.go

+		useSnapshotImage := os.Getenv(useLocalPluginSnapshotEnvVarName) != "" && snapshotImagePresent
+
+		//TODO disabling oauth until we can update getAdminPassword path to handle oauth (perhaps borrow from oauth integration tests)
+		newAppArgs := []string{exutil.FixturePath("testdata", "jenkins-ephemeral-template-no-oauth.json")}


Arguably orthogonal to your change, but if we could somehow copy examples/jenkins/jenkins-ephemeral-template.json from the origin source tree to a temporary location (the forcepull.go tests do something similar if I remember correctly), and somehow find this snippet in the template:

{ "name": "OPENSHIFT_ENABLE_OAUTH", "value": "true" },

and change the value to false, we could in fact use the official example template while still disabling oauth.

Assuming this is sufficiently simple (certainly simpler I think than handling oauth flows in our extended tests) of course.

Perhaps to be done in a separate PR if we want to get this change in quickly.

we can make ENABLE_OAUTH a parameter (with a default of true) in the jenkins-ephemeral(and jenkins-persistent) official templates if we wanted to do this, to make it easier.

Will do. Definitely the intent of this PR to reduce the templates we need to maintain.

bparees

one nit and lgtm pending passing tests.

bparees · 2016-11-01T18:59:32Z

test/extended/image_ecosystem/plugin.go

+			ginkgolog("")
+
+			// Create an imagestream based on the Jenkins' plugin PR-Testing image (https://github.com/openshift/jenkins-plugin/blob/master/PR-Testing/README).
+			err = oc.Run("new-build").Args("-D", "FROM openshift/jenkins-plugin-snapshot-test", "--to", "jenkins-plugin-snapshot-test").Execute()


make the FROM image name a constant and use the same constant on line 418 (the "tag" array).

i don't think there should be any issue with having ":latest" on the image name in both cases.

gabemontero · 2016-11-01T19:16:53Z

On Tue, Nov 1, 2016 at 3:04 PM, Ben Parees [email protected] wrote:

@bparees commented on this pull request.

In test/extended/image_ecosystem/plugin.go
#11711:
@@ -414,18 +417,46 @@ var _ = g.Describe("[image_ecosystem][jenkins][Slow] openshift pipeline plugin",
g.By("kick off the build for the jenkins ephermeral and application templates")
tag := []string{"openshift/jenkins-plugin-snapshot-test:latest"}
hexIDs, err := exutil.DumpAndReturnTagging(tag)
var jenkinsEphemeralPath string
var testingSnapshot bool
if len(hexIDs) > 0 && err == nil {
    // found an openshift pipeline plugin test image, must be testing a proposed change to the plugin
    jenkinsEphemeralPath = exutil.FixturePath("testdata", "jenkins-ephemeral-template-test-new-plugin.json")
    testingSnapshot = true
// If the user has expressed an interest in local plugin testing by setting the
// SNAPSHOT_JENKINS_IMAGE environment variable, try to use the local image. Inform them
// either about which image is being used in case their test fails.
snapshotImagePresent := len(hexIDs) > 0 && err == nil
useSnapshotImage := os.Getenv(useLocalPluginSnapshotEnvVarName) != "" && snapshotImagePresent
//TODO disabling oauth until we can update getAdminPassword path to handle oauth (perhaps borrow from oauth integration tests)
newAppArgs := []string{exutil.FixturePath("testdata", "jenkins-ephemeral-template-no-oauth.json")}
we can make ENABLE_OAUTH a parameter (with a default of true) in the
jenkins-ephemeral(and jenkins-persistent) official templates if we wanted
to do this, to make it easier.

True, but I thought when discussed in the past we liked the env var
apporach over template parameter for other reasons. Would this scenario be
sufficient to change are thoughts here?

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

bparees · 2016-11-01T19:26:40Z

True, but I thought when discussed in the past we liked the env var apporach over template parameter for other reasons. Would this scenario be sufficient to change are thoughts here?

@gabemontero it's still an env var to the container, it's just that now the env var value would be controllable by a template parameter instead of being hardcoded in the template.

gabemontero · 2016-11-01T19:49:19Z

On Tue, Nov 1, 2016 at 3:26 PM, Ben Parees [email protected] wrote:

True, but I thought when discussed in the past we liked the env var
apporach over template parameter for other reasons. Would this scenario be
sufficient to change are thoughts here?

@gabemontero https://github.com/gabemontero it's still an env var to
the container, it's just that now the env var value would be controllable
by a template parameter instead of being hardcoded in the template.

@bparees I get that, but I thought I remembered during our discussions a
perceived advantage to having the env var hardcoded in this particular
case. Essentially we didn't want to be
"outgoing" in allowing users to override oauth. But perhaps I'm
misremembering.

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

bparees · 2016-11-01T19:53:14Z

Essentially we didn't want to be "outgoing" in allowing users to override oauth. But perhaps I'm misremembering.

i think we may have felt it didn't need to be a parameter, but i can't think of a good reason not to allow it to be a parameter, aside from potential user confusion about whether they should be changing it.

if they set it to false, they are going to get admin/password behavior, which i guess is undesirable, but we can note that in the parameter description.

gabemontero · 2016-11-01T19:57:03Z

On Tue, Nov 1, 2016 at 3:53 PM, Ben Parees [email protected] wrote:

Essentially we didn't want to be "outgoing" in allowing users to override
oauth. But perhaps I'm misremembering.

i think we may have felt it didn't need to be a parameter, but i can't
think of a good reason not to allow it to be a parameter, aside from
potential user confusion about whether they should be changing it.

if they set it to false, they are going to get admin/password behavior,
which i guess is undesirable, but we can note that in the parameter
description.

OK then. I'll jump on board then. And we want @jupierce to make that
change as part of this PR ? If so, don't forget
jenkins-persistent-template.json as well.

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

bparees · 2016-11-01T19:58:49Z

OK then. I'll jump on board then. And we want @jupierce to make that
change as part of this PR ?

might as well, if we're consolidating templates let's go all the way.

jupierce · 2016-11-01T20:10:40Z

String constants used.
Removed no-oauth template.
Parameterized official jenkins ephemeral/persistent templates with ENABLE_OAUTH.
Changed test to use official ephemeral template.

@bparees @gabemontero ptal

gabemontero · 2016-11-01T20:16:51Z

IGTM

bparees

also needs hack/update-generated-bootstrap run since you're changing packaged templates.

bparees · 2016-11-01T20:19:20Z

test/extended/image_ecosystem/plugin.go

+			o.Expect(err).NotTo(o.HaveOccurred())
+
+			// Supplant the normal imagestream with the local imagestream using template parameters
+			newAppArgs = append(newAppArgs, "-p", fmt.Sprintf("NAMESPACE=%s,JENKINS_IMAGE_STREAM_TAG=%s:latest", oc.Namespace(), snapshotImageStream))


needs ENABLE_OAUTH=false
also split these parameters into multiple -p args, because we're disabling the comma separated key/value pairs behavior: #11539

sorry, i see you're handling OAUTH globally above. still need to use separate -p args instead of comma separated though.

ENABLE_OAUTH is set to false for all codepaths before this point (https://github.com/openshift/origin/pull/11711/files#diff-e4304911bb619db276427757be12a8c5R431) .
Comma change in next push.

bparees · 2016-11-01T20:27:18Z

still needs hack/update-generated-bootstrap.sh, otherwise lgtm.

jupierce · 2016-11-01T20:29:17Z

Commas no longer used for multiple template parameters.
Generated boostrap .
If dev defines USE_SNAPSHOT_JENKINS_IMAGE but does not have local jenkins snapshot, the test will fail.

bparees · 2016-11-01T20:31:05Z

lgtm

jupierce · 2016-11-02T17:27:24Z

Flake #11735
[test]

jupierce · 2016-11-02T17:27:27Z

[testextended][extended:core(openshift pipeline plugin)]

openshift-bot · 2016-11-02T17:29:42Z

Evaluated for origin test up to d2382ba

openshift-bot · 2016-11-02T17:29:52Z

Evaluated for origin testextended up to d2382ba

openshift-bot · 2016-11-02T18:54:06Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/725/) (Base Commit: 8e595f4) (Extended Tests: core(openshift pipeline plugin))

bparees · 2016-11-02T19:01:19Z

[merge]

openshift-bot · 2016-11-02T19:13:49Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11005/) (Base Commit: 8e595f4)

bparees · 2016-11-03T05:02:01Z

[merge]

Ben Parees | OpenShift

On Nov 3, 2016 12:53 AM, "OpenShift Bot" [email protected] wrote:

continuous-integration/openshift-jenkins/merge FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11041/) (Base
Commit: 75ed7e1
75ed7e1
)

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

bparees · 2016-11-03T12:41:37Z

flake #11650
[merge]

openshift-bot · 2016-11-03T12:45:27Z

Evaluated for origin merge up to d2382ba

openshift-bot · 2016-11-03T13:13:47Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11061/) (Base Commit: 3b6c372) (Image: devenv-rhel7_5306)

This was referenced Nov 1, 2016

Jenkins plugin using default serviceaccount instead of "jenkins" openshift/jenkins-plugin#103

Closed

Modifying PR Testing Process openshift/jenkins-plugin#104

Merged

gabemontero reviewed Nov 1, 2016

View reviewed changes

bparees requested changes Nov 1, 2016

View reviewed changes

bparees self-assigned this Nov 1, 2016

bparees requested changes Nov 1, 2016

View reviewed changes

Merging jenkins plugin test templates into one

d2382ba

gabemontero mentioned this pull request Nov 2, 2016

jenkins-plugin: properly handle cancelled jobs openshift/jenkins-plugin#106

Merged

openshift-bot merged commit 561b33b into openshift:master Nov 3, 2016

gabemontero mentioned this pull request Nov 3, 2016

Fix NPE when no arguments supplied on exec build step openshift/jenkins-plugin#107

Merged

Merging jenkins plugin test templates into one #11711

Merging jenkins plugin test templates into one #11711

Uh oh!

Conversation

jupierce commented Nov 1, 2016

Uh oh!

gabemontero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparees left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabemontero commented Nov 1, 2016

@bparees commented on this pull request.

Uh oh!

bparees commented Nov 1, 2016

Uh oh!

gabemontero commented Nov 1, 2016

Uh oh!

bparees commented Nov 1, 2016

Uh oh!

gabemontero commented Nov 1, 2016

Uh oh!

bparees commented Nov 1, 2016

Uh oh!

jupierce commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabemontero commented Nov 1, 2016

Uh oh!

bparees left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparees commented Nov 1, 2016

Uh oh!

jupierce commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bparees commented Nov 1, 2016

Uh oh!

jupierce commented Nov 2, 2016

Uh oh!

jupierce commented Nov 2, 2016

Uh oh!

openshift-bot commented Nov 2, 2016

Uh oh!

openshift-bot commented Nov 2, 2016

Uh oh!

openshift-bot commented Nov 2, 2016

Uh oh!

bparees commented Nov 2, 2016

Uh oh!

openshift-bot commented Nov 2, 2016

Uh oh!

bparees commented Nov 3, 2016

Uh oh!

bparees commented Nov 3, 2016

Uh oh!

openshift-bot commented Nov 3, 2016

Uh oh!

openshift-bot commented Nov 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jupierce commented Nov 1, 2016 •

edited

Loading

jupierce commented Nov 1, 2016 •

edited

Loading

openshift-bot commented Nov 3, 2016 •

edited

Loading