Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ 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
Adds support for user-defined spec.progressionProbes on ClusterExtensionRevision to gate phase progression based on custom assertions against rendered objects, with accompanying CRD/applyconfig updates and new e2e scenarios.
Changes:
- Extend
ClusterExtensionRevisionSpecwithprogressionProbesAPI types + deepcopy/applyconfig support. - Add CRD schema for
progressionProbesto experimental manifests/Helm CRD. - Update the controller to build and attach user-defined progression probes to the boxcutter progress probe chain; add e2e scenarios for pass/fail behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/features/revision.feature | Adds e2e scenarios covering progression probe pass/fail behavior (currently tagged @WIP). |
| manifests/experimental.yaml | Updates experimental CRD schema to include spec.progressionProbes. |
| manifests/experimental-e2e.yaml | Updates e2e experimental CRD schema to include spec.progressionProbes. |
| internal/operator-controller/controllers/clusterextensionrevision_controller.go | Builds user probes from spec.progressionProbes and adds them to the progress probe chain. |
| helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml | Updates Helm CRD schema with spec.progressionProbes. |
| applyconfigurations/utils.go | Registers apply-configuration kinds for the new embedded types. |
| applyconfigurations/api/v1/progressionprobe.go | Generated apply-configuration for ProgressionProbe. |
| applyconfigurations/api/v1/probeselector.go | Generated apply-configuration for ProbeSelector. |
| applyconfigurations/api/v1/probeassertion.go | Generated apply-configuration for ProbeAssertion. |
| applyconfigurations/api/v1/fieldvalueprobe.go | Generated apply-configuration for FieldValueProbe. |
| applyconfigurations/api/v1/clusterextensionrevisionspec.go | Adds apply-configuration support for spec.progressionProbes. |
| api/v1/zz_generated.deepcopy.go | Adds deepcopy support for the new API structs/fields. |
| api/v1/clusterextensionrevision_types.go | Introduces the new API types and spec.progressionProbes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextensionrevision_controller.go
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2550 +/- ##
==========================================
- Coverage 68.60% 67.84% -0.77%
==========================================
Files 131 137 +6
Lines 9330 9534 +204
==========================================
+ Hits 6401 6468 +67
- Misses 2438 2571 +133
- Partials 491 495 +4
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:
|
dtfranz
force-pushed
the
fieldvalue-user-probes
branch
from
3e0efb5 to
c1ddbb3
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Provides an API which allows custom probe definitions to determine readiness of the CER phases. Objects can be selected for in one of two ways: by GroupKind, or by Label (matchLabels and matchExpressions). They can then be tested via any of: Condition, FieldsEqual, and FieldValue. Condition checks that the object has a condition matching the type and status provided. FieldsEqual uses two provided field paths and checks for equality. FieldValue uses a provided field path and checks that the value is equal to the provided expected value. Signed-off-by: Daniel Franz <dfranz@redhat.com>
dtfranz
force-pushed
the
fieldvalue-user-probes
branch
from
c1ddbb3 to
d1436fd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expectedResult: probing.Result{ | ||
| Status: probing.StatusFalse, | ||
| Messages: []string{ | ||
| `missing key: "spec.foo"`, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The expected message for the missing-field case doesn’t match the actual probe output. FieldValueProbe returns missing key: "spec.foo", but this test asserts "spec.foo" missing, so the test will fail. Update the expected message (or align the probe’s message format) so they match.
| - selector: | ||
| type: GroupKind | ||
| groupKind: | ||
| group: core | ||
| kind: ConfigMap |
There was a problem hiding this comment.
For core ("" apiGroup) resources like ConfigMap, the GroupKind selector’s group should be an empty string (or omitted if allowed), not core. As written, this selector likely won’t match any core resources, so the probe won’t be exercised (and may cause the scenario to behave unexpectedly).
| - selector: | ||
| type: GroupKind | ||
| groupKind: | ||
| group: core | ||
| kind: Service |
There was a problem hiding this comment.
This GroupKind selector also uses group: core, which won’t match Kubernetes core resources (whose group is the empty string). Use group: "" here as well so the selector actually targets the intended objects.
| - selector: | ||
| type: GroupKind | ||
| groupKind: | ||
| group: core | ||
| kind: Service | ||
| assertions: | ||
| - type: FieldsEqual | ||
| fieldsEqual: | ||
| fieldA: "metadata.labels.foo" | ||
| fieldB: "metadata.labels.bar" |
There was a problem hiding this comment.
This probe is configured to select Kind Service, but the scenario doesn’t create any Service objects (it creates a ServiceAccount in phase2). As a result, the FieldsEqual probe won’t be applied to anything, so the scenario won’t validate the intended behavior. Either change the selector kind to ServiceAccount or add a matching Service object to the phases.
| - assertions | ||
| - selector | ||
| type: object | ||
| maxItems: 50 |
There was a problem hiding this comment.
The CRD schema here allows spec.progressionProbes up to 50 items, but the Go type annotation sets +kubebuilder:validation:MaxItems=20 for the same field (api/v1/clusterextensionrevision_types.go). This mismatch suggests the CRD YAML may be out of sync with the API types/controller-gen output; align the limits (and regenerate the CRDs) to avoid drift.
| maxItems: 50 | |
| maxItems: 20 |
| - assertions | ||
| - selector | ||
| type: object | ||
| maxItems: 50 |
There was a problem hiding this comment.
Same schema drift issue as manifests/experimental.yaml: spec.progressionProbes is set to maxItems: 50 here, but the Go API type uses +kubebuilder:validation:MaxItems=20. Please align the limits and regenerate the CRD YAML so it matches the source types.
| maxItems: 50 | |
| maxItems: 20 |
| - assertions | ||
| - selector | ||
| type: object | ||
| maxItems: 50 |
There was a problem hiding this comment.
Same schema drift issue: this CRD YAML sets spec.progressionProbes to maxItems: 50, while the Go type annotation sets MaxItems=20. Please make the Helm CRD, manifests, and Go types consistent (ideally by regenerating the CRD YAML from the API types).
| maxItems: 50 | |
| maxItems: 20 |
Description
WIP
Adds progressionProbes to ClusterExtensionRevision API
Needs:
Reviewer Checklist