CM-1040: Extract shared ApplyResource helper and migrate istiocsr to SSA#420
Open
sebrandon1 wants to merge 1 commit into
Open
CM-1040: Extract shared ApplyResource helper and migrate istiocsr to SSA#420sebrandon1 wants to merge 1 commit into
sebrandon1 wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sebrandon1: This pull request references CM-1040 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 task to target the "5.0.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. |
WalkthroughThis PR adds a shared Server-Side Apply helper and routes IstioCSR and TrustManager reconciliation for multiple Kubernetes resources through it. Tests now validate patch-based behavior and standardized error messages. ChangesShared SSA reconciliation
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/controller/istiocsr/install_instiocsr_test.go (1)
68-84: ⚡ Quick winPreserve generated-name simulation for
ClusterRolein these fakes.
createOrApplyClusterRoles()still depends onCreatemutating the object with a generated name before status is written and before theClusterRoleBindinggets itsRoleRef.Name. These stubs only backfillClusterRoleBinding, so the test can still pass even if the role name is left empty. I'd add a*rbacv1.ClusterRolebranch in eachCreateCallsblock as well.Representative tweak
m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { switch o := obj.(type) { + case *rbacv1.ClusterRole: + role := testClusterRole() + role.DeepCopyInto(o) case *appsv1.Deployment: if !reflect.DeepEqual(o.GetLabels(), labels) { return fmt.Errorf("labels mismatch in %v resource; got: %v, want: %v", o, o.GetLabels(), labels) }Also applies to: 110-117, 131-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/istiocsr/install_instiocsr_test.go` around lines 68 - 84, The CreateCalls test stubs need to simulate Create mutating a ClusterRole with a generated name so createOrApplyClusterRoles() sees a non-empty Role.Name; update the CreateCalls handlers (the ones that currently switch over *appsv1.Deployment and *rbacv1.ClusterRoleBinding) to also include a case for *rbacv1.ClusterRole that sets o.Name to a generated value (e.g., append "-generated" or a deterministic string) and preserves labels so subsequent logic that reads the ClusterRole's name (and the ClusterRoleBinding RoleRef.Name) behaves as in real Create; apply this same addition to the other CreateCalls blocks mentioned.pkg/controller/trustmanager/webhooks_test.go (1)
207-221: ⚡ Quick winKeep the error assertion specific to the webhook config.
ApplyResourcestill includes the resource name in the returned error, so shortening these expectations to justfailed to ... resourcemakes this table much less discriminating. A wrong object name or an extra apply call in this path would still satisfy the assertion. I'd keep the expected substring specific totrustManagerWebhookConfigNamehere, and mirror that in the other migrated reconciliation tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/trustmanager/webhooks_test.go` around lines 207 - 221, The test's error expectation is too generic; update the table entries (the case with name "patch error propagates" and similar migrated reconciliation tests) to assert the error message includes the specific webhook resource name by matching the substring that contains trustManagerWebhookConfigName (the Reconciler's ApplyResource error includes the resource name), e.g. expect the returned error to contain trustManagerWebhookConfigName along with "failed to apply resource", and adjust other tests that assert "failed to check if resource" / "failed to apply resource" to similarly include trustManagerWebhookConfigName so the assertions target the specific webhook config rather than any resource.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/common/applier.go`:
- Around line 33-36: The resourceName construction in applier.go currently uses
fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) which yields
"/name" for cluster-scoped objects; change the logic to detect an empty
namespace (desired.GetNamespace() == "") and build resourceName as just
desired.GetName(), otherwise build it as namespace + "/" + name so
cluster-scoped resources do not get a leading slash; update any use of the
existing resourceName variable accordingly.
---
Nitpick comments:
In `@pkg/controller/istiocsr/install_instiocsr_test.go`:
- Around line 68-84: The CreateCalls test stubs need to simulate Create mutating
a ClusterRole with a generated name so createOrApplyClusterRoles() sees a
non-empty Role.Name; update the CreateCalls handlers (the ones that currently
switch over *appsv1.Deployment and *rbacv1.ClusterRoleBinding) to also include a
case for *rbacv1.ClusterRole that sets o.Name to a generated value (e.g., append
"-generated" or a deterministic string) and preserves labels so subsequent logic
that reads the ClusterRole's name (and the ClusterRoleBinding RoleRef.Name)
behaves as in real Create; apply this same addition to the other CreateCalls
blocks mentioned.
In `@pkg/controller/trustmanager/webhooks_test.go`:
- Around line 207-221: The test's error expectation is too generic; update the
table entries (the case with name "patch error propagates" and similar migrated
reconciliation tests) to assert the error message includes the specific webhook
resource name by matching the substring that contains
trustManagerWebhookConfigName (the Reconciler's ApplyResource error includes the
resource name), e.g. expect the returned error to contain
trustManagerWebhookConfigName along with "failed to apply resource", and adjust
other tests that assert "failed to check if resource" / "failed to apply
resource" to similarly include trustManagerWebhookConfigName so the assertions
target the specific webhook config rather than any resource.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 683e1b36-07c6-463b-8cd3-d29a46add8d5
📒 Files selected for processing (26)
pkg/controller/common/applier.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/constants.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/install_istiocsr.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services.gopkg/controller/istiocsr/services_test.gopkg/controller/trustmanager/certificates.gopkg/controller/trustmanager/certificates_test.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/deployments.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/rbacs.gopkg/controller/trustmanager/rbacs_test.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/serviceaccounts_test.gopkg/controller/trustmanager/services.gopkg/controller/trustmanager/services_test.gopkg/controller/trustmanager/webhooks.gopkg/controller/trustmanager/webhooks_test.go
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
May 6, 2026 18:48
ca8b171 to
fdd735e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controller/istiocsr/serviceaccounts_test.go (1)
22-25: ⚡ Quick winCover the no-op branch explicitly.
Now that this table has
assertCalls, the"serviceaccount reconciliation successful"case should also assertPatchCallCount() == 0. Right now that case still passes ifApplyResourcestarts patching unchangedServiceAccounts on every reconcile, which is one of the main regression risks in this SSA migration.Suggested assertion
{ name: "serviceaccount reconciliation successful", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { switch o := obj.(type) { case *corev1.ServiceAccount: serviceaccount := testServiceAccount() serviceaccount.DeepCopyInto(o) } return true, nil }) }, + assertCalls: func(t *testing.T, mock *fakes.FakeCtrlClient) { + if mock.PatchCallCount() != 0 { + t.Errorf("createOrApplyServiceAccounts() Patch call count: %d, want 0", mock.PatchCallCount()) + } + }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/istiocsr/serviceaccounts_test.go` around lines 22 - 25, Update the "serviceaccount reconciliation successful" testcase in the table-driven test in serviceaccounts_test.go to explicitly assert that no patch operations occurred: inside its assertCalls function, call mock.PatchCallCount() and require it equals 0 (on the provided *fakes.FakeCtrlClient) to ensure ApplyResource did not issue patches for unchanged ServiceAccounts; keep other existing assertions and reference the Reconciler and ApplyResource behavior as context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 265-269: createOrApplyRoleBindings and
createOrApplyRoleBindingForLeases currently always call common.ApplyResource
(Server-Side Apply) but RoleBinding.roleRef is immutable—mirror the
ClusterRoleBinding handler's pattern: after calling common.ApplyResource (with
hasObjectChanged from getRoleBindingObject/hasObjectChanged), detect when the
failure or diff indicates only a roleRef drift (compare desired.RoleRef to the
live object.RoleRef), then delete the existing rbacv1.RoleBinding and recreate
the desired object (preserving owner refs/events/fieldOwner) as a fallback;
implement this delete-then-create flow in both createOrApplyRoleBindings and
createOrApplyRoleBindingForLeases using the same logic used in the
ClusterRoleBinding handler to avoid permanent SSA validation errors.
---
Nitpick comments:
In `@pkg/controller/istiocsr/serviceaccounts_test.go`:
- Around line 22-25: Update the "serviceaccount reconciliation successful"
testcase in the table-driven test in serviceaccounts_test.go to explicitly
assert that no patch operations occurred: inside its assertCalls function, call
mock.PatchCallCount() and require it equals 0 (on the provided
*fakes.FakeCtrlClient) to ensure ApplyResource did not issue patches for
unchanged ServiceAccounts; keep other existing assertions and reference the
Reconciler and ApplyResource behavior as context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3367fe15-60ea-4966-a17e-6222e23128a6
📒 Files selected for processing (26)
pkg/controller/common/applier.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/constants.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/install_istiocsr.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services.gopkg/controller/istiocsr/services_test.gopkg/controller/trustmanager/certificates.gopkg/controller/trustmanager/certificates_test.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/deployments.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/rbacs.gopkg/controller/trustmanager/rbacs_test.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/serviceaccounts_test.gopkg/controller/trustmanager/services.gopkg/controller/trustmanager/services_test.gopkg/controller/trustmanager/webhooks.gopkg/controller/trustmanager/webhooks_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/trustmanager/certificates.go
- pkg/controller/trustmanager/certificates_test.go
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
May 14, 2026 19:07
fdd735e to
26c1bb6
Compare
Member
Author
|
/retest |
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
May 29, 2026 15:51
26c1bb6 to
8b48aaa
Compare
Member
Author
|
/retest |
Member
Author
|
/retest-required |
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
June 15, 2026 16:25
8b48aaa to
4442995
Compare
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
June 24, 2026 19:51
4442995 to
786945d
Compare
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
July 6, 2026 16:17
786945d to
81c1b58
Compare
Member
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Member
Author
|
Addressed CodeRabbit feedback on RoleBinding.roleRef immutability:
All tests pass with 85.2% coverage on istiocsr package. |
Member
Author
|
/retest |
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
July 13, 2026 17:58
d4919c6 to
edaac18
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/istiocsr/rbacs.go (1)
285-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared helper for RoleBinding
roleRefdrift/delete logic.
createOrApplyRoleBindingsandcreateOrApplyRoleBindingForLeasesduplicate ~18 nearly-identical lines (Exists check,rbacRoleBindingRefModifiedcheck, log, Delete with NotFound tolerance). Given this PR's stated goal of removing duplicated boilerplate, extract this into a shared helper, e.g.deleteRoleBindingIfRoleRefChanged(desired *rbacv1.RoleBinding) error.♻️ Proposed refactor
+func (r *Reconciler) deleteRoleBindingIfRoleRefChanged(desired *rbacv1.RoleBinding) error { + key := client.ObjectKeyFromObject(desired) + existing := &rbacv1.RoleBinding{} + exists, err := r.Exists(r.ctx, key, existing) + if err != nil { + return common.FromClientError(err, "failed to check if RoleBinding %q exists", key) + } + if exists && rbacRoleBindingRefModified(desired, existing) { + r.log.V(1).Info("rolebinding roleRef changed, deleting for recreation (roleRef is immutable)", "name", key) + if err := r.Delete(r.ctx, existing); err != nil && !apierrors.IsNotFound(err) { + return common.FromClientError(err, "failed to delete RoleBinding %q to replace roleRef", key) + } + } + return nil +} + func (r *Reconciler) createOrApplyRoleBindings(istiocsr *v1alpha1.IstioCSR, serviceAccount string, resourceLabels map[string]string) error { desired := r.getRoleBindingObject(serviceAccount, istiocsr.GetNamespace(), istiocsr.Spec.IstioCSRConfig.Istio.Namespace, resourceLabels) - - // RoleBinding.roleRef is immutable; if it has changed, delete the existing binding first - key := client.ObjectKeyFromObject(desired) - existing := &rbacv1.RoleBinding{} - exists, err := r.Exists(r.ctx, key, existing) - if err != nil { - return common.FromClientError(err, "failed to check if RoleBinding %q exists", key) - } - if exists && rbacRoleBindingRefModified(desired, existing) { - r.log.V(1).Info("rolebinding roleRef changed, deleting for recreation (roleRef is immutable)", "name", key) - if err := r.Delete(r.ctx, existing); err != nil { - if !apierrors.IsNotFound(err) { - return common.FromClientError(err, "failed to delete RoleBinding %q to replace roleRef", key) - } - } - } + if err := r.deleteRoleBindingIfRoleRefChanged(desired); err != nil { + return err + } return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &rbacv1.RoleBinding{}, fieldOwner, func(d, e *rbacv1.RoleBinding) bool { return hasObjectChanged(d, e) }, ) }Apply the analogous change to
createOrApplyRoleBindingForLeases.Also applies to: 331-352
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/istiocsr/rbacs.go` around lines 285 - 306, Extract the duplicated RoleBinding roleRef drift handling from createOrApplyRoleBindings and createOrApplyRoleBindingForLeases into a shared helper such as deleteRoleBindingIfRoleRefChanged, accepting the desired RoleBinding. Move the Exists check, rbacRoleBindingRefModified comparison, diagnostic log, deletion, and NotFound tolerance into that helper, then invoke it from both creation methods before applying the resource.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 285-306: Extract the duplicated RoleBinding roleRef drift handling
from createOrApplyRoleBindings and createOrApplyRoleBindingForLeases into a
shared helper such as deleteRoleBindingIfRoleRefChanged, accepting the desired
RoleBinding. Move the Exists check, rbacRoleBindingRefModified comparison,
diagnostic log, deletion, and NotFound tolerance into that helper, then invoke
it from both creation methods before applying the resource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a0bb173e-0351-4833-bd59-fa1f25c895cb
📒 Files selected for processing (26)
pkg/controller/common/applier.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/constants.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/install_istiocsr.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services.gopkg/controller/istiocsr/services_test.gopkg/controller/trustmanager/certificates.gopkg/controller/trustmanager/certificates_test.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/deployments.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/rbacs.gopkg/controller/trustmanager/rbacs_test.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/serviceaccounts_test.gopkg/controller/trustmanager/services.gopkg/controller/trustmanager/services_test.gopkg/controller/trustmanager/webhooks.gopkg/controller/trustmanager/webhooks_test.go
🚧 Files skipped from review as they are similar to previous changes (23)
- pkg/controller/common/applier.go
- pkg/controller/istiocsr/constants.go
- pkg/controller/istiocsr/certificates_test.go
- pkg/controller/trustmanager/serviceaccounts.go
- pkg/controller/istiocsr/certificates.go
- pkg/controller/trustmanager/deployments_test.go
- pkg/controller/trustmanager/deployments.go
- pkg/controller/istiocsr/install_istiocsr.go
- pkg/controller/istiocsr/serviceaccounts.go
- pkg/controller/trustmanager/serviceaccounts_test.go
- pkg/controller/trustmanager/webhooks_test.go
- pkg/controller/istiocsr/serviceaccounts_test.go
- pkg/controller/istiocsr/install_instiocsr_test.go
- pkg/controller/trustmanager/rbacs_test.go
- pkg/controller/trustmanager/services_test.go
- pkg/controller/trustmanager/certificates_test.go
- pkg/controller/trustmanager/certificates.go
- pkg/controller/trustmanager/webhooks.go
- pkg/controller/istiocsr/rbacs_test.go
- pkg/controller/trustmanager/rbacs.go
- pkg/controller/istiocsr/services.go
- pkg/controller/istiocsr/services_test.go
- pkg/controller/istiocsr/networkpolicies.go
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
July 17, 2026 18:55
edaac18 to
b919b00
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/istiocsr/install_instiocsr_test.go (1)
51-67: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the SSA patch contract, not only labels.
This stub accepts any patch type/options and does not assert the expected patched resource set. A regression to a non-SSA patch—or skipping a migrated resource—would still pass. Assert
client.Apply, the field-owner/force options, and expected Patch calls via the fake’s recorded arguments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/istiocsr/install_instiocsr_test.go` around lines 51 - 67, Update the PatchCalls assertion around the SSA-migrated resources to validate the full apply contract: require a client.Apply patch, the expected field-owner and force options, and exactly the expected Service, ServiceAccount, Certificate, Role, and RoleBinding patch calls using the fake’s recorded arguments. Preserve the existing label assertions while rejecting skipped resources or non-SSA patches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/controller/istiocsr/install_instiocsr_test.go`:
- Around line 51-67: Update the PatchCalls assertion around the SSA-migrated
resources to validate the full apply contract: require a client.Apply patch, the
expected field-owner and force options, and exactly the expected Service,
ServiceAccount, Certificate, Role, and RoleBinding patch calls using the fake’s
recorded arguments. Preserve the existing label assertions while rejecting
skipped resources or non-SSA patches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a104d795-c66e-4f2b-acf9-4a6a4f27e78f
📒 Files selected for processing (26)
pkg/controller/common/applier.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/constants.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/install_istiocsr.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services.gopkg/controller/istiocsr/services_test.gopkg/controller/trustmanager/certificates.gopkg/controller/trustmanager/certificates_test.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/deployments.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/rbacs.gopkg/controller/trustmanager/rbacs_test.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/serviceaccounts_test.gopkg/controller/trustmanager/services.gopkg/controller/trustmanager/services_test.gopkg/controller/trustmanager/webhooks.gopkg/controller/trustmanager/webhooks_test.go
🚧 Files skipped from review as they are similar to previous changes (22)
- pkg/controller/trustmanager/controller_test.go
- pkg/controller/trustmanager/serviceaccounts_test.go
- pkg/controller/common/applier.go
- pkg/controller/istiocsr/certificates.go
- pkg/controller/trustmanager/certificates_test.go
- pkg/controller/trustmanager/deployments_test.go
- pkg/controller/trustmanager/webhooks_test.go
- pkg/controller/trustmanager/serviceaccounts.go
- pkg/controller/trustmanager/services_test.go
- pkg/controller/istiocsr/certificates_test.go
- pkg/controller/trustmanager/certificates.go
- pkg/controller/trustmanager/services.go
- pkg/controller/istiocsr/networkpolicies.go
- pkg/controller/istiocsr/install_istiocsr.go
- pkg/controller/trustmanager/deployments.go
- pkg/controller/istiocsr/serviceaccounts_test.go
- pkg/controller/istiocsr/services.go
- pkg/controller/istiocsr/serviceaccounts.go
- pkg/controller/istiocsr/rbacs_test.go
- pkg/controller/istiocsr/services_test.go
- pkg/controller/trustmanager/webhooks.go
- pkg/controller/trustmanager/rbacs.go
Extract a generic ApplyResource[T] function for Server-Side Apply reconciliation and migrate istiocsr controllers to use it. Handle RoleBinding.roleRef immutability by deleting and recreating when the roleRef changes. Add direct unit tests for ApplyResource covering create, no-op, update, error handling, and error classification (irrecoverable vs retryable). Add networkpolicies_test.go with tests for asset decoding, namespace fallback, full apply, and error propagation.
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
July 17, 2026 19:00
b919b00 to
45acb13
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/common/applier_test.go (1)
47-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the SSA patch contract, not just the call count.
This stub discards
client.Apply,FieldOwner, andForceOwnership. A regression to a non-SSA patch or missing ownership options would still pass every test here. Capture the patch/options and assert them in a successful apply case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/common/applier_test.go` around lines 47 - 49, Update stubCtrlClient.Patch to retain the received client.Patch and PatchOption values, then extend the successful apply test assertions to verify client.Apply is used with the expected FieldOwner and ForceOwnership options, in addition to checking the patch count.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/controller/common/applier_test.go`:
- Around line 47-49: Update stubCtrlClient.Patch to retain the received
client.Patch and PatchOption values, then extend the successful apply test
assertions to verify client.Apply is used with the expected FieldOwner and
ForceOwnership options, in addition to checking the patch count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 37a2c255-13f8-4e8a-9a03-a2cad993605a
📒 Files selected for processing (28)
pkg/controller/common/applier.gopkg/controller/common/applier_test.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/constants.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/install_istiocsr.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/networkpolicies_test.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services.gopkg/controller/istiocsr/services_test.gopkg/controller/trustmanager/certificates.gopkg/controller/trustmanager/certificates_test.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/deployments.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/rbacs.gopkg/controller/trustmanager/rbacs_test.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/serviceaccounts_test.gopkg/controller/trustmanager/services.gopkg/controller/trustmanager/services_test.gopkg/controller/trustmanager/webhooks.gopkg/controller/trustmanager/webhooks_test.go
🚧 Files skipped from review as they are similar to previous changes (23)
- pkg/controller/trustmanager/serviceaccounts_test.go
- pkg/controller/istiocsr/constants.go
- pkg/controller/trustmanager/rbacs_test.go
- pkg/controller/istiocsr/install_istiocsr.go
- pkg/controller/common/applier.go
- pkg/controller/istiocsr/certificates_test.go
- pkg/controller/trustmanager/services.go
- pkg/controller/trustmanager/certificates_test.go
- pkg/controller/trustmanager/controller_test.go
- pkg/controller/trustmanager/webhooks_test.go
- pkg/controller/istiocsr/services_test.go
- pkg/controller/trustmanager/services_test.go
- pkg/controller/trustmanager/serviceaccounts.go
- pkg/controller/istiocsr/networkpolicies.go
- pkg/controller/istiocsr/install_instiocsr_test.go
- pkg/controller/trustmanager/webhooks.go
- pkg/controller/istiocsr/rbacs_test.go
- pkg/controller/istiocsr/services.go
- pkg/controller/istiocsr/certificates.go
- pkg/controller/trustmanager/deployments.go
- pkg/controller/istiocsr/serviceaccounts_test.go
- pkg/controller/trustmanager/certificates.go
- pkg/controller/trustmanager/rbacs.go
5 tasks
Contributor
|
@sebrandon1: 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. |
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
2 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.
Summary
common.ApplyResource[T]()helper for SSA-based reconciliation with pluggable drift detection callbackscreateOrApplymethods to use the shared helpercreateOrApplymethods from Create/UpdateWithRetry to SSA via the shared helperistioCSRCreateReconwarning events from migrated methods (SSA is inherently idempotent)Test plan
go test ./pkg/controller/common/...passesgo test ./pkg/controller/istiocsr/...passesgo test ./pkg/controller/trustmanager/...passesgo build ./...succeedsmake lintshows only pre-existing issues (9 total, none introduced)Summary by CodeRabbit
Refactoring
Bug Fixes
roleRefdrift (replaces the existing binding when needed).Tests