OCPBUGS-85151: Re-enable serviceaccount-pull-secrets controller when registry managementState changes from Removed#8522
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
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/severity-moderate
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-85151, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
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:
📝 WalkthroughWalkthroughreconcileRegistryAndIngress now reads the OpenShiftControllerManagerConfig only when needed and toggles the "-openshift.io/serviceaccount-pull-secrets" disable token: it adds that entry when ImageRegistry.Spec.ManagementState == Removed (for supported platforms), removes it when managementState is not Removed but the token was present (restoring ["*"] if controllers become empty), and otherwise leaves the config unchanged. A helper isServiceAccountPullSecretsControllerDisabled and tests were added; slices import and a disable-token variable were introduced. Sequence Diagram(s)sequenceDiagram
participant H as HostedClusterConfigOperator
participant R as ImageRegistry
participant P as PlatformChecker
participant CM as OpenShiftControllerManager ConfigMap
participant K as Kubernetes API
H->>R: Read registryConfig.Spec.ManagementState
H->>P: Check platform (skip if IBMCloud/Azure)
P-->>H: Platform allowed
H->>CM: Get current controller-manager config
CM-->>H: Return controllers list
alt managementState == Removed
H->>CM: Ensure "-openshift.io/serviceaccount-pull-secrets" present
else managementState != Removed and controller disabled
H->>CM: Remove "-openshift.io/serviceaccount-pull-secrets" (restore ["*"] if empty)
else
H-->>CM: No change
end
H->>K: Update ConfigMap if changed
K-->>H: Acknowledge update
Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
do-not-merge/needs-area
area/control-plane-operator
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-85151, which is valid. 3 validation(s) were run on this bug
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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8522 +/- ##
==========================================
- Coverage 40.13% 40.12% -0.01%
==========================================
Files 753 753
Lines 93045 93012 -33
==========================================
- Hits 37345 37325 -20
+ Misses 53003 52989 -14
- Partials 2697 2698 +1
... and 19 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (1)
644-652: 💤 Low valueConsider selectively removing only the
-serviceaccount-pull-secretsentry instead of clearing the entireControllersslice.When re-enabling the controller (line 649),
config.Controllers = nildiscards every entry in the slice. The codebase analysis shows that only HCCO and CPO write to this field, and CPO's logic preserves the state it previously set. In the current implementation, this is safe because no other code paths introduce additional disabled-controller entries.However, for defensive programming, consider removing only the
-openshift.io/serviceaccount-pull-secretselement to keep the operation symmetric with the targeted nature of the disable logic. This guards against future changes that might add other entries to the slice.The CPO code (config.go line 41-44) includes explicit preservation logic with the comment "Preserve any existing Controllers configuration," suggesting the intent to allow modifications; selective removal would align better with that design.
🤖 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 `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go` around lines 644 - 652, Instead of clearing the entire Controllers slice when re-enabling, remove only the disabled entry for the serviceaccount-pull-secrets controller: find and delete the string fmt.Sprintf("-%s", openshiftcpv1.OpenShiftServiceAccountPullSecretsController) from config.Controllers (preserving any other entries), and if the resulting slice is empty set config.Controllers = nil; keep using isServiceAccountPullSecretsControllerDisabled to detect the disabled state and update the block that currently sets config.Controllers = nil to perform this selective removal.
🤖 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
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`:
- Around line 644-652: Instead of clearing the entire Controllers slice when
re-enabling, remove only the disabled entry for the serviceaccount-pull-secrets
controller: find and delete the string fmt.Sprintf("-%s",
openshiftcpv1.OpenShiftServiceAccountPullSecretsController) from
config.Controllers (preserving any other entries), and if the resulting slice is
empty set config.Controllers = nil; keep using
isServiceAccountPullSecretsControllerDisabled to detect the disabled state and
update the block that currently sets config.Controllers = nil to perform this
selective removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0f9764f4-0751-4d1f-8162-1f28226e6adf
📒 Files selected for processing (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
vsolanki12
force-pushed
the
OCPBUGS-85151-hcco-re-enable-pull-secrets-controller
branch
from
aa804b9 to
e75b62a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go (2)
3650-3650: ⚡ Quick winAdd
t.Parallel()for consistent parallel test execution.Other test functions in this file use
t.Parallel()(e.g.,TestReconcileOLMat line 237,TestReconcileKubeadminPasswordHashSecretat line 485). This pure unit test has no shared state or dependencies that would prevent parallel execution, so addingt.Parallel()would improve test performance and maintain consistency.⚡ Suggested enhancement
func TestIsServiceAccountPullSecretsControllerDisabled(t *testing.T) { + t.Parallel() tests := []struct {🤖 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 `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go` at line 3650, The test function TestIsServiceAccountPullSecretsControllerDisabled lacks t.Parallel(); add a call to t.Parallel() as the first statement inside that function to allow it to run concurrently with other unit tests (same pattern used in TestReconcileOLM and TestReconcileKubeadminPasswordHashSecret), ensuring there is no shared state or setup that would conflict with parallel execution.
3656-3681: 💤 Low valueConsider adding test case for disabled entry without wildcard.
The current test cases cover the disabled state with both wildcard and disabled entry (line 3668), but don't test the edge case where only the disabled entry exists without the wildcard:
["-openshift.io/serviceaccount-pull-secrets"]. While this scenario is unlikely in real-world OpenShift configurations, adding it would provide more thorough unit test coverage of the helper function's behavior.📝 Optional test case
{ name: "When only the wildcard is present, it should return false", controllers: []string{"*"}, expected: false, }, + { + name: "When only the disabled entry is present without wildcard, it should return true", + controllers: []string{"-openshift.io/serviceaccount-pull-secrets"}, + expected: true, + }, }🤖 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 `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go` around lines 3656 - 3681, Add a new table-driven test case to the existing tests in resources_test.go: add an entry with name like "When only the disabled entry is present, it should return true", controllers set to []string{"-openshift.io/serviceaccount-pull-secrets"}, and expected true; place it alongside the other cases in the same test table so the helper that reads the controllers slice is exercised for the scenario without a wildcard.
🤖 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
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go`:
- Line 3650: The test function TestIsServiceAccountPullSecretsControllerDisabled
lacks t.Parallel(); add a call to t.Parallel() as the first statement inside
that function to allow it to run concurrently with other unit tests (same
pattern used in TestReconcileOLM and TestReconcileKubeadminPasswordHashSecret),
ensuring there is no shared state or setup that would conflict with parallel
execution.
- Around line 3656-3681: Add a new table-driven test case to the existing tests
in resources_test.go: add an entry with name like "When only the disabled entry
is present, it should return true", controllers set to
[]string{"-openshift.io/serviceaccount-pull-secrets"}, and expected true; place
it alongside the other cases in the same test table so the helper that reads the
controllers slice is exercised for the scenario without a wildcard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f07389d3-c64d-4d7f-84ce-3a9fef19fe36
📒 Files selected for processing (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
vsolanki12
force-pushed
the
OCPBUGS-85151-hcco-re-enable-pull-secrets-controller
branch
from
e75b62a to
b984ee2
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Pre-commit review (Go + HyperShift profile). Overall: PASS WITH RECOMMENDATIONS — the fix is correct and does not introduce new architectural violations. See inline comments for specific suggestions.
| } else if isServiceAccountPullSecretsControllerDisabled(config.Controllers) { | ||
| log.Info("imageregistry operator managementstate is no longer removed, re-enabling serviceaccount-pull-secrets controller") | ||
| config.Controllers = nil | ||
| } else { |
There was a problem hiding this comment.
[Medium] Consider using []string{"*"} instead of nil here.
The Controllers field is tagged json:"controllers" (no omitempty), so nil serializes to "controllers": null. Whether OCM interprets null as "use default (all controllers)" vs "empty list (run nothing)" is ambiguous.
Using []string{"*"} is explicit and unambiguous — it says "run all on-by-default controllers," which is the documented default and the intent here.
This also makes the CPO interaction in adaptConfig deterministic: with nil, CPO's len(existingControllers) > 0 is false so it enters neither branch and relies on the base config default. With ["*"], CPO explicitly preserves it via the else if branch.
There was a problem hiding this comment.
Thanks @bryan-cox ,
Done. Changed to selective removal of the disabled entry, falling back to []string{"*"} when the remaining list is empty.
| config.Controllers = nil | ||
| } else { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
[Low — readability] This return nil without mutation is correct — CreateOrUpdate deep-compares before/after and skips the API write when nothing changed. But the intent is non-obvious to readers unfamiliar with the upsert framework. A short comment would help:
} else {
// No change needed; returning nil without mutation causes CreateOrUpdate to skip the update.
return nil
}There was a problem hiding this comment.
Done added. // No change needed; returning nil without mutation causes CreateOrUpdate to skip the update.
|
|
||
| // isServiceAccountPullSecretsControllerDisabled checks if the ServiceAccountPullSecretsController is disabled. | ||
| func isServiceAccountPullSecretsControllerDisabled(controllers []string) bool { | ||
| disabled := fmt.Sprintf("-%s", openshiftcpv1.OpenShiftServiceAccountPullSecretsController) |
There was a problem hiding this comment.
[Low — DRY] This fmt.Sprintf("-%s", openshiftcpv1.OpenShiftServiceAccountPullSecretsController) also appears at line ~647 in the reconcile logic. Consider extracting to a package-level var:
var disabledServiceAccountPullSecretsController = fmt.Sprintf("-%s", openshiftcpv1.OpenShiftServiceAccountPullSecretsController)Then use it in both the slice literal and this helper. The duplication is low-risk since it's derived from a typed constant, but extracting it makes the relationship between the two usages explicit.
There was a problem hiding this comment.
Extracted var disabledServiceAccountPullSecretsController and used it in both the reconcile logic and the helper function.
| @@ -3646,3 +3646,45 @@ func TestCleanupLegacyResources(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
There was a problem hiding this comment.
[Medium — test gap] The helper function tests are well-structured (table-driven, Gherkin names, gomega) — nice work. However, the core behavioral change — the bidirectional toggling in reconcileRegistryAndIngress — has no test coverage. The re-enable path (Removed → Managed) is the entire point of this PR.
Consider adding a test that:
- Sets up a reconciler with a
cpClientcontaining an OCM ConfigMap where the controller is already disabled (Controllers: ["*", "-openshift.io/serviceaccount-pull-secrets"]) - Sets
registryConfig.Spec.ManagementState = Managed - Calls the reconcile path
- Asserts
config.Controllersis reset (to["*"]or nil depending on chosen approach)
Also covers the idempotent no-op case: ManagementState = Managed with controller already enabled → no ConfigMap update.
There was a problem hiding this comment.
Done. Added TestReconcileRegistryAndIngress_ServiceAccountPullSecretsController with 4 test cases: disable when Removed, re-enable when Managed, no-op when already enabled, and platform exclusion for IBMCloud.
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| g := NewWithT(t) |
There was a problem hiding this comment.
[Low] Nit: subtests are independent and could benefit from t.Parallel() inside each t.Run callback, consistent with Go best practices for table-driven tests.
There was a problem hiding this comment.
Done. Added t.Parallel() inside each t.Run callback.
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
Thanks for the fix — the bidirectional logic is a good improvement. Left a couple of comments.
| config.Controllers = []string{"*", fmt.Sprintf("-%s", openshiftcpv1.OpenShiftServiceAccountPullSecretsController)} | ||
| } else if isServiceAccountPullSecretsControllerDisabled(config.Controllers) { | ||
| log.Info("imageregistry operator managementstate is no longer removed, re-enabling serviceaccount-pull-secrets controller") | ||
| config.Controllers = nil |
There was a problem hiding this comment.
Setting config.Controllers = nil is fragile — it would wipe out any other controller overrides that might exist in the list, not just the -openshift.io/serviceaccount-pull-secrets entry. Instead, consider removing only the specific disable entry (e.g., -openshift.io/serviceaccount-pull-secrets) from the slice, and then set Controllers = nil only if the remaining list is just ["*"] (or empty).
There was a problem hiding this comment.
Thank you @csrwng,
Done. Now removes only the -openshift.io/serviceaccount-pull-secrets entry from the slice, preserving any other controller overrides. Falls back to []string{"*"} if the remaining list is empty.
| } | ||
| } | ||
|
|
||
| func TestIsServiceAccountPullSecretsControllerDisabled(t *testing.T) { |
There was a problem hiding this comment.
nit: The helper function is well-tested, but it would be good to also have reconciliation-level tests covering the full reconcileRegistryAndIngress flow — e.g., verifying that when managementState transitions from Removed to Managed, the OCM ConfigMap's Controllers field is correctly updated, and conversely that it stays untouched when the state hasn't changed.
There was a problem hiding this comment.
Done. Added TestReconcileRegistryAndIngress_ServiceAccountPullSecretsController with 4 test cases: disable when Removed, re-enable when Managed, no-op when already enabled, and platform exclusion for IBMCloud.
vsolanki12
force-pushed
the
OCPBUGS-85151-hcco-re-enable-pull-secrets-controller
branch
from
b984ee2 to
1259fec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`:
- Around line 647-650: The current branch that handles
registryConfig.Spec.ManagementState == operatorv1.Removed overwrites
config.Controllers with only "*" and
disabledServiceAccountPullSecretsController, dropping any existing overrides;
instead, modify the logic in the block that sets config.Controllers so it
preserves existing entries and only ensures the
disabledServiceAccountPullSecretsController
("-openshift.io/serviceaccount-pull-secrets") is present when absent: check
config.Controllers (using isServiceAccountPullSecretsControllerDisabled helper
or similar) and append disabledServiceAccountPullSecretsController if not
already present, keeping all other entries intact rather than replacing the
entire slice.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0048244b-e3c4-45c4-8ec9-baaa0464b946
📒 Files selected for processing (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
vsolanki12
force-pushed
the
OCPBUGS-85151-hcco-re-enable-pull-secrets-controller
branch
from
1259fec to
9caba92
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (1)
647-650:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve existing OCM controller overrides when disabling pull-secrets.
This branch overwrites
config.Controllersand drops unrelated existing overrides. Add onlydisabledServiceAccountPullSecretsControllerwhen absent.Suggested fix
- if registryConfig.Spec.ManagementState == operatorv1.Removed { - log.Info("imageregistry operator managementstate is removed, disabling serviceaccount-pull-secrets controller") - config.Controllers = []string{"*", disabledServiceAccountPullSecretsController} + if registryConfig.Spec.ManagementState == operatorv1.Removed { + if isServiceAccountPullSecretsControllerDisabled(config.Controllers) { + // No change needed; returning nil without mutation causes CreateOrUpdate to skip the update. + return nil + } + log.Info("imageregistry operator managementstate is removed, disabling serviceaccount-pull-secrets controller") + if len(config.Controllers) == 0 { + config.Controllers = []string{"*", disabledServiceAccountPullSecretsController} + } else { + config.Controllers = append(config.Controllers, disabledServiceAccountPullSecretsController) + }🤖 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 `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go` around lines 647 - 650, When registryConfig.Spec.ManagementState == operatorv1.Removed we must not overwrite config.Controllers (which loses other OCM overrides); instead check config.Controllers and if disabledServiceAccountPullSecretsController is not present (use isServiceAccountPullSecretsControllerDisabled or its inverse) append it to config.Controllers (or create a new slice preserving existing entries) so only the pull-secrets controller is disabled while keeping other controller overrides intact; update the branch that currently sets config.Controllers = []string{"*", disabledServiceAccountPullSecretsController} to perform this append/merge logic using the existing symbols config.Controllers and disabledServiceAccountPullSecretsController.
🤖 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.
Duplicate comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`:
- Around line 647-650: When registryConfig.Spec.ManagementState ==
operatorv1.Removed we must not overwrite config.Controllers (which loses other
OCM overrides); instead check config.Controllers and if
disabledServiceAccountPullSecretsController is not present (use
isServiceAccountPullSecretsControllerDisabled or its inverse) append it to
config.Controllers (or create a new slice preserving existing entries) so only
the pull-secrets controller is disabled while keeping other controller overrides
intact; update the branch that currently sets config.Controllers = []string{"*",
disabledServiceAccountPullSecretsController} to perform this append/merge logic
using the existing symbols config.Controllers and
disabledServiceAccountPullSecretsController.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: af70d034-39ef-4a29-b03e-28c26b5e93e8
📒 Files selected for processing (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
vsolanki12
force-pushed
the
OCPBUGS-85151-hcco-re-enable-pull-secrets-controller
branch
from
9caba92 to
ce93fe0
Compare
…stry managementState changes from Removed The HCCO previously only disabled the serviceaccount-pull-secrets controller in openshift-controller-manager when the image registry managementState was set to Removed. It had no logic to re-enable the controller when managementState changed back to Managed. Combined with CPO's preservation of existing controller overrides, this caused the controller to remain permanently disabled. This change makes the logic bidirectional: when managementState is no longer Removed and the controller is currently disabled, it clears the controller override to re-enable it. Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
vsolanki12
force-pushed
the
OCPBUGS-85151-hcco-re-enable-pull-secrets-controller
branch
from
ce93fe0 to
53df980
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go (2)
3803-3807: ⚡ Quick winMake reconcile assertions strict to avoid false positives.
Filtering only one error substring can let unrelated regressions pass. This test should assert no reconcile errors for the scoped scenario.
♻️ Suggested tightening
errs := r.reconcileRegistryAndIngress(t.Context(), hcp, log) -for _, e := range errs { - g.Expect(e.Error()).ToNot(ContainSubstring("openshift-controller-manager config"), "unexpected OCM config error: %v", e) -} +g.Expect(errs).To(BeEmpty(), "unexpected reconcile errors")🤖 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 `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go` around lines 3803 - 3807, The test currently calls r.reconcileRegistryAndIngress(...) and only filters errors that contain "openshift-controller-manager config", which can hide other regressions; replace the loop and substring check on the returned errs with a strict assertion that errs is empty (e.g., assert that errs has length 0 or use BeEmpty) after calling r.reconcileRegistryAndIngress, referencing the same call site and the errs variable so any unexpected reconcile errors will fail the test.
3713-3753: ⚡ Quick winAdd override-preservation test cases for controller list mutations.
The table currently validates basic enable/disable transitions, but it doesn’t lock in behavior for unrelated existing overrides. Add cases that verify
-openshift.io/serviceaccount-pull-secretsis toggled without altering other entries.♻️ Suggested test additions
tests := []struct { name string platformType hyperv1.PlatformType managementState operatorv1.ManagementState existingOCMControllers []string hasExistingOCMConfig bool expectedControllers []string }{ + { + name: "When managementState is Removed, it should preserve existing overrides while adding disable token", + platformType: hyperv1.AWSPlatform, + managementState: operatorv1.Removed, + existingOCMControllers: []string{"*", "-some-other-controller"}, + hasExistingOCMConfig: true, + expectedControllers: []string{"*", "-some-other-controller", disabledServiceAccountPullSecretsController}, + }, + { + name: "When managementState is Managed, it should remove only the pull-secrets disable token", + platformType: hyperv1.AWSPlatform, + managementState: operatorv1.Managed, + existingOCMControllers: []string{"*", "-some-other-controller", disabledServiceAccountPullSecretsController}, + hasExistingOCMConfig: true, + expectedControllers: []string{"*", "-some-other-controller"}, + },As per coding guidelines, "Unit test any code changes and additions, and include e2e tests when changes impact consumer behavior."
🤖 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 `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go` around lines 3713 - 3753, Add unit test rows to the existing tests table that assert toggling of disabledServiceAccountPullSecretsController does not modify unrelated controller entries: add one case where existingOCMControllers includes another custom override (e.g., "custom-controller") plus disabledServiceAccountPullSecretsController and managementState Managed expecting expectedControllers to include "custom-controller" (and not the disabled entry), and another case where existingOCMControllers includes "custom-controller" without the disabled entry and managementState Removed expecting expectedControllers to include both "custom-controller" and disabledServiceAccountPullSecretsController; update the tests slice entries that use platformType, managementState, existingOCMControllers, hasExistingOCMConfig and expectedControllers so the controller-list mutation logic in the code paths exercised by the test (the logic that reads/modifies OCM controller lists) is validated for preservation of unrelated overrides.
🤖 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
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go`:
- Around line 3803-3807: The test currently calls
r.reconcileRegistryAndIngress(...) and only filters errors that contain
"openshift-controller-manager config", which can hide other regressions; replace
the loop and substring check on the returned errs with a strict assertion that
errs is empty (e.g., assert that errs has length 0 or use BeEmpty) after calling
r.reconcileRegistryAndIngress, referencing the same call site and the errs
variable so any unexpected reconcile errors will fail the test.
- Around line 3713-3753: Add unit test rows to the existing tests table that
assert toggling of disabledServiceAccountPullSecretsController does not modify
unrelated controller entries: add one case where existingOCMControllers includes
another custom override (e.g., "custom-controller") plus
disabledServiceAccountPullSecretsController and managementState Managed
expecting expectedControllers to include "custom-controller" (and not the
disabled entry), and another case where existingOCMControllers includes
"custom-controller" without the disabled entry and managementState Removed
expecting expectedControllers to include both "custom-controller" and
disabledServiceAccountPullSecretsController; update the tests slice entries that
use platformType, managementState, existingOCMControllers, hasExistingOCMConfig
and expectedControllers so the controller-list mutation logic in the code paths
exercised by the test (the logic that reads/modifies OCM controller lists) is
validated for preservation of unrelated overrides.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e364d9b7-1b2e-4fb1-9ce8-da89868c831c
📒 Files selected for processing (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
|
/retest |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, vsolanki12 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
|
/retest |
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aks
e2e-aws
|
|
/retest |
|
/verified by me on local env |
openshift-ci-robot
added
the
verified
|
@vsolanki12: This PR has been marked as verified by 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. |
|
Before fix: After fix: |
|
@vsolanki12: all tests passed! 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. |
|
/retest |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryBoth Enterprise Contract check failures are pre-existing infrastructure issues completely unrelated to PR #8522's code changes. The PR only modifies two Go source files in Root CauseThe root cause is a combination of two issues: 1. Pre-existing build pipeline test failures (persistent across ALL builds):
2. EC policy change in treatment of test failures: The Enterprise Contract policies evaluate these task
The same three task failures are treated as warnings in some EC evaluations but failures in others. The 6 EC policy failures correspond to 3 failing tasks × 2 EC policy rules each (likely The EC integration test scenarios ( Recommendations
Evidence
|
5 participants
What this PR does / why we need it:
When the image registry
managementStateis set toRemoved, HCCO disables theserviceaccount-pull-secretscontroller in openshift-controller-manager by adding a-openshift.io/serviceaccount-pull-secretsoverride to the OCM ConfigMap. However, the logic was one-way — there was no code path to re-enable the controller whenmanagementStatechanged back toManaged. Combined with CPO's preservation of existing controller overrides (PR #8072), this caused the controller to remain permanently disabled.This change makes the logic bidirectional: when
managementStateis no longerRemovedand the controller is currently disabled, it clears the controller override to re-enable it.Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/OCPBUGS-85151
Special notes for your reviewer:
Tested manually on a self-managed HyperShift cluster (AWS, release 4.22.0-rc.3). Verified:
managementState: Removeddisables the controller (log: "disabling serviceaccount-pull-secrets controller")managementState: Managedre-enables the controller (log: "re-enabling serviceaccount-pull-secrets controller", controllers reset to null)Checklist:
Summary by CodeRabbit
Bug Fixes
Tests