InstallPlan wait for condition update #2510
Conversation
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @akihikokuroda. Thanks for your PR. I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
akihikokuroda
changed the title
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
akihikokuroda
force-pushed
the
fix2509
branch
from
0da9a45
to
f50d78c
Compare
There was a problem hiding this comment.
Thanks for continuing to dish these flake/cleanup PRs out - I think these changes are reasonable. I just had one quick comment about simplifying the Eventually block. Any idea on how we could reproduce this more frequently to gauge whether these fixes will remove this flake?
|
@akihikokuroda I'm going to recycle the checks for this PR as well as we may have regressed in CI health in the past couple of weeks and I want to double check whether these changes pop up as failures in the e2e-tests workflow. |
akihikokuroda
force-pushed
the
fix2509
branch
from
f50d78c
to
0c1b2ea
Compare
|
/lgtm |
|
The unit test failure should be fixed by this: #2521 |
akihikokuroda
force-pushed
the
fix2509
branch
from
0c1b2ea
to
d8681e0
Compare
akihikokuroda
force-pushed
the
fix2509
branch
from
d8681e0
to
dd80477
Compare
test/e2e/installplan_e2e_test.go
Outdated
| return false | ||
| } | ||
| return hasCondition(fetchedInstallPlan, cond) | ||
| }, 1*time.Minute, interval).Should(BeTrue()) |
There was a problem hiding this comment.
Non-blocking nit: the minute timeout seems a bit arbitrary.
There was a problem hiding this comment.
Yes, I feel a minute is a little short now. I'll change it to 5.
| // first check that a condition with a message exists | ||
| fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseInstalling)) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(fetchedInstallPlan).NotTo(BeNil()) |
There was a problem hiding this comment.
Based on this return statement within the fetchInstallPlanWithNamespace function, it is possible that the fetchedInstallPlan will be nil if the Get call from the client ever returns nil, nil.
I can't think of a time where this could happen, but we should probably safeguard against it.
There was a problem hiding this comment.
Yes, that's right. I'll add nil check for fetchedInstallPaln. Thanks!
Signed-off-by: akihikokuroda <akihikokuroda2020@gmail.com>
akihikokuroda
force-pushed
the
fix2509
branch
from
dd80477
to
5eefbbf
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akihikokuroda, awgreene The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| return false | ||
| } | ||
| return hasCondition(fetchedInstallPlan, cond) | ||
| }, 5*time.Minute, interval).Should(BeTrue()) |
There was a problem hiding this comment.
nit: technically we can drop 5*time.Minute call as it's the global default (and interval might also be the global default but I'm a bit fuzzier on that).
|
/lgtm |
|
Force merging to try and clear out as many e2e flakes (that don't touch any runtime code) as much as possible. |
Signed-off-by: akihikokuroda akihikokuroda2020@gmail.com
Description of the change:
There are a small gap between the phase update and the condition update of the InstallPlan. This change makes sure the correct condition is put into the installplan at the beginning of the test.
Motivation for the change:
Closes #2509
Reviewer Checklist
/doc