Repository navigation
⚠️ Rename ClusterObjectSet Ready reason to AllObjectsReady; decouple ClusterExtension Available - #2980
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughCompleted ClusterObjectSet revisions now report Ready with reason ChangesCondition reason updates
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Merge Risk: ⚪ Minimal · up to The new Ready reason and separate Available reason are consistent with the documented behavior. No actionable issue remains before merge. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The reviewed changes preserve readiness gates and status lifecycle behavior while separating the two resources’ success vocabulary. No material security risk was found. The experimental ClusterObjectSet reason and exported constant change, so consumers that depend on those names may need updates. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 9 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f8c2307 to
ebfdc80
Compare
|
ok to override apidiff - all changes are to the experimental set |
|
/lgtm |
perdasilva
changed the title
| const availableTrueMessage = "Objects are available and pass all probes." | ||
|
|
||
| // availableFromReady maps a ClusterObjectSet Ready condition onto a ClusterExtension Available condition. | ||
| // Non-ready states are surfaced as-is (only the Type is rewritten to Available). The ready (True) state is |
There was a problem hiding this comment.
Is this the long-term path? Or should ClusterExtension itself have its own contract of condition types and reasons, separate from the underlying COD/COS?
It seems like it should have a contract one way or the other, even if that contract is explicitly "matches COD/COS exactly as of 6 Oct 2026". That way we don't ever accidentally leak something into the CE API without an explicit decision about it.
There was a problem hiding this comment.
I'm definitely of the camp that that CE should have its own set of exposed condition types and reasons. I think we should either address this in parallel or in tandem with the ClusterObjectDeployment integration.
|
At one point, weren't we concerned about the implications of the I'm not personally all that concerned about that distinction, but just wanted to raise the question since it had come up before. |
…lusterExtension Available
Rename the ClusterObjectSet Ready condition's success reason from
ProbesSucceeded to AllObjectsReady, with an updated message ("All objects
are at the desired state"). The reason now describes the observed outcome
rather than the probing mechanism.
Decouple the ClusterExtension Available condition from the revision-level
Ready condition: availableFromReady() re-emits the ready (True) state with
a ClusterExtension-owned reason (ReasonProbesSucceeded) and a stable
message, so rewording the ClusterObjectSet Ready condition no longer
alters the ClusterExtension API surface.
Update CRD docs, apply configurations, manifests, and tests accordingly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ebfdc80 to
3a90769
Compare
Nico had an argument that
So, I'm sort of leaning away from probe success being the signal that we want in favor of something like On a tangent, I also think we should create a couple of follow-up tickets for the next release cycle to think about events and controller metrics. We need to divorce the .status from controller state and keep it at object (level) state. |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fao89, joelanford The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
d1b1337
into
operator-framework:main
4 participants
Description
Refines the
ClusterObjectSetReadycondition vocabulary and decouples theClusterExtensionAvailablecondition from it.Changes
Readysuccess reasonProbesSucceeded→AllObjectsReady, with an updated message ("All objects are at the desired state"). The reason now describes the observed outcome rather than the probing mechanism.ClusterExtensionAvailablecondition from the revision-levelReadycondition. A newavailableFromReady()helper re-emits the ready (True) state with aClusterExtension-owned reason (ReasonProbesSucceeded) and a stable message, so rewording theClusterObjectSetReadycondition no longer alters theClusterExtensionAPI surface. Non-ready states continue to be surfaced as-is (only theTypeis rewritten toAvailable).Notes
ClusterObjectSet) API surface only; the standardClusterExtensionAvailablereason/message is intentionally held stable.🤖 Generated with Claude Code
Summary by CodeRabbit
ClusterObjectSetrollouts now report Ready with the reasonAllObjectsReadyand the message “All objects are at the desired state.”Availablecondition continues to useProbesSucceededwhen availability checks succeed, keeping rollout readiness and availability reporting distinct.