deploy: reconcile streams on config creation/updates #10456
Conversation
|
[test] |
| return nil, getErr | ||
| } | ||
| if !exists { | ||
| existsErr := kapierrors.NewNotFound(imageapi.Resource("imagestreams"), name) |
There was a problem hiding this comment.
squash these two lines to one
|
This looks pretty cool and cleaner than what we have now. @bparees you might put this on your radar for image change trigger in builds. |
| @@ -14,6 +17,19 @@ type StoreToImageStreamLister struct { | |||
| cache.Indexer | |||
| } | |||
|
|
|||
| // Get returns the image stream with the provided namespace and name. | |||
| func (s *StoreToImageStreamLister) Get(namespace, name string) (*imageapi.ImageStream, error) { | |||
There was a problem hiding this comment.
Refactor to match client interface. .ImageStreams(namespace).Get(name). I'm not ready to break that pattern yet.
|
I'm missing something fundamental. Why would an update to a DC cause our image change trigger to fire? |
| return false | ||
| } | ||
|
|
||
| func (c *ImageChangeController) getByKey(key string) (*imageapi.ImageStream, error) { |
There was a problem hiding this comment.
I'd rather see a lister in use.
There was a problem hiding this comment.
I would have to parse the key to a namespace and a name in order to use the stream lister.
You just added a new ICT to your DC. If the image stream exists, it should be reconciled for the image to be picked up by your DC, no? Remember that we are going to remove the controller in 1.3.1 in favor of /instantiate. |
… interval" This reverts commit cc7f896.
That makes sense. Can you add that as a comment in the code? I didn't get there just reading it. |
|
One comment to add, might have one //TODO outstanding, one comment to add, and a squash. Then lgtm. |
| @@ -24,7 +24,11 @@ func ImageStreamReferenceIndexFunc(obj interface{}) ([]string, error) { | |||
| } | |||
| params := trigger.ImageChangeParams | |||
| name, _, _ := imageapi.SplitImageStreamTag(params.From.Name) | |||
| keys = append(keys, params.From.Namespace+"/"+name) | |||
| namespace := params.From.Namespace | |||
There was a problem hiding this comment.
@deads2k last thing that needs review. On master we currently default empty namespaces for ICT references to the config namespace but this is not true for older versions so our index needs to take empty namespace into account and use the config ns.
There was a problem hiding this comment.
Shouldn't that happen in defaulting? If so, it'll happen client-side in the controller if a namespaceless ICT is read from the API, right?
There was a problem hiding this comment.
Correct but what worries me is that this is a client library and older servers are not defaulting.
There was a problem hiding this comment.
anything containing this code will be reading using a client library that does the defaulting, right?
There was a problem hiding this comment.
Correct. Then I guess we don't need this change.
|
All comments addressed, green tests, [merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8065/) (Image: devenv-rhel7_4857) |
|
Evaluated for origin merge up to 7ce9013 |
|
Evaluated for origin test up to 7ce9013 |
|
@smarterclayton this should decrease the number of flakes in deployments \o/ |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8065/) |
Fixes #9018
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1355976
@mfojtik @deads2k @liggitt