Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Refactors how default progression probes are applied by moving the built-in/default probe set out of the ClusterExtensionRevision controller and into the applier-generated ClusterExtensionRevision spec, and extends the ProgressionProbes API with an observedGeneration gating option.
Changes:
- Add
observedGenerationtoProgressionProbe(API/types + applyconfigurations + CRD schemas) and wire it into probe building. - Generate/stamp default progression probes in the applier when constructing
ClusterExtensionRevisionobjects (and update unit expectations). - Update E2E feature specs to explicitly include progression probes when applying
ClusterExtensionRevisiondirectly.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/features/revision.feature | Adds explicit progressionProbes to CER YAML used in E2E scenarios. |
| manifests/experimental.yaml | Adds CRD schema for progressionProbes[].observedGeneration. |
| manifests/experimental-e2e.yaml | Same CRD schema update for E2E manifests. |
| helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml | Helm-packaged CRD schema updated with observedGeneration. |
| api/v1/clusterextensionrevision_types.go | Adds ObservedGeneration to ProgressionProbe and renames Go constants for probe type enums. |
| api/v1/zz_generated.deepcopy.go | Adds deepcopy support for ObservedGeneration *bool. |
| applyconfigurations/api/v1/progressionprobe.go | Adds ObservedGeneration field + WithObservedGeneration() builder. |
| internal/operator-controller/controllers/clusterextensionrevision_controller.go | Stops injecting controller-side default probes; builds observedGeneration-wrapped probes from CER spec. |
| internal/operator-controller/applier/boxcutter.go | Stamps default probes onto generated CER specs via defaultProgressionProbes(). |
| internal/operator-controller/applier/boxcutter_test.go | Updates expected generated CER spec to include default probes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextensionrevision_controller.go
Show resolved
Hide resolved
| var prober probing.Prober | ||
| if progressionProbe.ObservedGeneration != nil && *progressionProbe.ObservedGeneration { | ||
| // Wrap the and-ed probes within an ObservedGeneration probe | ||
| prober = &probing.ObservedGenerationProbe{Prober: assertions} | ||
| } else { | ||
| prober = &assertions | ||
| } |
There was a problem hiding this comment.
The new ObservedGeneration flag changes how progression probes are built (wrapping assertions in an ObservedGenerationProbe when enabled), but there doesn't appear to be unit test coverage asserting this behavior. Adding a focused test for buildProgressionProbes covering observedGeneration=true/false would help prevent regressions (e.g., verifying the resulting selector probe contains/doesn't contain an ObservedGenerationProbe wrapper).
dtfranz
force-pushed
the
default-probes-cer
branch
from
94d5d71 to
8eb532b
Compare
dtfranz
force-pushed
the
default-probes-cer
branch
from
8eb532b to
4457677
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2586 +/- ##
==========================================
+ Coverage 65.84% 68.69% +2.84%
==========================================
Files 137 137
Lines 9560 9663 +103
==========================================
+ Hits 6295 6638 +343
+ Misses 2795 2524 -271
- Partials 470 501 +31
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:
|
Migrates the default progression probes out of the CER controller into the boxcutter applier in order to use the ProgressionProbes API to transparently stamp out the checks. Also adds flag to the API to allow checking status.observedGeneration==metadata.generation before executing probes. Signed-off-by: Daniel Franz <dfranz@redhat.com>
dtfranz
force-pushed
the
default-probes-cer
branch
from
4457677 to
55b3524
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 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
Migrates the default progression probes out of the CER controller into the boxcutter applier in order to use the ProgressionProbes API to transparently stamp out the checks. Also adds flag to the API to allow checking status.observedGeneration==metadata.generation before executing probes.
Description
Reviewer Checklist