Open
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a RevisionController and revision-generation pipeline, extracts common command options to a new package, introduces structured controller reconcile results and condition merging, extends provider image manifest handling, and adds extensive tests and test-matchers across the codebase. Changes
Sequence DiagramsequenceDiagram
actor Manager as Controller Manager
participant RC as RevisionController
participant Infra as Infrastructure API
participant CA as ClusterAPI (ClusterAPI singleton)
participant RG as RevisionGenerator
participant PIM as ProviderImageManifests
Manager->>RC: Reconcile(ctx)
activate RC
RC->>Infra: Get Infrastructure
Infra-->>RC: Infrastructure (+PlatformStatus)
RC->>PIM: Read provider manifest files
PIM-->>RC: ProviderImageManifests (profiles)
RC->>RG: NewRenderedRevision(profiles)
RG-->>RC: RenderedRevision (ContentID, Components)
RC->>CA: Get ClusterAPI singleton
CA-->>RC: ClusterAPI (status.revisions)
RC->>RC: mergeRevisions(desired)
RC->>RG: ForInstall(releaseVersion, nextIndex)
RG-->>RC: InstallerRevision
RC->>RC: enforce max revisions
RC->>CA: Patch ClusterAPI status (DesiredRevision, Revisions)
CA-->>RC: Status patched
RC->>RC: WriteClusterOperatorConditions()
RC-->>Manager: Result / error
deactivate RC
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-merge-robot
added
the
needs-rebase
mdbooth
force-pushed
the
revision-controller
branch
from
b34812d to
e65778a
Compare
openshift-merge-robot
removed
the
needs-rebase
mdbooth
force-pushed
the
revision-controller
branch
from
a7c68b7 to
b8e3123
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
mdbooth
force-pushed
the
revision-controller
branch
from
b8e3123 to
f905659
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
pkg/test/matchers/condition_test.go (1)
152-177: Missing test coverage forNegatedFailureMessagewhen match succeeds.The failure message tests only exercise
FailureMessagewhenMatchreturnsfalse. There are no tests forNegatedFailureMessagewhenMatchreturnstrue(i.e., when usingToNot()on a matching condition). This would have caught the nil pointer dereference bug in the implementation.🧪 Suggested test to add for NegatedFailureMessage
func TestHaveCondition_NegatedFailureMessages(t *testing.T) { g := NewWithT(t) conditions := []configv1.ClusterOperatorStatusCondition{ { Type: "Progressing", Status: configv1.ConditionTrue, Reason: "Working", }, } // Test NegatedFailureMessage when match succeeds matcher := HaveCondition("Progressing").WithStatus(configv1.ConditionTrue) success, err := matcher.Match(conditions) g.Expect(err).ToNot(HaveOccurred()) g.Expect(success).To(BeTrue()) // This should not panic and should produce a meaningful message g.Expect(matcher.NegatedFailureMessage(conditions)).To(ContainSubstring("Progressing")) } func TestBeCondition_NegatedFailureMessages(t *testing.T) { g := NewWithT(t) cond := configv1.ClusterOperatorStatusCondition{ Type: "Progressing", Status: configv1.ConditionTrue, Reason: "Working", } // Test NegatedFailureMessage when match succeeds matcher := BeCondition("Progressing").WithStatus(configv1.ConditionTrue) success, err := matcher.Match(cond) g.Expect(err).ToNot(HaveOccurred()) g.Expect(success).To(BeTrue()) g.Expect(matcher.NegatedFailureMessage(cond)).To(ContainSubstring("Progressing")) }Also applies to: 354-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/matchers/condition_test.go` around lines 152 - 177, Add tests covering NegatedFailureMessage for cases where Match returns true to avoid nil-pointer panics: create tests like TestHaveCondition_NegatedFailureMessages and TestBeCondition_NegatedFailureMessages that build matching inputs, call matcher := HaveCondition("Progressing").WithStatus(configv1.ConditionTrue) (and BeCondition equivalent), run success, err := matcher.Match(...), assert success==true and err==nil, then call matcher.NegatedFailureMessage(...) and assert it contains the condition Type (e.g., "Progressing"); this ensures NegatedFailureMessage handles the successful-match path in HaveCondition/BeCondition implementations.pkg/revisiongenerator/revision.go (2)
165-168: Variable shadows package name.The variable
unstructuredat line 165 shadows the importedunstructuredpackage. While Go allows this, it reduces clarity.✨ Rename for clarity
- var unstructured unstructured.Unstructured - if err := k8syaml.Unmarshal([]byte(yaml), &unstructured); err != nil { + var obj unstructured.Unstructured + if err := k8syaml.Unmarshal([]byte(yaml), &obj); err != nil { return nil, fmt.Errorf("error unmarshalling transformed manifest: %w", err) } - switch unstructured.GroupVersionKind().GroupKind() { + switch obj.GroupVersionKind().GroupKind() { case schema.GroupKind{Group: "apiextensions.k8s.io", Kind: "CustomResourceDefinition"}: - component.crds = append(component.crds, unstructured) + component.crds = append(component.crds, obj) default: - component.objects = append(component.objects, unstructured) + component.objects = append(component.objects, obj) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/revisiongenerator/revision.go` around lines 165 - 168, The local variable named "unstructured" shadows the imported package "unstructured"; rename the variable (for example to "usObj" or "obj") wherever it's declared and used in revision.go (the instance of type unstructured.Unstructured used with k8syaml.Unmarshal) so it no longer collides with the package name, keeping the type as unstructured.Unstructured and updating subsequent references to the new variable name.
121-137: Potential edge case: naive truncation could lose the revision index.If
releaseVersionis very long, the simple truncation at line 133 could remove the revision index, potentially causing naming collisions between different revisions. Consider truncating the version instead of the suffix to preserve uniqueness.💡 Alternative approach preserving revision index
func buildRevisionName(releaseVersion, contentID string, index int64) operatorv1alpha1.RevisionName { // Format: <version>-<contentID[:8]>-<number> shortContentID := contentID if len(shortContentID) > revisionContentIDLen { shortContentID = shortContentID[:revisionContentIDLen] } - name := fmt.Sprintf("%s-%s-%d", releaseVersion, shortContentID, index) + suffix := fmt.Sprintf("-%s-%d", shortContentID, index) + version := releaseVersion + maxVersionLen := maxRevisionNameLen - len(suffix) + if maxVersionLen < 0 { + maxVersionLen = 0 + } + if len(version) > maxVersionLen { + version = version[:maxVersionLen] + } + name := version + suffix - // Truncate if necessary - if len(name) > maxRevisionNameLen { - name = name[:maxRevisionNameLen] - } - return operatorv1alpha1.RevisionName(name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/revisiongenerator/revision.go` around lines 121 - 137, The current buildRevisionName function truncates the whole assembled name which can cut off the trailing "-<contentID>-<index>" and lose the revision index; change logic to always preserve the suffix and only truncate releaseVersion when necessary: compute shortContentID using revisionContentIDLen, build the suffix := fmt.Sprintf("-%s-%d", shortContentID, index), calculate allowedVersionLen := maxRevisionNameLen - len(suffix) (clamp to 0 if negative), truncate releaseVersion to allowedVersionLen, then assemble name as releaseVersion+suffix and return operatorv1alpha1.RevisionName(name); this ensures the revision index (and shortContentID) are never dropped.pkg/controllers/revision/revision_controller.go (1)
161-169: Acknowledged technical debt: potential conflict with installer controller.The XXX comment correctly identifies that using
Status().Update()can conflict with the installer controller. TheresourceVersionwill prevent data corruption, but conflicts will cause retries. Converting to Server-Side Apply (SSA) with field ownership as noted would be the proper fix.Consider tracking this as an issue if not already captured.
Would you like me to open an issue to track the SSA migration for this status update?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/revision/revision_controller.go` around lines 161 - 169, The status update should use Server-Side Apply to avoid conflicts with the installer controller: replace the r.Status().Update(ctx, clusterAPI) call with a status Patch using SSA (e.g., r.Status().Patch(ctx, clusterAPI, client.Apply, client.FieldOwner("revision-controller"), client.ForceOwnership) or equivalent in your controller-runtime version), ensuring you patch only the status subresource that includes clusterAPI.Status.Revisions and clusterAPI.Status.DesiredRevision (operate on a deep copy if needed) and set a clear field owner (e.g., "revision-controller"); this converts the update to SSA ownership semantics and prevents retry conflicts with the installer controller.pkg/operatorstatus/controller_status.go (1)
237-266: Minor:LastTransitionTimeupdate onReasonchange differs from K8s convention.Line 252 updates
LastTransitionTimewhenStatusORReasonchanges. The K8s convention is to update only onStatuschange. IncludingReasonis more conservative but may cause unnecessary timestamp churn.This is a stylistic choice, not a bug, so flagging as informational.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operatorstatus/controller_status.go` around lines 237 - 266, The code currently treats a change in Reason as a status transition and updates LastTransitionTime; modify the logic in the loop over newConditions so LastTransitionTime is updated only when Status changes (not when only Reason changes): use existing := findClusterOperatorCondition(*cond.Type) and compare existing.Status vs *cond.Status to decide whether to call cond.WithLastTransitionTime(now) and set needsUpdate, while still marking needsUpdate if Message or Reason differ (but preserve existing.LastTransitionTime when only Reason changed); keep references to cond.WithLastTransitionTime, existing.Status, existing.Reason, needsUpdate, newConditions and logConditions to locate and adjust the branch logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/capi-operator/main.go`:
- Around line 194-201: The RevisionController setup currently calls os.Exit(1)
on error which bypasses the surrounding function's error propagation; update the
error handling for (&revision.RevisionController{...}).SetupWithManager(mgr) to
return the error (e.g., return err) instead of calling os.Exit(1), keeping the
existing klog.Error call so the caller can handle shutdown consistently with
other controllers like CapiInstallerController.
In `@e2e/go.mod`:
- Line 18: The dependency github.com/openshift/api listed at
v0.0.0-20260213204242-d34f11c515b3 is declared in go.mod (and go.sum) but
missing from the vendor directory, causing vendor-mode builds to fail; fix by
running `go mod vendor` inside the e2e module to populate vendor with
github.com/openshift/api (and commit the updated vendor/ entries), then ensure
the vendor changes are included in the PR so the e2e build can resolve that
module.
In `@manifests-gen/go.mod`:
- Line 19: The go.mod declares github.com/openshift/api
v0.0.0-20260213204242-d34f11c515b3 but the vendor directory is missing that
module; run `go mod vendor` (or otherwise update the vendor tree) to populate
vendor with github.com/openshift/api at the declared version so the repository
works with -mod=vendor, or if selective vendoring was intended, remove or adjust
the github.com/openshift/api requirement from go.mod to match the vendored
contents.
In `@pkg/controllers/revision/revision_controller.go`:
- Around line 236-240: The toClusterOperator handler is enqueuing the wrong
object name (uses clusterOperatorName) so reconciliation targets a non-existent
ClusterAPI; update the handler (toClusterOperator) to enqueue the actual
ClusterAPI name by returning client.ObjectKey{Name: clusterName} (use the
existing clusterName constant) instead of clusterOperatorName so the real
"cluster" ClusterAPI is reconciled when infra becomes ready.
In `@pkg/test/matchers/condition.go`:
- Around line 314-333: The getStringValue function can panic when derefValue
returns an invalid reflect.Value (e.g., nil pointer fields); update
getStringValue to check for v.IsValid() (or v.Kind() == reflect.Invalid)
immediately after v = derefValue(v) and return an empty string (or a safe
default) if invalid, before any calls to v.Interface() or v.Kind(); ensure all
subsequent type assertions (string, fmt.Stringer) are only performed on a valid
reflect.Value so functions like derefValue and getStringValue no longer call
v.Interface() on invalid values.
- Around line 284-301: The NegatedFailureMessage in ConditionMatcher can panic
because when Match() succeeds it never sets
failureField/failureExpectedMatcher/failureActual; update NegatedFailureMessage
(method on ConditionMatcher) to detect the "success" case (e.g. failureField ==
"" or failureExpectedMatcher == nil) and return a safe generic negated-failure
string indicating the conditionType was found and all fields matched (use
format.Object(actual, 1) for the Condition/Conditions output) instead of falling
through to call failureExpectedMatcher.NegatedFailureMessage on a nil matcher.
---
Nitpick comments:
In `@pkg/controllers/revision/revision_controller.go`:
- Around line 161-169: The status update should use Server-Side Apply to avoid
conflicts with the installer controller: replace the r.Status().Update(ctx,
clusterAPI) call with a status Patch using SSA (e.g., r.Status().Patch(ctx,
clusterAPI, client.Apply, client.FieldOwner("revision-controller"),
client.ForceOwnership) or equivalent in your controller-runtime version),
ensuring you patch only the status subresource that includes
clusterAPI.Status.Revisions and clusterAPI.Status.DesiredRevision (operate on a
deep copy if needed) and set a clear field owner (e.g., "revision-controller");
this converts the update to SSA ownership semantics and prevents retry conflicts
with the installer controller.
In `@pkg/operatorstatus/controller_status.go`:
- Around line 237-266: The code currently treats a change in Reason as a status
transition and updates LastTransitionTime; modify the logic in the loop over
newConditions so LastTransitionTime is updated only when Status changes (not
when only Reason changes): use existing :=
findClusterOperatorCondition(*cond.Type) and compare existing.Status vs
*cond.Status to decide whether to call cond.WithLastTransitionTime(now) and set
needsUpdate, while still marking needsUpdate if Message or Reason differ (but
preserve existing.LastTransitionTime when only Reason changed); keep references
to cond.WithLastTransitionTime, existing.Status, existing.Reason, needsUpdate,
newConditions and logConditions to locate and adjust the branch logic.
In `@pkg/revisiongenerator/revision.go`:
- Around line 165-168: The local variable named "unstructured" shadows the
imported package "unstructured"; rename the variable (for example to "usObj" or
"obj") wherever it's declared and used in revision.go (the instance of type
unstructured.Unstructured used with k8syaml.Unmarshal) so it no longer collides
with the package name, keeping the type as unstructured.Unstructured and
updating subsequent references to the new variable name.
- Around line 121-137: The current buildRevisionName function truncates the
whole assembled name which can cut off the trailing "-<contentID>-<index>" and
lose the revision index; change logic to always preserve the suffix and only
truncate releaseVersion when necessary: compute shortContentID using
revisionContentIDLen, build the suffix := fmt.Sprintf("-%s-%d", shortContentID,
index), calculate allowedVersionLen := maxRevisionNameLen - len(suffix) (clamp
to 0 if negative), truncate releaseVersion to allowedVersionLen, then assemble
name as releaseVersion+suffix and return operatorv1alpha1.RevisionName(name);
this ensures the revision index (and shortContentID) are never dropped.
In `@pkg/test/matchers/condition_test.go`:
- Around line 152-177: Add tests covering NegatedFailureMessage for cases where
Match returns true to avoid nil-pointer panics: create tests like
TestHaveCondition_NegatedFailureMessages and
TestBeCondition_NegatedFailureMessages that build matching inputs, call matcher
:= HaveCondition("Progressing").WithStatus(configv1.ConditionTrue) (and
BeCondition equivalent), run success, err := matcher.Match(...), assert
success==true and err==nil, then call matcher.NegatedFailureMessage(...) and
assert it contains the condition Type (e.g., "Progressing"); this ensures
NegatedFailureMessage handles the successful-match path in
HaveCondition/BeCondition implementations.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (43)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.summanifests-gen/go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openapi/generated_openapi/zz_generated.openapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagecontentsourcepolicies.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_operator-lifecycle-manager_01_olms.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (30)
.golangci.ymlcmd/capi-controllers/main.gocmd/capi-operator/main.gocmd/crd-compatibility-checker/main.gocmd/machine-api-migration/main.goe2e/go.modgo.modmanifests-gen/go.modpkg/commoncmdoptions/commonoptions.gopkg/controllers/revision/helpers_test.gopkg/controllers/revision/revision_controller.gopkg/controllers/revision/revision_controller_test.gopkg/controllers/revision/suite_test.gopkg/conversion/capi2mapi/aws_fuzz_test.gopkg/conversion/mapi2capi/aws_fuzz_test.gopkg/conversion/mapi2capi/openstack_fuzz_test.gopkg/operatorstatus/controller_status.gopkg/operatorstatus/controller_status_test.gopkg/operatorstatus/operator_status.gopkg/providerimages/manifestreader.gopkg/providerimages/manifestreader_test.gopkg/providerimages/providerimages_test.gopkg/revisiongenerator/revision.gopkg/revisiongenerator/revision_test.gopkg/revisiongenerator/transform.gopkg/revisiongenerator/transform_test.gopkg/test/envtest.gopkg/test/matchers/condition.gopkg/test/matchers/condition_test.gopkg/util/conditions_test.go
💤 Files with no reviewable changes (5)
- pkg/conversion/mapi2capi/openstack_fuzz_test.go
- pkg/operatorstatus/operator_status.go
- pkg/conversion/capi2mapi/aws_fuzz_test.go
- pkg/util/conditions_test.go
- pkg/conversion/mapi2capi/aws_fuzz_test.go
JoelSpeed
reviewed
Feb 24, 2026
| return opresult.Error(fmt.Errorf("getting platform: %w", err)) | ||
| } | ||
|
|
||
| if platform == "" { |
Contributor
There was a problem hiding this comment.
At what point do we determine that a platform might not be supported? Will this controller only run on supported platforms? Or does it still search and exit on L95?
Contributor
Author
There was a problem hiding this comment.
This controller runs on unsupported platforms, as do all the controllers in the capi-operator endpoint. We could consider if they should or not.
Contributor
There was a problem hiding this comment.
As long as this sees that it is on an unsupported platform and bails out cleanly without a requeue, I'm not concerned for now
Contributor
Author
There was a problem hiding this comment.
It doesn't know anything about unsupported platforms. It's just going to run. I'll have a think about that.
Contributor
Author
There was a problem hiding this comment.
It's my intention to address this with #475. I'd like to merge that first (it's hopefully quite simple) and then rebase on top.
mdbooth
force-pushed
the
revision-controller
branch
from
f905659 to
dc556d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
manifests-gen/go.mod (1)
19-19:⚠️ Potential issue | 🟠 MajorVendor directory still needs to be updated for this module.
Same issue as flagged previously:
github.com/openshift/apiatv0.0.0-20260213204242-d34f11c515b3is present in go.mod/go.sum but missing frommanifests-gen/vendor/. Rungo mod vendorin themanifests-gendirectory and commit the result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@manifests-gen/go.mod` at line 19, The vendor directory for the manifests-gen module is out of sync: github.com/openshift/api at version v0.0.0-20260213204242-d34f11c515b3 is in go.mod/go.sum but missing from manifests-gen/vendor; fix this by changing into the manifests-gen module, run `go mod vendor` to populate manifests-gen/vendor (ensuring the github.com/openshift/api entry is vendored), verify go.mod/go.sum remain correct, and commit the updated manifests-gen/vendor directory and any modified go.sum.e2e/go.mod (1)
18-18:⚠️ Potential issue | 🟠 MajorVendor directory still needs to be updated for this module.
The vendoring gap flagged in the previous review remains:
github.com/openshift/apiat the new version is declared in go.mod/go.sum but absent frome2e/vendor/. Rungo mod vendorin thee2edirectory and commit the result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/go.mod` at line 18, The go.mod declares github.com/openshift/api at a new pseudo-version but the e2e/vendor/ tree wasn't updated; run `go mod vendor` from the e2e module root to populate e2e/vendor with the new github.com/openshift/api files, verify the added files, and commit the updated vendor directory (ensure go.sum updated as needed and include the vendor/ changes in the same commit).
🧹 Nitpick comments (1)
pkg/controllers/revision/revision_controller.go (1)
159-166: Resolve theXXXstatus update conflict with SSA.The current Status().Update will conflict with the installer controller; switching to SSA/patching avoids churn and conflict retries.
Do you want me to sketch an SSA-based status patch for this?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/revision/revision_controller.go` around lines 159 - 166, Instead of mutating clusterAPI and calling r.Status().Update (which causes conflicts), make a copy of the original ClusterAPI (orig := clusterAPI.DeepCopy()), modify only the status fields on clusterAPI (append to clusterAPI.Status.Revisions and set clusterAPI.Status.DesiredRevision), then call r.Status().Patch(ctx, clusterAPI, client.MergeFrom(orig)) so the controller-runtime will send a status patch (SSA/merge) rather than a full update; replace the r.Status().Update call with r.Status().Patch(ctx, clusterAPI, client.MergeFrom(orig)) and handle the returned error as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/revision/helpers_test.go`:
- Around line 175-203: The tests currently use providerImgs[i].ContentID
directly as the ConfigMap name in configMapYAML/ensureManifestPaths which can
contain invalid DNS-1123 characters or be too long; change ensureManifestPaths
to generate a DNS-1123-safe name (e.g., a sanitized/truncated ContentID or a
stable hash like sha256 prefix combined with providerImgs[i].Name) for the
ConfigMap file name and pass that safe name into configMapYAML, and then embed
the original ContentID into the ConfigMap content (either under data or
metadata.annotations) so uniqueness is preserved without producing invalid
resource names; update references to configMapYAML and the created file naming
logic in ensureManifestPaths accordingly.
In `@pkg/controllers/revision/revision_controller.go`:
- Around line 70-75: The current error handling joins the reconcileResult error
with the condition-write error which can preserve a TerminalError and prevent
requeue; change the return so that when WriteClusterOperatorConditions(ctx, log,
r.Client) returns an error you return that condition-write error alone (wrap it
with context like "failed to write conditions") instead of using errors.Join
with reconcileResult.Error(), ensuring retries are not suppressed; update the
block that calls reconcileResult.WriteClusterOperatorConditions and the returned
error handling to return only the wrapped write error while still allowing
reconcileResult.Error() to be surfaced elsewhere if needed.
In `@pkg/providerimages/manifestreader.go`:
- Around line 56-61: The code currently yields YAML documents even when they
contain only a separator. After creating doc with strings.TrimPrefix in the
YAMLReader, add a whitespace-trim check (e.g. trimmed := strings.TrimSpace(doc))
and skip/continue if trimmed == "" so separator-only or empty docs are not
yielded; then proceed to call yield(doc, nil) as before. Ensure you reference
the existing doc variable and the yield(...) call when making the change.
In `@pkg/test/matchers/condition.go`:
- Around line 169-220: Reset the matcher's transient failure state at the start
of Match to avoid leaking stale failureField/failureActual values across reused
matcher instances; add statements that clear m.failureField (e.g., set to empty
string) and m.failureActual (set to nil) at the top of ConditionMatcher.Match
before calling matchSingleCondition or matchConditionSlice so subsequent calls
(including negated assertions) start with a clean state.
---
Duplicate comments:
In `@e2e/go.mod`:
- Line 18: The go.mod declares github.com/openshift/api at a new pseudo-version
but the e2e/vendor/ tree wasn't updated; run `go mod vendor` from the e2e module
root to populate e2e/vendor with the new github.com/openshift/api files, verify
the added files, and commit the updated vendor directory (ensure go.sum updated
as needed and include the vendor/ changes in the same commit).
In `@manifests-gen/go.mod`:
- Line 19: The vendor directory for the manifests-gen module is out of sync:
github.com/openshift/api at version v0.0.0-20260213204242-d34f11c515b3 is in
go.mod/go.sum but missing from manifests-gen/vendor; fix this by changing into
the manifests-gen module, run `go mod vendor` to populate manifests-gen/vendor
(ensuring the github.com/openshift/api entry is vendored), verify go.mod/go.sum
remain correct, and commit the updated manifests-gen/vendor directory and any
modified go.sum.
---
Nitpick comments:
In `@pkg/controllers/revision/revision_controller.go`:
- Around line 159-166: Instead of mutating clusterAPI and calling
r.Status().Update (which causes conflicts), make a copy of the original
ClusterAPI (orig := clusterAPI.DeepCopy()), modify only the status fields on
clusterAPI (append to clusterAPI.Status.Revisions and set
clusterAPI.Status.DesiredRevision), then call r.Status().Patch(ctx, clusterAPI,
client.MergeFrom(orig)) so the controller-runtime will send a status patch
(SSA/merge) rather than a full update; replace the r.Status().Update call with
r.Status().Patch(ctx, clusterAPI, client.MergeFrom(orig)) and handle the
returned error as before.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (43)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.summanifests-gen/go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openapi/generated_openapi/zz_generated.openapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagecontentsourcepolicies.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_operator-lifecycle-manager_01_olms.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (25)
.golangci.ymlcmd/capi-controllers/main.gocmd/capi-operator/main.gocmd/crd-compatibility-checker/main.gocmd/machine-api-migration/main.goe2e/go.modgo.modmanifests-gen/go.modpkg/commoncmdoptions/commonoptions.gopkg/controllers/revision/helpers_test.gopkg/controllers/revision/revision_controller.gopkg/controllers/revision/revision_controller_test.gopkg/controllers/revision/suite_test.gopkg/operatorstatus/controller_status.gopkg/operatorstatus/controller_status_test.gopkg/operatorstatus/operator_status.gopkg/providerimages/manifestreader.gopkg/providerimages/manifestreader_test.gopkg/revisiongenerator/revision.gopkg/revisiongenerator/revision_test.gopkg/revisiongenerator/transform.gopkg/revisiongenerator/transform_test.gopkg/test/envtest.gopkg/test/matchers/condition.gopkg/test/matchers/condition_test.go
💤 Files with no reviewable changes (1)
- pkg/operatorstatus/operator_status.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/commoncmdoptions/commonoptions.go
- pkg/revisiongenerator/transform.go
- pkg/providerimages/manifestreader_test.go
- pkg/revisiongenerator/transform_test.go
- go.mod
- pkg/test/envtest.go
- .golangci.yml
openshift-ci-robot
added
the
jira/valid-reference
|
@mdbooth: This pull request references OCPCLOUD-3350 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
mdbooth
force-pushed
the
revision-controller
branch
from
dc556d8 to
2220192
Compare
|
@mdbooth: This pull request references OCPCLOUD-3350 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pkg/test/matchers/condition.go (1)
169-176:⚠️ Potential issue | 🟡 MinorStale failure state still not reset at the start of
Match.When a
ConditionMatcherinstance is reused across multipleMatchcalls,failureField,failureActual, andfailureExpectedMatcherretain values from the previous call. Concretely: if a first call fails on a field mismatch (setsfailureExpectedMatcher), and a second call on the same instance succeeds (leaving those fields stale),NegatedFailureMessagewill fall through to line 310 and callfailureExpectedMatcher.NegatedFailureMessage(m.failureActual)with the stale previous actual — producing a misleading message.🛡️ Proposed fix
func (m *ConditionMatcher) Match(actual interface{}) (bool, error) { + m.failureField = "" + m.failureActual = nil + m.failureExpectedMatcher = nil if m.singleElement { return m.matchSingleCondition(actual) } return m.matchConditionSlice(actual) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/matchers/condition.go` around lines 169 - 176, The Match method on ConditionMatcher should clear any previous failure state before evaluating a new actual; at the start of ConditionMatcher.Match, reset m.failureField, m.failureActual, and m.failureExpectedMatcher to their zero values (empty string / nil) so stale state from prior Match calls cannot leak into NegatedFailureMessage; make this change before calling m.matchSingleCondition or m.matchConditionSlice.pkg/controllers/revision/revision_controller.go (1)
251-269: Verify the Infrastructure watch enqueues the ClusterAPI name.If
operatorstatus.ToClusterOperatormaps tocluster-api, this controller will reconcile a non-existent ClusterAPI object and never act on infra readiness. Please confirm the mapping targets theclusterClusterAPI singleton.#!/bin/bash # Inspect ToClusterOperator mapping target rg -n 'func ToClusterOperator' -g'*.go' rg -n 'ToClusterOperator' -g'*.go'💡 If it maps to cluster-operator, consider a local map func
+import "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ - Watches(&configv1.Infrastructure{}, - handler.EnqueueRequestsFromMapFunc(operatorstatus.ToClusterOperator), + Watches(&configv1.Infrastructure{}, + handler.EnqueueRequestsFromMapFunc(func(_ context.Context, _ client.Object) []reconcile.Request { + return []reconcile.Request{{NamespacedName: client.ObjectKey{Name: clusterAPIName}}} + }), builder.WithPredicates(predicate.Funcs{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/revision/revision_controller.go` around lines 251 - 269, The Infrastructure watch currently uses handler.EnqueueRequestsFromMapFunc(operatorstatus.ToClusterOperator) which may map to "cluster-api" instead of the singleton ClusterAPI named by clusterAPIName; verify operatorstatus.ToClusterOperator returns requests for the ClusterAPI named clusterAPIName and if it does not, replace it with a local map function that converts an Infrastructure object to a ctrl.Request for the ClusterAPI singleton (using clusterAPIName) and use that map func in the Watches call (update the Watches invocation referencing operatorstatus.ToClusterOperator to instead reference your new map function); also update/add tests to assert the Infrastructure watch enqueues the expected ClusterAPI name.pkg/controllers/revision/helpers_test.go (1)
175-203: ConfigMap name derived from ContentID may be invalid.ContentID can include DNS-1123-invalid characters; using it as the
metadata.namemay be unsafe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/revision/helpers_test.go` around lines 175 - 203, The test currently uses providerImgs[i].ContentID directly as the ConfigMap metadata.name (in configMapYAML called from ensureManifestPaths), but ContentID may contain DNS-1123-invalid characters; sanitize it before use: create a helper that converts ContentID to a DNS-1123-compliant name (lowercase, replace invalid chars with '-', collapse runs, trim non-alphanumeric ends, enforce <=253 chars and non-empty—fallback to a stable hash if needed) and use that sanitized name when calling configMapYAML / writing files in ensureManifestPaths so metadata.name is always valid Kubernetes DNS-1123.
🧹 Nitpick comments (6)
pkg/test/matchers/condition_test.go (1)
152-177:NegatedFailureMessagefor the "match succeeded" path is not covered.The critical nil-pointer guard added to
NegatedFailureMessage(condition.go lines 296–305) — the fix from the prior review — is never exercised by any test. All existingToNot(…)assertions create fresh matchers that returnfalse(type not found), so they only reach thefailureField == conditionFieldTypebranch. The guard code path (failureExpectedMatcher == nilafterMatch()returnstrue) is dead from a test perspective.Consider adding a direct call pair to each failure-message test, e.g.:
// NegatedFailureMessage after successful match (exercises lines 296-305 of condition.go) matcher = HaveCondition("Progressing") success, err = matcher.Match(conditions) g.Expect(err).ToNot(HaveOccurred()) g.Expect(success).To(BeTrue()) g.Expect(matcher.NegatedFailureMessage(conditions)).To(ContainSubstring("Progressing"))Also applies to: 354-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/matchers/condition_test.go` around lines 152 - 177, Add assertions that exercise the NegatedFailureMessage nil-pointer guard by invoking Match(...) on a matcher that should succeed and then calling NegatedFailureMessage(...); specifically, after creating matcher := HaveCondition("Progressing") call success, err := matcher.Match(conditions), assert success==true and err==nil, then assert matcher.NegatedFailureMessage(conditions) contains "Progressing". Repeat the same pattern for the other failure-message test block (the one covering the other case mentioned) so the code path in NegatedFailureMessage (the nil check for failureExpectedMatcher) is executed.pkg/operatorstatus/controller_status_test.go (1)
187-193: "on Error" sub-test discards the error fromResult().The test confirms
RequeueAfteris forwarded but doesn't assert that the underlying error is also returned byResult(). Adding the assertion would make this the only sub-test that verifies error propagation through theResult()path.♻️ Proposed fix
t.Run("on Error", func(t *testing.T) { g := NewWithT(t) - result := testResultGenerator.Error(fmt.Errorf("transient"), WithRequeueAfter(15*time.Second)) + testErr := fmt.Errorf("transient") + result := testResultGenerator.Error(testErr, WithRequeueAfter(15*time.Second)) - ctrlResult, _ := result.Result() + ctrlResult, err := result.Result() + g.Expect(err).To(Equal(testErr)) g.Expect(ctrlResult.RequeueAfter).To(Equal(15 * time.Second)) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operatorstatus/controller_status_test.go` around lines 187 - 193, The "on Error" sub-test currently ignores the error returned by testResultGenerator.Error(...).Result(); update the test to capture both values (e.g., `ctrlResult, err := result.Result()`), assert that `err` is non-nil and matches the original error (or its message), and keep the existing assertion that `ctrlResult.RequeueAfter` equals 15 * time.Second so the test verifies both requeue timing and error propagation from Result(); reference the call to `testResultGenerator.Error(...)` and the `Result()` method when adding the assertion.pkg/operatorstatus/controller_status.go (3)
237-241: Nil condition pointer would panic before reaching the field-level guard.If any entry in
newConditionsis a nil*ClusterOperatorStatusConditionApplyConfiguration, evaluatingcond.Typeat line 238 panics with a nil-pointer dereference before the explicitnilcheck is ever reached. The guard itself needs a leading pointer check.♻️ Proposed fix
for _, cond := range newConditions { - if cond.Type == nil || cond.Status == nil || cond.Reason == nil || cond.Message == nil { + if cond == nil || cond.Type == nil || cond.Status == nil || cond.Reason == nil || cond.Message == nil { // Programming error - should never happen panic(fmt.Sprintf("condition is missing required fields: %+v", cond)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operatorstatus/controller_status.go` around lines 237 - 241, The loop over newConditions can dereference a nil pointer before the field guards; add a leading pointer nil-check inside the loop that panics (or otherwise handles) when cond == nil before accessing cond.Type, cond.Status, cond.Reason or cond.Message. Specifically, update the for _, cond := range newConditions loop that validates *ClusterOperatorStatusConditionApplyConfiguration to first check "if cond == nil { panic(...)" with a clear message, then proceed to the existing field-level nil checks for cond.Type, cond.Status, cond.Reason and cond.Message.
161-168:termErr.Error()leaks the"terminal error: "prefix into operator status conditions.
terminalError.Error()returns"terminal error: " + te.err.Error(), so the ClusterOperator condition message will read"terminal error: bad config"rather than just"bad config". This is an implementation detail of controller-runtime leaking into a user-visible API surface. Suggest unwrapping to extract just the inner message:♻️ Proposed fix
func (c ControllerResultGenerator) nonRetryableError(termErr error, opts ...ReconcileResultOption) ReconcileResult { + // Unwrap to avoid leaking the "terminal error: " prefix into user-visible conditions. + msg := termErr.Error() + if inner := errors.Unwrap(termErr); inner != nil { + msg = inner.Error() + } return resultWithOptions(ReconcileResult{ ControllerResultGenerator: c, - progressing: c.progressingCondition(configv1.ConditionFalse, ReasonNonRetryableError, termErr.Error()), - degraded: c.degradedCondition(configv1.ConditionTrue, ReasonNonRetryableError, termErr.Error()), + progressing: c.progressingCondition(configv1.ConditionFalse, ReasonNonRetryableError, msg), + degraded: c.degradedCondition(configv1.ConditionTrue, ReasonNonRetryableError, msg), err: termErr, }, opts...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operatorstatus/controller_status.go` around lines 161 - 168, The nonRetryableError method is using termErr.Error() which preserves the controller-runtime wrapper prefix; instead, unwrap termErr to its underlying cause before using its message for conditions. In ControllerResultGenerator.nonRetryableError, extract the root error by looping with errors.Unwrap (or using errors.As) until no further unwrap is possible, then use that rootErr.Error() when calling progressingCondition and degradedCondition (and keep termErr as the err field); also ensure the "errors" package is imported.
84-89:WithRequeueAfteris silently a no-op when the result carries a non-nil error.Controller-runtime ignores
RequeueAfter(and logs a warning) when the reconciler returns a non-nil error:"RequeueAfter and Requeue will always be ignored if the error is non-nil."This meansWithRequeueAfterhas no effect on results fromError(...)orNonRetryableError(...). The option's doc comment should call this out, or the factory methods should discardrequeueAfterwhenerr != nil.♻️ Option A – document the limitation
// WithRequeueAfter sets requeueAfter on the returned reconcile.Result. +// Note: controller-runtime ignores RequeueAfter when the result carries a +// non-nil error; this option has no effect on Error or NonRetryableError results. func WithRequeueAfter(requeueAfter time.Duration) ReconcileResultOption {♻️ Option B – enforce in Result()
func (r *ReconcileResult) Result() (ctrl.Result, error) { - return ctrl.Result{RequeueAfter: r.requeueAfter}, r.err + if r.err != nil { + return ctrl.Result{}, r.err + } + return ctrl.Result{RequeueAfter: r.requeueAfter}, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operatorstatus/controller_status.go` around lines 84 - 89, The WithRequeueAfter option currently sets ReconcileResult.requeueAfter but controller-runtime will ignore RequeueAfter when an error is returned; update the ReconcileResult.Result() builder to enforce this by clearing/ignoring requeueAfter when r.err != nil (e.g., if r.err != nil then set local requeueAfter = 0 or omit it from the constructed reconcile.Result) so the returned Result matches controller-runtime behavior; also update the WithRequeueAfter doc comment to state that RequeueAfter is ignored when the result carries a non-nil error (and reference WithRequeueAfter, ReconcileResult, Result(), Error(...) and NonRetryableError(...) to locate the changes).pkg/controllers/revision/revision_controller.go (1)
151-174: Consider following through on the SSA note.The XXX comment flags a real conflict risk with the installer controller. Converting this status update to SSA would prevent update races.
If you want, I can draft an SSA-based update for this block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/revision/revision_controller.go` around lines 151 - 174, The status update in writeNewRevision currently does a direct r.Status().Update which can conflict with the installer controller; change this to a server-side-apply (SSA) status patch to avoid races: after appending newRevision to clusterAPI.Status.Revisions and setting clusterAPI.Status.DesiredRevision, perform an SSA patch against the status subresource (use r.Status().Patch with client.Apply and set a consistent FieldOwner like "revision-controller", optionally ForceOwnership) instead of r.Status().Update; keep the same newRevision assignment and return values, just replace the Update call with the SSA status Patch to ensure safe merges with the installer controller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/test/envtest.go`:
- Around line 64-66: StartEnvTest now returns client.WithWatch instead of
client.Client so update the client variable declarations in the listed
suite_test.go files (pkg/controllers/*/suite_test.go) from client.Client to
client.WithWatch; locate the test-level client variables (e.g., k8sClient or
client declarations used with StartEnvTest and in BeforeSuite/Setup) and change
their type to client.WithWatch, adjust any import references to
sigs.k8s.io/controller-runtime/pkg/client if needed, and ensure any places that
relied on concrete client.Client behavior still compile against the
client.WithWatch interface.
---
Duplicate comments:
In `@pkg/controllers/revision/helpers_test.go`:
- Around line 175-203: The test currently uses providerImgs[i].ContentID
directly as the ConfigMap metadata.name (in configMapYAML called from
ensureManifestPaths), but ContentID may contain DNS-1123-invalid characters;
sanitize it before use: create a helper that converts ContentID to a
DNS-1123-compliant name (lowercase, replace invalid chars with '-', collapse
runs, trim non-alphanumeric ends, enforce <=253 chars and non-empty—fallback to
a stable hash if needed) and use that sanitized name when calling configMapYAML
/ writing files in ensureManifestPaths so metadata.name is always valid
Kubernetes DNS-1123.
In `@pkg/controllers/revision/revision_controller.go`:
- Around line 251-269: The Infrastructure watch currently uses
handler.EnqueueRequestsFromMapFunc(operatorstatus.ToClusterOperator) which may
map to "cluster-api" instead of the singleton ClusterAPI named by
clusterAPIName; verify operatorstatus.ToClusterOperator returns requests for the
ClusterAPI named clusterAPIName and if it does not, replace it with a local map
function that converts an Infrastructure object to a ctrl.Request for the
ClusterAPI singleton (using clusterAPIName) and use that map func in the Watches
call (update the Watches invocation referencing operatorstatus.ToClusterOperator
to instead reference your new map function); also update/add tests to assert the
Infrastructure watch enqueues the expected ClusterAPI name.
In `@pkg/test/matchers/condition.go`:
- Around line 169-176: The Match method on ConditionMatcher should clear any
previous failure state before evaluating a new actual; at the start of
ConditionMatcher.Match, reset m.failureField, m.failureActual, and
m.failureExpectedMatcher to their zero values (empty string / nil) so stale
state from prior Match calls cannot leak into NegatedFailureMessage; make this
change before calling m.matchSingleCondition or m.matchConditionSlice.
---
Nitpick comments:
In `@pkg/controllers/revision/revision_controller.go`:
- Around line 151-174: The status update in writeNewRevision currently does a
direct r.Status().Update which can conflict with the installer controller;
change this to a server-side-apply (SSA) status patch to avoid races: after
appending newRevision to clusterAPI.Status.Revisions and setting
clusterAPI.Status.DesiredRevision, perform an SSA patch against the status
subresource (use r.Status().Patch with client.Apply and set a consistent
FieldOwner like "revision-controller", optionally ForceOwnership) instead of
r.Status().Update; keep the same newRevision assignment and return values, just
replace the Update call with the SSA status Patch to ensure safe merges with the
installer controller.
In `@pkg/operatorstatus/controller_status_test.go`:
- Around line 187-193: The "on Error" sub-test currently ignores the error
returned by testResultGenerator.Error(...).Result(); update the test to capture
both values (e.g., `ctrlResult, err := result.Result()`), assert that `err` is
non-nil and matches the original error (or its message), and keep the existing
assertion that `ctrlResult.RequeueAfter` equals 15 * time.Second so the test
verifies both requeue timing and error propagation from Result(); reference the
call to `testResultGenerator.Error(...)` and the `Result()` method when adding
the assertion.
In `@pkg/operatorstatus/controller_status.go`:
- Around line 237-241: The loop over newConditions can dereference a nil pointer
before the field guards; add a leading pointer nil-check inside the loop that
panics (or otherwise handles) when cond == nil before accessing cond.Type,
cond.Status, cond.Reason or cond.Message. Specifically, update the for _, cond
:= range newConditions loop that validates
*ClusterOperatorStatusConditionApplyConfiguration to first check "if cond == nil
{ panic(...)" with a clear message, then proceed to the existing field-level nil
checks for cond.Type, cond.Status, cond.Reason and cond.Message.
- Around line 161-168: The nonRetryableError method is using termErr.Error()
which preserves the controller-runtime wrapper prefix; instead, unwrap termErr
to its underlying cause before using its message for conditions. In
ControllerResultGenerator.nonRetryableError, extract the root error by looping
with errors.Unwrap (or using errors.As) until no further unwrap is possible,
then use that rootErr.Error() when calling progressingCondition and
degradedCondition (and keep termErr as the err field); also ensure the "errors"
package is imported.
- Around line 84-89: The WithRequeueAfter option currently sets
ReconcileResult.requeueAfter but controller-runtime will ignore RequeueAfter
when an error is returned; update the ReconcileResult.Result() builder to
enforce this by clearing/ignoring requeueAfter when r.err != nil (e.g., if r.err
!= nil then set local requeueAfter = 0 or omit it from the constructed
reconcile.Result) so the returned Result matches controller-runtime behavior;
also update the WithRequeueAfter doc comment to state that RequeueAfter is
ignored when the result carries a non-nil error (and reference WithRequeueAfter,
ReconcileResult, Result(), Error(...) and NonRetryableError(...) to locate the
changes).
In `@pkg/test/matchers/condition_test.go`:
- Around line 152-177: Add assertions that exercise the NegatedFailureMessage
nil-pointer guard by invoking Match(...) on a matcher that should succeed and
then calling NegatedFailureMessage(...); specifically, after creating matcher :=
HaveCondition("Progressing") call success, err := matcher.Match(conditions),
assert success==true and err==nil, then assert
matcher.NegatedFailureMessage(conditions) contains "Progressing". Repeat the
same pattern for the other failure-message test block (the one covering the
other case mentioned) so the code path in NegatedFailureMessage (the nil check
for failureExpectedMatcher) is executed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (43)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.summanifests-gen/go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openapi/generated_openapi/zz_generated.openapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagecontentsourcepolicies.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_operator-lifecycle-manager_01_olms.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (30)
.golangci.ymlcmd/capi-controllers/main.gocmd/capi-operator/main.gocmd/crd-compatibility-checker/main.gocmd/machine-api-migration/main.goe2e/go.modgo.modmanifests-gen/go.modpkg/commoncmdoptions/commonoptions.gopkg/controllers/revision/helpers_test.gopkg/controllers/revision/revision_controller.gopkg/controllers/revision/revision_controller_test.gopkg/controllers/revision/suite_test.gopkg/conversion/capi2mapi/aws_fuzz_test.gopkg/conversion/mapi2capi/aws_fuzz_test.gopkg/conversion/mapi2capi/openstack_fuzz_test.gopkg/operatorstatus/controller_status.gopkg/operatorstatus/controller_status_test.gopkg/operatorstatus/operator_status.gopkg/providerimages/manifestreader.gopkg/providerimages/manifestreader_test.gopkg/providerimages/providerimages_test.gopkg/revisiongenerator/revision.gopkg/revisiongenerator/revision_test.gopkg/revisiongenerator/transform.gopkg/revisiongenerator/transform_test.gopkg/test/envtest.gopkg/test/matchers/condition.gopkg/test/matchers/condition_test.gopkg/util/conditions_test.go
💤 Files with no reviewable changes (5)
- pkg/conversion/mapi2capi/openstack_fuzz_test.go
- pkg/conversion/mapi2capi/aws_fuzz_test.go
- pkg/conversion/capi2mapi/aws_fuzz_test.go
- pkg/util/conditions_test.go
- pkg/operatorstatus/operator_status.go
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/providerimages/manifestreader.go
- manifests-gen/go.mod
- pkg/revisiongenerator/transform.go
- e2e/go.mod
- cmd/capi-controllers/main.go
- pkg/controllers/revision/suite_test.go
- pkg/commoncmdoptions/commonoptions.go
- pkg/revisiongenerator/transform_test.go
- cmd/crd-compatibility-checker/main.go
- .golangci.yml
- pkg/revisiongenerator/revision.go
- pkg/controllers/revision/revision_controller_test.go
- go.mod
mdbooth
force-pushed
the
revision-controller
branch
from
2220192 to
71440d1
Compare
|
@mdbooth: This pull request references OCPCLOUD-3350 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
pkg/test/matchers/condition.go (1)
169-176: Stale matcher failure state on reuse.If the same
ConditionMatcherinstance is reused across multipleExpectcalls, thefailure*fields from a priorMatchinvocation can leak into subsequentFailureMessage/NegatedFailureMessagecalls. Resetting them at the top ofMatchwould make this bulletproof.🧹 Suggested fix
func (m *ConditionMatcher) Match(actual interface{}) (bool, error) { + m.failureField = "" + m.failureActual = nil + m.failureExpectedMatcher = nil + if m.singleElement { return m.matchSingleCondition(actual) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/matchers/condition.go` around lines 169 - 176, The ConditionMatcher.Match method can leak previous run state via its failure* fields; at the start of Match (before calling matchSingleCondition or matchConditionSlice) reset all fields used by FailureMessage/NegatedFailureMessage on the ConditionMatcher instance (e.g., failureReason, failureIndex, failureExpected, etc.) to their zero values so each Expect starts fresh; keep the existing branching logic that calls matchSingleCondition(actual) when m.singleElement is true and matchConditionSlice(actual) otherwise.
🧹 Nitpick comments (3)
pkg/test/envtest.go (1)
63-65: Align the doc comment with actual return order.The comment says “returns a client and config”, but the function returns config first, then client. Small wording fix will reduce reader confusion.
✏️ Suggested wording tweak
-// StartEnvTest starts a new test environment and returns a client and config. +// StartEnvTest starts a new test environment and returns a config and client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/envtest.go` around lines 63 - 65, The doc comment for StartEnvTest is misleading about the return order; update the comment to state that the function returns a config and then a client (the returned *rest.Config followed by the client.WithWatch) so it matches the actual signature of StartEnvTest. Ensure the summary and any inline phrasing refer to "config and client" (or "configuration and client") in that order and keep the note that the returned client implements client.WithWatch for interceptor clients.pkg/test/matchers/condition.go (1)
328-343: Dead code:v.Interface().(string)assertion on lines 335-337 is unreachable.After
derefValue, ifv.Kind() == reflect.Stringis true (line 331), we return early. For types with underlying kindstring(e.g.,type Foo string),reflect.Kindis stillreflect.String, sov.String()on line 332 already handles them. The type assertion on line 335 can never succeed for a non-string-kind value.You can safely remove lines 334-337.
🧹 Suggested cleanup
func getStringValue(v reflect.Value) string { v = derefValue(v) if v.Kind() == reflect.String { return v.String() } - // For other types, try to convert to string via interface - if s, ok := v.Interface().(string); ok { - return s - } // Try fmt.Stringer interface if stringer, ok := v.Interface().(fmt.Stringer); ok { return stringer.String() } return fmt.Sprintf("%v", v.Interface()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/matchers/condition.go` around lines 328 - 343, In getStringValue, remove the unreachable type assertion block that checks v.Interface().(string) after derefValue and the reflect.String branch; instead keep the existing reflect.String early return, then attempt the fmt.Stringer assertion (fmt.Stringer) and finally fallback to fmt.Sprintf, ensuring you only reference getStringValue, derefValue, reflect.String and fmt.Stringer when locating the code to edit.pkg/controllers/revision/revision_controller.go (1)
295-298: InfrastructureUpdateFuncpredicate only covers the !ready→ready transition.Once Infrastructure is already ready (
PlatformStatus != nil), subsequent updates to it (e.g., aPlatformStatusfield change) will not trigger reconciliation through this watch. TheFor(ClusterAPI)watch covers ongoing changes to the primary resource, so this is fine for the current use-case where platform type is immutable post-install. Worth confirming this is intentional if future fields inPlatformStatusare expected to drive revision changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/revision/revision_controller.go` around lines 295 - 298, The UpdateFunc currently only enqueues on a !ready→ready transition (using isInfrastructureReady on e.ObjectOld/e.ObjectNew); update it so it also enqueues when PlatformStatus changes while already ready (or remove the restrictive predicate), e.g., compare relevant PlatformStatus fields between e.ObjectOld and e.ObjectNew and return true if isInfrastructureReady(e.ObjectNew) && (!isInfrastructureReady(e.ObjectOld) || !platformStatusEqual(e.ObjectOld, e.ObjectNew)); ensure you reference the UpdateFunc handling event.UpdateEvent and the isInfrastructureReady helper (or add a platformStatusEqual helper) so changes to PlatformStatus will trigger reconciliation if intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/revision/helpers_test.go`:
- Around line 176-177: latestRevision currently panics with an
index-out-of-range on empty slices; add an explicit guard at the top of the
latestRevision(revisions []operatorv1alpha1.ClusterAPIInstallerRevision)
function such as if len(revisions) == 0 { panic("latestRevision: received empty
revisions slice") } (or return an explicit zero value/error if you prefer) so
failures are clear and locate the change in the latestRevision function and the
operatorv1alpha1.ClusterAPIInstallerRevision type usage.
In `@pkg/controllers/revision/revision_controller.go`:
- Line 289: operatorstatus.ToClusterOperator currently enqueues
controllers.ClusterOperatorName ("cluster-api") which is wrong; update
ToClusterOperator so it enqueues the ClusterAPI singleton name "cluster" (the
name used in revision_controller.go line where the singleton is defined) instead
of controllers.ClusterOperatorName, ensuring the
handler.EnqueueRequestsFromMapFunc(operatorstatus.ToClusterOperator) will
trigger reconciliation for the correct Cluster API object; locate and replace
the enqueued value in operatorstatus.ToClusterOperator (or use a shared constant
for "cluster" if available) so the MapFunc returns a request for name "cluster".
In `@pkg/operatorstatus/controller_status_test.go`:
- Around line 180-193: In the "on Error" test, capture and assert the second
return value from result.Result() instead of discarding it: call ctrlResult, err
:= result.Result() and assert err is non-nil (and/or matches the original
fmt.Errorf("transient")) so regressions in error propagation are caught; update
the test that uses testResultGenerator.Error(...) and Result() to include this
assertion (e.g., expect err to match the transient error) while keeping the
existing RequeueAfter assertion.
In `@pkg/revisiongenerator/revision.go`:
- Around line 220-225: The current truncation of name := fmt.Sprintf("%s-%s-%d",
releaseVersion, shortContentID, index) may remove the uniqueness suffix; change
the logic to build the suffix separately (e.g., suffix := fmt.Sprintf("-%s-%d",
shortContentID, index)), reserve len(suffix) characters when enforcing
maxRevisionNameLen, and truncate only the releaseVersion portion to
maxRevisionNameLen - len(suffix) (guarding against negative lengths) before
concatenating releaseVersion + suffix; update uses of name, maxRevisionNameLen,
releaseVersion, shortContentID, and index accordingly.
---
Duplicate comments:
In `@pkg/test/matchers/condition.go`:
- Around line 169-176: The ConditionMatcher.Match method can leak previous run
state via its failure* fields; at the start of Match (before calling
matchSingleCondition or matchConditionSlice) reset all fields used by
FailureMessage/NegatedFailureMessage on the ConditionMatcher instance (e.g.,
failureReason, failureIndex, failureExpected, etc.) to their zero values so each
Expect starts fresh; keep the existing branching logic that calls
matchSingleCondition(actual) when m.singleElement is true and
matchConditionSlice(actual) otherwise.
---
Nitpick comments:
In `@pkg/controllers/revision/revision_controller.go`:
- Around line 295-298: The UpdateFunc currently only enqueues on a !ready→ready
transition (using isInfrastructureReady on e.ObjectOld/e.ObjectNew); update it
so it also enqueues when PlatformStatus changes while already ready (or remove
the restrictive predicate), e.g., compare relevant PlatformStatus fields between
e.ObjectOld and e.ObjectNew and return true if
isInfrastructureReady(e.ObjectNew) && (!isInfrastructureReady(e.ObjectOld) ||
!platformStatusEqual(e.ObjectOld, e.ObjectNew)); ensure you reference the
UpdateFunc handling event.UpdateEvent and the isInfrastructureReady helper (or
add a platformStatusEqual helper) so changes to PlatformStatus will trigger
reconciliation if intended.
In `@pkg/test/envtest.go`:
- Around line 63-65: The doc comment for StartEnvTest is misleading about the
return order; update the comment to state that the function returns a config and
then a client (the returned *rest.Config followed by the client.WithWatch) so it
matches the actual signature of StartEnvTest. Ensure the summary and any inline
phrasing refer to "config and client" (or "configuration and client") in that
order and keep the note that the returned client implements client.WithWatch for
interceptor clients.
In `@pkg/test/matchers/condition.go`:
- Around line 328-343: In getStringValue, remove the unreachable type assertion
block that checks v.Interface().(string) after derefValue and the reflect.String
branch; instead keep the existing reflect.String early return, then attempt the
fmt.Stringer assertion (fmt.Stringer) and finally fallback to fmt.Sprintf,
ensuring you only reference getStringValue, derefValue, reflect.String and
fmt.Stringer when locating the code to edit.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (89)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.summanifests-gen/go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openapi/generated_openapi/zz_generated.openapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagecontentsourcepolicies.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_operator-lifecycle-manager_01_olms.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/acceptrisk.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/clusterversionstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/conditionalupdate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/conditionalupdaterisk.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/oidcprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenclaimvalidationcelrule.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenclaimvalidationrule.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenissuer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenuservalidationrule.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/update.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clustermonitoringspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/criocredentialproviderconfigspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/criocredentialproviderconfigstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusoperatorconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/config_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1alpha1/criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1alpha1/interface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/generic.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1alpha1/criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1alpha1/expansion_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/machinesetstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/machinestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/backupjobreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallercomponent.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallercomponentimage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallerrevision.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapispec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapistatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterversionoperator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterversionoperatorspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterversionoperatorstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/etcdbackup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/etcdbackupspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/etcdbackupstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/imagecontentsourcepolicy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/imagecontentsourcepolicyspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/olm.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/olmspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/olmstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/repositorydigestmirrors.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (25)
.golangci.ymlcmd/capi-controllers/main.gocmd/capi-operator/main.gocmd/crd-compatibility-checker/main.gocmd/machine-api-migration/main.goe2e/go.modgo.modmanifests-gen/go.modpkg/commoncmdoptions/commonoptions.gopkg/controllers/revision/helpers_test.gopkg/controllers/revision/revision_controller.gopkg/controllers/revision/revision_controller_test.gopkg/controllers/revision/suite_test.gopkg/operatorstatus/controller_status.gopkg/operatorstatus/controller_status_test.gopkg/operatorstatus/operator_status.gopkg/providerimages/manifestreader.gopkg/providerimages/manifestreader_test.gopkg/revisiongenerator/revision.gopkg/revisiongenerator/revision_test.gopkg/revisiongenerator/transform.gopkg/revisiongenerator/transform_test.gopkg/test/envtest.gopkg/test/matchers/condition.gopkg/test/matchers/condition_test.go
💤 Files with no reviewable changes (1)
- pkg/operatorstatus/operator_status.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/test/matchers/condition_test.go
- pkg/providerimages/manifestreader_test.go
- pkg/controllers/revision/revision_controller_test.go
- pkg/providerimages/manifestreader.go
- manifests-gen/go.mod
- cmd/crd-compatibility-checker/main.go
- pkg/revisiongenerator/transform_test.go
- go.mod
- pkg/operatorstatus/controller_status.go
- pkg/commoncmdoptions/commonoptions.go
- pkg/controllers/revision/suite_test.go
mdbooth
force-pushed
the
revision-controller
branch
from
71440d1 to
c95d753
Compare
|
@mdbooth: This pull request references OCPCLOUD-3350 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
pkg/test/matchers/condition.go (1)
170-176:⚠️ Potential issue | 🟡 MinorReset transient failure state at the start of
Match.
ConditionMatcherkeeps failure context on the struct, so reusing the same matcher instance can leak stale state into laterFailureMessage/NegatedFailureMessageoutput. Clear the transient fields before dispatching.Suggested patch
func (m *ConditionMatcher) Match(actual interface{}) (bool, error) { + m.failureField = "" + m.failureActual = nil + m.failureExpectedMatcher = nil + if m.singleElement { return m.matchSingleCondition(actual) } return m.matchConditionSlice(actual) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/matchers/condition.go` around lines 170 - 176, The Match method on ConditionMatcher retains transient failure state across invocations; at the start of ConditionMatcher.Match, clear any fields used to build FailureMessage/NegatedFailureMessage (reset the transient failure-context fields on the struct) before dispatching to matchSingleCondition or matchConditionSlice so stale state cannot leak into later messages.cmd/capi-operator/main.go (1)
194-201:⚠️ Potential issue | 🟡 MinorRedundant
klog.Errorbefore returning — this error is logged again bymain().
setupCapiInstallerController→setupControllers→main()logs all errors viaklog.Error(err, "unable to setup controllers"). Theklog.Errorat line 199 causes a double-log. The other controller setups in this file (e.g.,CapiInstallerController,ClusterOperatorController) return without a priorklog.Error.🧹 Proposed fix
if err := (&revision.RevisionController{ Client: mgr.GetClient(), ProviderProfiles: providerProfiles, ReleaseVersion: util.GetReleaseVersion(), }).SetupWithManager(mgr); err != nil { - klog.Error(err, "unable to create revision controller", "controller", "RevisionController") return fmt.Errorf("unable to create revision controller: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/capi-operator/main.go` around lines 194 - 201, The block creating revision.RevisionController currently logs the error with klog.Error and then returns it, causing duplicate logs upstream; remove the klog.Error(err, "unable to create revision controller", "controller", "RevisionController") call so the function simply returns fmt.Errorf("unable to create revision controller: %w", err) when (&revision.RevisionController{...}).SetupWithManager(mgr) fails (identify the RevisionController struct instantiation and its SetupWithManager call to locate the change).
🧹 Nitpick comments (3)
pkg/controllers/revision/revision_controller.go (1)
151-155:desiredRevision == nilguard is unreachable dead code.
generateDesiredRevisionalways returns either(non-nil revision, nil)or(nil, non-nil result), andreconcileshort-circuits on a non-nil result before reachingmergeRevisions. The nil-guard will never fire. Consider removing it (or adding apanic/errors.Newto make the contract explicit if you prefer defensive coding).♻️ Proposed refactor
func (r *RevisionController) mergeRevisions(log logr.Logger, apiRevisions []operatorv1alpha1.ClusterAPIInstallerRevision, desiredRevision revisiongenerator.RenderedRevision) ([]operatorv1alpha1.ClusterAPIInstallerRevision, error) { - // If there's no current revision we have nothing to merge - if desiredRevision == nil { - return apiRevisions, nil - } - nextRevisionIndex := int64(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/revision/revision_controller.go` around lines 151 - 155, The nil-check guard for desiredRevision in mergeRevisions is unreachable because generateDesiredRevision either returns a non-nil revision or a non-nil reconcile result and reconcile short-circuits before mergeRevisions; remove the unreachable block checking "if desiredRevision == nil { return apiRevisions, nil }" from the mergeRevisions function (or, if you prefer an explicit contract, replace it with a defensive panic or return an error such as errors.New("mergeRevisions called with nil desiredRevision") so the failure is explicit), referencing the mergeRevisions function and the desiredRevision value and ensuring callers (reconcile/generateDesiredRevision) remain unchanged.pkg/operatorstatus/controller_status.go (1)
294-299: Remove unreachable branch in unchanged-condition path.Inside
existing.Status == ... && existing.Reason == ... && existing.Message == ..., the nestedif existing.Message != *cond.Messagecan never be true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operatorstatus/controller_status.go` around lines 294 - 299, The branch checking if existing.Message != *cond.Message inside the block that already confirms existing.Status == *cond.Status && existing.Reason == *cond.Reason && existing.Message == *cond.Message is unreachable; remove that inner if and its body (the conditional that sets needsUpdate) and instead ensure needsUpdate is only set when fields that can actually change differ (e.g., compare LastTransitionTime or other mutable fields), updating logic around existing, cond, needsUpdate and the call to cond.WithLastTransitionTime(existing.LastTransitionTime) accordingly.pkg/operatorstatus/controller_status_test.go (1)
91-166: Add regression tests for nil-error constructor inputs.Please add cases for
testResultGenerator.Error(nil)andtestResultGenerator.NonRetryableError(nil)so nil-input behavior is explicit and panic regressions are caught.Also applies to: 197-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operatorstatus/controller_status_test.go` around lines 91 - 166, Add two regression subtests that call testResultGenerator.Error(nil) and testResultGenerator.NonRetryableError(nil) to ensure the constructors handle nil without panicking; for each call assert that the function returns without panic (use g.Expect(func(){ ... }).NotTo(Panic()) or just invoke) and that result.err is nil and the returned progressing/degraded condition objects are well-formed (non-nil) — reference the test helpers testResultGenerator.Error and testResultGenerator.NonRetryableError and mirror the style of the existing subtests (similar to the "plain error" and "already-terminal error" cases) so nil-input behavior is explicit and covered (also add equivalent checks for the block referenced around the other tests noted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operatorstatus/controller_status.go`:
- Around line 280-283: The panic in mergeConditions must be replaced with proper
error propagation: change mergeConditions to return an error (e.g., error)
instead of panicking when a condition has nil required fields, construct a
descriptive error (including the cond value) and return it; update callers such
as WriteClusterOperatorConditions to handle and propagate that error (return it
up the reconcile path) so controller-runtime handles failures instead of
crashing; ensure signatures and call sites for mergeConditions and
WriteClusterOperatorConditions are updated to accept/return the error and adjust
any tests accordingly.
- Around line 165-176: Guard ControllerResultGenerator.Error and
ControllerResultGenerator.nonRetryableError against nil error inputs: at the
start of Error(err error, ...) and nonRetryableError(termErr error, ...), check
if the incoming error is nil and avoid calling .Error(); use a safe fallback
message (e.g. "nil error" or "unknown error") for any progressing/degraded
condition message arguments and for reason text instead of calling
err.Error()/termErr.Error(), and keep the err field on the returned
ReconcileResult as nil if the input was nil. Update both functions (references:
ControllerResultGenerator.Error and ControllerResultGenerator.nonRetryableError)
so no .Error() is invoked on a nil pointer.
In `@pkg/providerimages/manifestreader_test.go`:
- Around line 99-101: The test in manifestreader_test.go currently uses a
hardcoded Unix path for ProviderImageManifests.ManifestsPath; change it to build
a missing-file path using t.TempDir() (e.g. filepath.Join(t.TempDir(),
"manifests.yaml")) so the test is portable across platforms, and ensure the test
imports filepath if not already present and assigns the resulting path to
ManifestsPath when constructing the ProviderImageManifests instance.
---
Duplicate comments:
In `@cmd/capi-operator/main.go`:
- Around line 194-201: The block creating revision.RevisionController currently
logs the error with klog.Error and then returns it, causing duplicate logs
upstream; remove the klog.Error(err, "unable to create revision controller",
"controller", "RevisionController") call so the function simply returns
fmt.Errorf("unable to create revision controller: %w", err) when
(&revision.RevisionController{...}).SetupWithManager(mgr) fails (identify the
RevisionController struct instantiation and its SetupWithManager call to locate
the change).
In `@pkg/test/matchers/condition.go`:
- Around line 170-176: The Match method on ConditionMatcher retains transient
failure state across invocations; at the start of ConditionMatcher.Match, clear
any fields used to build FailureMessage/NegatedFailureMessage (reset the
transient failure-context fields on the struct) before dispatching to
matchSingleCondition or matchConditionSlice so stale state cannot leak into
later messages.
---
Nitpick comments:
In `@pkg/controllers/revision/revision_controller.go`:
- Around line 151-155: The nil-check guard for desiredRevision in mergeRevisions
is unreachable because generateDesiredRevision either returns a non-nil revision
or a non-nil reconcile result and reconcile short-circuits before
mergeRevisions; remove the unreachable block checking "if desiredRevision == nil
{ return apiRevisions, nil }" from the mergeRevisions function (or, if you
prefer an explicit contract, replace it with a defensive panic or return an
error such as errors.New("mergeRevisions called with nil desiredRevision") so
the failure is explicit), referencing the mergeRevisions function and the
desiredRevision value and ensuring callers (reconcile/generateDesiredRevision)
remain unchanged.
In `@pkg/operatorstatus/controller_status_test.go`:
- Around line 91-166: Add two regression subtests that call
testResultGenerator.Error(nil) and testResultGenerator.NonRetryableError(nil) to
ensure the constructors handle nil without panicking; for each call assert that
the function returns without panic (use g.Expect(func(){ ... }).NotTo(Panic())
or just invoke) and that result.err is nil and the returned progressing/degraded
condition objects are well-formed (non-nil) — reference the test helpers
testResultGenerator.Error and testResultGenerator.NonRetryableError and mirror
the style of the existing subtests (similar to the "plain error" and
"already-terminal error" cases) so nil-input behavior is explicit and covered
(also add equivalent checks for the block referenced around the other tests
noted).
In `@pkg/operatorstatus/controller_status.go`:
- Around line 294-299: The branch checking if existing.Message != *cond.Message
inside the block that already confirms existing.Status == *cond.Status &&
existing.Reason == *cond.Reason && existing.Message == *cond.Message is
unreachable; remove that inner if and its body (the conditional that sets
needsUpdate) and instead ensure needsUpdate is only set when fields that can
actually change differ (e.g., compare LastTransitionTime or other mutable
fields), updating logic around existing, cond, needsUpdate and the call to
cond.WithLastTransitionTime(existing.LastTransitionTime) accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (89)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.summanifests-gen/go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openapi/generated_openapi/zz_generated.openapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagecontentsourcepolicies.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_operator-lifecycle-manager_01_olms.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/acceptrisk.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/clusterversionstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/conditionalupdate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/conditionalupdaterisk.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/oidcprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenclaimvalidationcelrule.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenclaimvalidationrule.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenissuer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenuservalidationrule.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/update.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clustermonitoringspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/criocredentialproviderconfigspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/criocredentialproviderconfigstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusoperatorconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/config_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1alpha1/criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1alpha1/interface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/generic.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1alpha1/criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1alpha1/expansion_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/machinesetstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/machinestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/backupjobreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallercomponent.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallercomponentimage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallerrevision.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapispec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapistatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterversionoperator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterversionoperatorspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterversionoperatorstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/etcdbackup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/etcdbackupspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/etcdbackupstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/imagecontentsourcepolicy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/imagecontentsourcepolicyspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/olm.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/olmspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/olmstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/repositorydigestmirrors.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (26)
.golangci.ymlcmd/capi-controllers/main.gocmd/capi-operator/main.gocmd/crd-compatibility-checker/main.gocmd/machine-api-migration/main.goe2e/go.modgo.modmanifests-gen/go.modpkg/commoncmdoptions/commonoptions.gopkg/controllers/revision/helpers_test.gopkg/controllers/revision/revision_controller.gopkg/controllers/revision/revision_controller_test.gopkg/controllers/revision/suite_test.gopkg/operatorstatus/controller_status.gopkg/operatorstatus/controller_status_test.gopkg/operatorstatus/operator_status.gopkg/providerimages/manifestreader.gopkg/providerimages/manifestreader_test.gopkg/revisiongenerator/helpers_test.gopkg/revisiongenerator/revision.gopkg/revisiongenerator/revision_test.gopkg/revisiongenerator/transform.gopkg/revisiongenerator/transform_test.gopkg/test/envtest.gopkg/test/matchers/condition.gopkg/test/matchers/condition_test.go
💤 Files with no reviewable changes (1)
- pkg/operatorstatus/operator_status.go
🚧 Files skipped from review as they are similar to previous changes (11)
- .golangci.yml
- cmd/crd-compatibility-checker/main.go
- go.mod
- pkg/revisiongenerator/transform.go
- cmd/capi-controllers/main.go
- e2e/go.mod
- pkg/test/matchers/condition_test.go
- pkg/commoncmdoptions/commonoptions.go
- pkg/revisiongenerator/revision_test.go
- pkg/controllers/revision/revision_controller_test.go
- pkg/revisiongenerator/transform_test.go
JoelSpeed
reviewed
Feb 26, 2026
| func (r *RevisionController) buildComponentList(platform configv1.PlatformType) []providerimages.ProviderImageManifests { | ||
| // Iterate over only providers that have either no platform restriction, or | ||
| // match the current platform. | ||
| componentsByPlatform := func(yield func(providerimages.ProviderImageManifests) bool) { |
Contributor
There was a problem hiding this comment.
What is the benefit of using an iterator here instead of creating a list, then sorting it, then returning that list? (A more traditional go approach)
Contributor
Author
There was a problem hiding this comment.
I think this reads better. slices.SortedFunc() is in the standard library and takes an iterator. This cleanly separates the concerns of filtering and sorting. (It will read even better when the Go wonks realise that Map and Filter are table stakes in a standard library, but 🤷)
Iterators can't be 'traditional' in Go because they're still relatively new. They've been around for a long time elsewhere.
Meh. I wrote filter and put it next to SliceMap, which should also be in the standard library. That leaves just the relevant logic here. FWIW I also wrote out 'loop and append to a fresh slice'. It was 6 lines instead of 3, and the filter selector was not as easy to see at a glance.
| Expect(cl.Status().Update(ctx, clusterOperator)).To(Succeed()) | ||
|
|
||
| // Restart the manager with the same provider images | ||
| mgr = newManagerWrapper(defaultProviderImgs) |
Contributor
There was a problem hiding this comment.
Where is this manager stopped?
Contributor
Author
There was a problem hiding this comment.
I'll answer here even though I'm removing this, because I think the same is present elsewhere: it's in BeforeEach. Usually I DeferCleanup immediately on creation. However, where I've done this it's because I manually stopped the previous manager and the created a new one using the same shared variable.
I'm also not massively happy with how it reads, but I didn't spend the time to come up with anything better.
| originalRev1 := initialClusterAPI.Status.Revisions[0] | ||
|
|
||
| // Restart the manager with different provider images | ||
| mgr = newManagerWrapper(updatedProviderImgs) |
Contributor
There was a problem hiding this comment.
Would it be better to just call reconcile directly when you're doing tests where you don't want the manager running?
Contributor
Author
There was a problem hiding this comment.
I do want the manager running.
Until we introduce additional factors (like unmanaged CRDs), the manager will only ever produce a single, unchanging revision. There's no way to prompt it to create anything different without restarting it with a different set of provider images. These come from the release image and can't change, so we only pull them on startup.
In this case I need 2 revisions, so I need to run the manager twice with 2 different inputs.
I could just call Reconcile, but that embeds assumptions like we're going to complete the reconcile as a one-shot. I like to confine direct Reconcile() tests to only where they're needed.
mdbooth
force-pushed
the
revision-controller
branch
from
c95d753 to
d18d333
Compare
This replaces hard-coded deployment in CustomNoUpgrade and TechPreviewNoUpgrade.
Avoids a circular dependency issue in util
mdbooth
force-pushed
the
revision-controller
branch
from
6c1623e to
72fdae5
Compare
|
@mdbooth: This pull request references OCPCLOUD-3350 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
mdbooth
force-pushed
the
revision-controller
branch
from
72fdae5 to
8fb5f4e
Compare
Contributor
|
/lgtm |
|
Scheduling tests matching the |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed 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
Contributor
|
@mdbooth: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
openshift-merge-robot
added
the
needs-rebase
Contributor
|
PR needs rebase. DetailsInstructions 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the revision controller
TODO:
Summary by CodeRabbit
New Features
Refactor