deploy: allow to trigger deployment when ICT is updated #13886
Conversation
|
[test] |
| // comparison. | ||
| if config.Status.LatestVersion > 0 && !triggeredByDifferentImage(*t.ImageChangeParams, *decoded) { | ||
| // comparison. Also configs with new/updated triggers should always trigger. | ||
| if config.Status.LatestVersion == 0 || hasUpdatedTriggers(*config, *decoded) || triggeredByDifferentImage(*t.ImageChangeParams, *decoded) { |
There was a problem hiding this comment.
@Kargakis this logic need check, it makes my brain hurt.
|
@Kargakis I want to add extended test proving this works as expected. |
|
I think we also need to make the generictrigger controller enqueue DCs when there is an update in the triggers of the DC. |
|
@Kargakis +1 |
|
[test] flake: #11114 |
|
@Kargakis added extended test to prove this works, PTAL |
| err = wait.PollImmediate(500*time.Millisecond, 10*time.Second, func() (bool, error) { | ||
| dc, err = oc.Client().DeploymentConfigs(oc.Namespace()).Get(name) | ||
| if err != nil { | ||
| return false, err |
There was a problem hiding this comment.
Log this error and return nil
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| expectLatestVersion := func(version int) { | ||
| dc, err := oc.Client().DeploymentConfigs(oc.Namespace()).Get(name) |
| expectLatestVersion(1) | ||
|
|
||
| g.By("updating the image change trigger to point to test:v2 image") | ||
| { |
Shouldn't we also fix this here? |
will prefer separate PR to unblock customer first. |
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| _, err = oc.Run("set").Args("triggers", "dc/"+name, "--from-image", "test:v2", "--auto", "-c", "test").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(waitForSyncedConfig(oc, name, deploymentRunTimeout)).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
We should switch waitForSyncedConfig to accept a generation so you can pass the generation you get from the last set triggers update, for this to be strictly correct.
In the test, you are using |
|
@Kargakis i would say that you need a ConfigChange trigger to trigger when DC is updated? |
|
@Kargakis fixed, thanks! |
|
flake: #13650 |
|
LGTM for unblocking the customer, let's follow-up with fixing the case where the image already exists |
|
[merge] |
|
[merge] |
|
dnf flake [test] |
|
[test] dnf flake again |
|
Evaluated for origin test up to ca25116 |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1033/) (Base Commit: cd4661d) |
|
flake: #13942 |
|
[merge] |
|
flake: #13943 |
|
[merge] |
|
@mfojtik did you create any issue for the follow-up work? |
|
Evaluated for origin merge up to ca25116 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/530/) (Base Commit: df6ad91) (Image: devenv-rhel7_6194) |
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1444545
Basically detect if there are any new triggers in the current DC (comparing to previous version). If yes, then don't prevent the controller from triggering.