🌱 ClusterExtensionRevision conditions #2281
Conversation
Signed-off-by: Per G. da Silva <pegoncal@redhat.com>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/hold wip |
openshift-ci
bot
added
the
do-not-merge/hold
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2281 +/- ##
==========================================
+ Coverage 70.32% 70.38% +0.06%
==========================================
Files 90 90
Lines 8794 8813 +19
==========================================
+ Hits 6184 6203 +19
- Misses 2196 2197 +1
+ Partials 414 413 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| if err := c.establishWatch(ctx, rev, revision); err != nil { | ||
| inRollout := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable) == nil |
There was a problem hiding this comment.
I worry about looking at the status to drive the reconciliation logic here. We should probably look at the state of the objects themselves to make this determination, no? Or, is this just temporary and in lieu of the new boxcutter PR that should surface this state more easily?
| if err := c.establishWatch(ctx, rev, revision); err != nil { | ||
| inRollout := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable) == nil | ||
| if inRollout { | ||
| if err := c.establishWatch(ctx, rev, revision); err != nil { |
There was a problem hiding this comment.
should establishWatch just be idempotent and we call it whether were rolling out or not?
…ble|Unavailable) helper methods for improved code readability
| }) | ||
| if inRollout { | ||
| // given that we retry, we are going to keep Progressing condition True | ||
| rev.MarkAsProgressing(ocv1.ClusterExtensionRevisionReasonPhaseValidationError, fmt.Sprintf("phase %d validation error: %s", i, verr)) |
There was a problem hiding this comment.
nit: I wonder if we should just use a "Retrying" reason rather than PhaseValidationError/RevisionValidationFailure/etc.".
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
perdasilva
force-pushed
the
revision-conditions
branch
from
8a1a5d4 to
496d418
Compare
…rator Start busybox's httpd and serves probes from created files
pedjak
force-pushed
the
revision-conditions
branch
from
496d418 to
8c97bd7
Compare
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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-sigs/prow repository. |
3 participants
Description
Reviewer Checklist