Fix imagestream bugs #20666

coreydaley · 2018-08-16T06:01:29Z

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1612723
https://bugzilla.redhat.com/show_bug.cgi?id=1611462

coreydaley · 2018-08-16T06:01:46Z

/assign bparees

coreydaley · 2018-08-16T14:02:48Z

/retest

bparees · 2018-08-16T14:06:19Z

pkg/oc/lib/newapp/cmd/newapp.go

+	sort.Sort(sort.Reverse(sort.IntSlice(tagsToRemove)))
+	for _, objIndex := range tagsToRemove {
+		objects = append(objects[:objIndex], objects[objIndex+1:]...)
+	}


this looks overly cute (and is probably incorrect. each time you change objects, objIndex becomes invalid for the next reference). Just iterate the original list and copy the ones that aren't in "tagsToRemove" into a new list.

updated in current push

bparees · 2018-08-16T14:07:24Z

pkg/oc/lib/newapp/app/imageref.go

 	}
+	if len(r.ObjectName) > 0 {
+		return ""
+	}


updated in current push

still here, still not clear why?

regressed, will be removed in next push

bparees · 2018-08-16T15:30:16Z

pkg/oc/lib/newapp/cmd/newapp.go

+
+	objectsToKeep := app.Objects{}
+	for i, obj := range objects {
+		if _, ok := objectsToRemove[i]; ok {


why not just key objectsToRemove by the "namespace/name" value?

(that would seem less fragile to me)

coreydaley · 2018-08-16T16:16:16Z

@bparees ptal, comments have been addressed

coreydaley · 2018-08-16T16:48:29Z

/retest

bparees · 2018-08-16T17:10:39Z

pkg/oc/lib/newapp/app/imageref.go

 		ObjectMeta: metav1.ObjectMeta{
 			Name:        istname,
 			Annotations: map[string]string{"openshift.io/imported-from": r.Reference.Exact()},
-			Namespace:   r.SuggestNamespace(),


why is this no longer needed? it seems like a useful disambiguation.

setting namespace in another function to c.OriginNamespace if one doesn't exist

bparees · 2018-08-16T17:11:08Z

pkg/oc/lib/newapp/cmd/newapp.go

+				istNamespace = c.OriginNamespace
+			}
+			streamName, tagName, ok := imageapi.SplitImageStreamTag(ist.Name)
+			if ok {


needs to do something(throw an error?) if !ok

updated in next push

bparees · 2018-08-16T17:12:45Z

pkg/oc/lib/newapp/cmd/newapp.go

+		if ist, ok := obj.(*imageapi.ImageStreamTag); ok {
+			istNamespace := ist.Namespace
+			if len(istNamespace) == 0 {
+				istNamespace = c.OriginNamespace


i guess this is why/how you got rid of the suggestnamespace logic.

bparees · 2018-08-16T17:13:51Z

test/cmd/newapp.sh

 os::cmd::expect_failure 'oc get dc/php'
 os::cmd::expect_success_and_text 'oc new-app -f test/testdata/template-without-app-label.json -o yaml' 'app: ruby-helloworld-sample'

+# remove duplicate imagestream tags before creating objects


how is this testing that?

updated in current push

bparees · 2018-08-16T17:30:35Z

existing imagestreams/tags:

openshift/php:5.6
myproject/perl:1.0

scenarios:

# set project to myproject
oc project myproject

# should fail (openshift/php:latest isn't a valid tag in the openshift/php imagestream)
oc new-app --image-stream=openshift/php somerepo 

# should create a myproject/php imagestream with a "latest" tag
oc new-app --docker-image=library/php somerepo 

# should create a myproject/perl:latest tag in the myproject/perl imagestream
oc new-app --docker-image=library/perl somerepo

that's the sort of scenarios i'd expect to see test coverage for (and anything else you've stumbled across while working on this). i'd expect them to be well defined and more explicit than what you currently have.

coreydaley · 2018-08-19T03:59:14Z

@bparees ptal, updates tests, hopefully that is more what you are looking for.

bparees · 2018-08-16T17:24:04Z

test/cmd/newapp.sh

+os::cmd::expect_success_and_text 'oc new-app  openshift/ruby-22-centos7 https://github.com/openshift/ruby-hello-world  --strategy=docker --loglevel=5' 'Removing duplicate tag from object list: myproject/ruby-22-centos7:latest'
+os::cmd::expect_success 'oc delete all -l app=ruby-22-centos7'
+
+# create imagestreams in the correct namespace


why would this invocation create an imagestream at all?

bparees · 2018-08-19T04:34:15Z

test/cmd/newapp.sh

+# should succeed and create the php:latest tag in the current namespace
+os::cmd::expect_success 'oc new-app --docker-image=library/php https://github.com/sclorg/cakephp-ex --strategy=source'
+os::cmd::try_until_success 'oc get imagestreamtag php:latest -n test-imagestreams'
+os::cmd::expect_success 'oc create imagestreamtag php:latest -n openshift'


this is valid? what does it do? (what does php:latest point to?)

fixed in next push

bparees · 2018-08-19T04:34:38Z

test/cmd/newapp.sh

+os::cmd::expect_success 'oc create imagestreamtag php:latest -n openshift'
+
+# create a new tag for an existing imagestream in the current namespace
+os::cmd::expect_success 'oc create imagestreamtag perl:5.0'


same question here

fixed in next push

bparees · 2018-08-19T04:36:45Z

test/cmd/newapp.sh

+os::cmd::try_until_success 'oc get imagestreamtag perl:latest -n test-imagestreams'
+
+# remove redundant imagestream tag before creating objects
+os::cmd::expect_success_and_text 'oc new-app  openshift/ruby-22-centos7 https://github.com/openshift/ruby-hello-world  --strategy=docker --loglevel=5' 'Removing duplicate tag from object list'


what redundant tag is being removed? I would expect this invocation to create a new imagetream named "ruby-22-centos7" with a tag named "latest" pointing to the image "docker.io/openshift/ruby-22-centos7:latest", in the current namespace.

Based on bugzilla https://bugzilla.redhat.com/show_bug.cgi?id=1612723

Command output: Removing duplicate tag from object list: myproject/ruby-22-centos7:latest

ah, this was the case where the processing creates both the imagestreamtag and the imagestream containing the tag?

bparees · 2018-08-19T04:38:41Z

test/cmd/newapp.sh

+
+# create imagestream in the correct namespace
+os::cmd::expect_success 'oc new-app --name=mytest --image-stream=mysql --env=MYSQL_USER=test --env=MYSQL_PASSWORD=redhat --env=MYSQL_DATABASE=testdb -l app=mytest'
+os::cmd::try_until_success 'oc get is mytest -n test-imagestreams'


why would this create an imagestream? you asked new-app to create an app based on an existing imagestream (named mysql, located in either this namespace or the openshift namespace). I wouldn't expect it to ever create an imagestream though (you haven't pointed to an image, so what image would the imagestream reference?)

I have based this test off of this bugzilla https://bugzilla.redhat.com/show_bug.cgi?id=1611462
This command was trying to create the imagestream in the openshift namespace, now it creates it in the users namespace (and it does create an imagestream)

hm. it's odd to me that new-app creates a new imagestream in this scenario (rather than pointing the openshift/mysql imagestream) but i guess it's one of the many mysteries of new-app.

bparees · 2018-08-19T04:40:55Z

test/cmd/newapp.sh

+
+# don't create an unnecessary imagestream
+os::cmd::expect_success 'oc new-app https://github.com/sclorg/nodejs-ex'
+os::cmd::expect_failure_and_text 'oc get is nodejs:10 -n test-imagestreams' 'not found'


the imagestream name (if created) would be nodejs, nodejs:10 is an istag name. actually it might be "nodejs-ex" too.

updated in next push

bparees · 2018-08-19T04:42:13Z

test/cmd/newapp.sh

+os::cmd::expect_success 'oc project cmd-newapp'
+os::cmd::expect_success 'oc delete project test-imagestreams'
+
+# end imagestream/tag creation and reuse


where's the test that ensures that if you've already got a "nodejs" imagestream in the current namespace, but it doesn't contain a "nodejs:10" tag, the "nodejs:10" tag is added to the existing "nodejs" imagestream and you don't get an error trying to create a new "nodejs" imagestream?

restored in next push

bparees · 2018-08-19T04:43:11Z

test/cmd/newapp.sh

-os::cmd::expect_success "oc new-build -D $'FROM node:8\nRUN echo \"Test\"' --name=node8"
-os::cmd::try_until_success 'oc get imagestreamtags node:8'
-os::cmd::expect_success "oc new-build -D $'FROM node:10\nRUN echo \"Test\"' --name=node10"
-os::cmd::try_until_success 'oc get imagestreamtags node:10'


per my comment above, i think these 4 lines should be retained/restored. I don't think this behavior is covered/tested in what you have above.

restored in next push

coreydaley · 2018-08-19T16:58:48Z

@bparees updated per your comments, also responded to your comments about specific sections and why they are there

coreydaley · 2018-08-19T20:25:23Z

/retest

coreydaley · 2018-08-19T20:25:48Z

/refresh

coreydaley · 2018-08-19T22:34:33Z

/retest

bparees

one more question/comment and then i think this will be good to go

bparees · 2018-08-20T03:27:34Z

test/cmd/newapp.sh

+
+# don't create an unnecessary imagestream
+os::cmd::expect_success 'oc new-app https://github.com/sclorg/nodejs-ex'
+os::cmd::expect_failure_and_text 'oc get istag node:10 -n test-imagestreams' 'not found'


should this be "nodejs:10"? "nodejs" is the name of the imagestream in openshift, at least.

also should probably just look for the "nodejs" imagestream (the imagestream shouldn't be created at all, right?), not a specific tag.

fixed in current push

coreydaley · 2018-08-20T12:42:32Z

@bparees updated per your comment

coreydaley · 2018-08-20T12:51:08Z

/refresh

coreydaley · 2018-08-20T12:51:17Z

/retest

coreydaley · 2018-08-20T17:27:12Z

@bparees ptal

bparees · 2018-08-20T17:34:02Z

/lgtm

openshift-ci-robot · 2018-08-20T17:34:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, coreydaley

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/oc/lib/newapp/OWNERS~~ [bparees]
~~test/cmd/OWNERS~~ [bparees]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

juanvallejo · 2018-08-20T19:09:17Z

@bparees @coreydaley any chance we could have this PR wait for #20645? It definitely overlaps with this one and I'd like to avoid having to rebase that if at all possible

juanvallejo · 2018-08-20T19:12:35Z

/hold

Would like to wait for a response to above comment

bparees · 2018-08-20T19:24:33Z

@juanvallejo this fixes two bugs for QE, how soon will your PR go in?

juanvallejo · 2018-08-20T19:28:39Z

@bparees

this fixes two bugs for QE, how soon will your PR go in?

Just need to get tests passing for it, will try to have it mergeable by end of day today. If I encounter any problems between now and end of day that might delay it from merging any longer, I'll just go ahead and unhold this one and take the rebase on mine

bparees · 2018-08-20T19:31:51Z

ok

…

On Mon, Aug 20, 2018 at 3:28 PM, Juan Vallejo ***@***.***> wrote: @bparees <https://github.com/bparees> this fixes two bugs for QE, how soon will your PR go in? Just need to get tests passing for it, will try to have it mergeable by end of day today. If I encounter any problems between now and end of day that might delay it from merging any longer, I'll just go ahead and unhold this one and take the rebase on mine — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20666 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEvl3gRAubrsMMDLCSOOtMulP6rpmUKqks5uSw3zgaJpZM4V_Pzh> .

-- Ben Parees | OpenShift

juanvallejo · 2018-08-20T21:40:13Z

/hold cancel

Will keep working on my PR, will rebase since this will most likely merge first

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 16, 2018

openshift-ci-robot assigned bparees Aug 16, 2018

openshift-ci-robot requested review from bparees and mfojtik August 16, 2018 06:01

bparees reviewed Aug 16, 2018

View reviewed changes

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 19, 2018

bparees reviewed Aug 19, 2018

View reviewed changes

bparees requested changes Aug 20, 2018

View reviewed changes

coreydaley changed the title ~~[WIP] Fix imagestream bugs~~ Fix imagestream bugs Aug 20, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 20, 2018

Fixing a few imagestream bugs

cef2c66

bparees approved these changes Aug 20, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2018

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2018

openshift-merge-robot merged commit a166e3e into openshift:master Aug 20, 2018

Fix imagestream bugs #20666

Fix imagestream bugs #20666

Uh oh!

Conversation

coreydaley commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coreydaley commented Aug 16, 2018

Uh oh!

coreydaley commented Aug 16, 2018

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!

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!

Choose a reason for hiding this comment

Uh oh!

coreydaley commented Aug 16, 2018

Uh oh!

coreydaley commented Aug 16, 2018

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!

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!

Choose a reason for hiding this comment

Uh oh!

bparees commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coreydaley commented Aug 19, 2018

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!

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!

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!

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!

coreydaley commented Aug 16, 2018 •

edited

Loading

bparees commented Aug 16, 2018 •

edited

Loading