CNTRLPLANE-2789: add StatefulSet test case to controlplane-component reconcile tests#7764
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2789 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 "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. |
|
/auto-cc |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2789 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 "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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository: openshift/coderabbit/.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 selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new StatefulSet YAML asset for a test component, makes a small Deployment volume metadata change, and refactors tests to support both Deployment and StatefulSet workloads with per-kind constructors and a shared workload extraction. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @hypershift-jira-solve-ci[bot]. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
bot
added
area/control-plane-operator
|
/test unit |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2789 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 "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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/test-component/statefulset.yaml (1)
18-62: Add container securityContext to satisfy baseline pod‑security checks.This manifest currently omits container-level securityContext, which triggers CKV_K8S_20/23 and weakens the default posture. Even for test fixtures, it’s better to explicitly disable privilege escalation and enforce non‑root to avoid policy failures.
Suggested hardening
containers: - args: - --foo=bar command: - /usr/bin/test-component + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + capabilities: + drop: ["ALL"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/assets/test-component/statefulset.yaml` around lines 18 - 62, The container spec for the container named "test-component" in statefulset.yaml lacks a container-level securityContext; add a securityContext block under the container (the item with name: test-component) that explicitly sets allowPrivilegeEscalation: false and enforces non-root (e.g., runAsNonRoot: true and a safe runAsUser like 1000) to satisfy baseline pod-security checks and prevent privilege escalation.support/controlplane-component/controlplane-component_test.go (2)
261-262: Consider adding a nil guard forAffinitybefore accessing nested fields.Lines 261-262 access
result.podTemplate.Spec.Affinity.NodeAffinityand.PodAffinitywithout verifying thatAffinityis non-nil. If the reconciler fails to set affinity rules, this will panic rather than produce a clear assertion failure.Suggested improvement
// enforce affinity rules. + g.Expect(result.podTemplate.Spec.Affinity).NotTo(BeNil(), "affinity should be set") nodeAffinity := &corev1.NodeAffinity{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/controlplane-component/controlplane-component_test.go` around lines 261 - 262, The test accesses nested fields result.podTemplate.Spec.Affinity.NodeAffinity and .PodAffinity without verifying Affinity is non-nil; add a nil guard by asserting result.podTemplate.Spec.Affinity is not nil (e.g., g.Expect(result.podTemplate.Spec.Affinity).NotTo(BeNil())) before asserting on NodeAffinity and PodAffinity, then keep the existing equality checks for NodeAffinity and PodAffinity to avoid panics and produce clear test failures.
215-215: Consider adding a nil guard before dereferencingAutomountServiceAccountToken.Line 215 dereferences
result.podTemplate.Spec.AutomountServiceAccountTokenwithout a nil check. While the reconciler should always set this field, a defensive assertion would produce a clearer failure message if the field is unexpectedly nil.Suggested improvement
// enforce automount token sa is false. - g.Expect(*result.podTemplate.Spec.AutomountServiceAccountToken).To(BeFalse()) + g.Expect(result.podTemplate.Spec.AutomountServiceAccountToken).NotTo(BeNil(), "AutomountServiceAccountToken should be set") + g.Expect(*result.podTemplate.Spec.AutomountServiceAccountToken).To(BeFalse())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/controlplane-component/controlplane-component_test.go` at line 215, Add a nil-guard assertion before dereferencing AutomountServiceAccountToken in the test: first assert that result.podTemplate.Spec.AutomountServiceAccountToken is not nil (e.g. g.Expect(result.podTemplate.Spec.AutomountServiceAccountToken).ToNot(BeNil())), then assert its boolean value (g.Expect(*result.podTemplate.Spec.AutomountServiceAccountToken).To(BeFalse())). This ensures clearer test failures if AutomountServiceAccountToken is unexpectedly nil while keeping the existing check on its value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/test-component/statefulset.yaml`:
- Around line 18-62: The container spec for the container named "test-component"
in statefulset.yaml lacks a container-level securityContext; add a
securityContext block under the container (the item with name: test-component)
that explicitly sets allowPrivilegeEscalation: false and enforces non-root
(e.g., runAsNonRoot: true and a safe runAsUser like 1000) to satisfy baseline
pod-security checks and prevent privilege escalation.
In `@support/controlplane-component/controlplane-component_test.go`:
- Around line 261-262: The test accesses nested fields
result.podTemplate.Spec.Affinity.NodeAffinity and .PodAffinity without verifying
Affinity is non-nil; add a nil guard by asserting
result.podTemplate.Spec.Affinity is not nil (e.g.,
g.Expect(result.podTemplate.Spec.Affinity).NotTo(BeNil())) before asserting on
NodeAffinity and PodAffinity, then keep the existing equality checks for
NodeAffinity and PodAffinity to avoid panics and produce clear test failures.
- Line 215: Add a nil-guard assertion before dereferencing
AutomountServiceAccountToken in the test: first assert that
result.podTemplate.Spec.AutomountServiceAccountToken is not nil (e.g.
g.Expect(result.podTemplate.Spec.AutomountServiceAccountToken).ToNot(BeNil())),
then assert its boolean value
(g.Expect(*result.podTemplate.Spec.AutomountServiceAccountToken).To(BeFalse())).
This ensures clearer test failures if AutomountServiceAccountToken is
unexpectedly nil while keeping the existing check on its value.
hypershift-jira-solve-ci
bot
force-pushed
the
fix-CNTRLPLANE-2789
branch
from
1dd580d to
e73f81b
Compare
|
@coderabbitai Thanks for the nitpick suggestions — all three have been addressed:
AI-assisted response via Claude Code |
|
Addressed all three nitpick suggestions from the latest review:
All changes amended into the existing review-fixes commit. Tests pass. AI-assisted response via Claude Code |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2789 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 "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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/test-component/statefulset.yaml`:
- Around line 82-87: The ConfigMap volume definition for the volume named
"my-configmap" is missing the required configMap.name field; update the
configMap block under that volume (the volume with name: my-configmap) to
include a name: "<your-configmap-resource-name>" entry that points to the
ConfigMap resource to mount (e.g., add name: my-configmap or the actual
ConfigMap's metadata.name).
control-plane-operator/controllers/hostedcontrolplane/v2/assets/test-component/statefulset.yaml
Show resolved
Hide resolved
hypershift-jira-solve-ci
bot
force-pushed
the
fix-CNTRLPLANE-2789
branch
2 times, most recently
from
a13d1d8 to
7ca4ffa
Compare
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2789 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 "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. |
|
@coderabbitai Thanks for catching the missing AI-assisted response via Claude Code |
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, hypershift-jira-solve-ci[bot] 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
|
/lgtm cancel |
|
hypershift-jira-solve-ci - please squash all three commits together |
|
/test address-review-comments |
…ests Resolve the TODO at controlplane-component_test.go requesting a StatefulSet test case. The reconcile logic handles both Deployment and StatefulSet workloads, but only the Deployment path was covered. The test table is refactored to be parameterized with newComponent and getWorkload functions so the same builtin hypershift opinion validations (labels, affinity, volume permissions, image pull policy, service account kubeconfig injection, etc.) are exercised for both workload types. A workloadResult struct extracts the common fields (labels, PodTemplateSpec, replicas) from either workload kind. Also adds a statefulset.yaml asset manifest for the test-component fixture, mirroring the existing deployment.yaml, required for the StatefulSet reconcile test case to load its workload manifest via LoadStatefulSetManifest. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
hypershift-jira-solve-ci
bot
force-pushed
the
fix-CNTRLPLANE-2789
branch
from
7ca4ffa to
397c6c0
Compare
|
Done. All three commits have been squashed into a single commit: AI-assisted response via Claude Code |
|
/lgtm |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2789 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 "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. |
Test Resultse2e-aws
Failed TestsTotal failed tests: 2
e2e-aks
|
|
/verified by UTs |
openshift-ci-robot
added
the
verified
|
@bryan-cox: 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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@hypershift-jira-solve-ci[bot]: The following test 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. |
enxebre
added
the
acknowledge-critical-fixes-only
|
/retest |
1 similar comment
|
/retest |
|
/override "Red Hat Konflux / hypershift-operator-main-enterprise-contract / hypershift-operator-main" |
|
/override "Red Hat Konflux / hypershift-operator-enterprise-contract / hypershift-operator-main" |
|
@bryan-cox: Overrode contexts on behalf of bryan-cox: Red Hat Konflux / hypershift-operator-main-enterprise-contract / hypershift-operator-main 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 kubernetes-sigs/prow repository. |
|
@bryan-cox: Overrode contexts on behalf of bryan-cox: Red Hat Konflux / hypershift-operator-enterprise-contract / hypershift-operator-main 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 kubernetes-sigs/prow repository. |
|
/test address-review-comments |
4 participants
What this PR does / why we need it:
Adds StatefulSet test coverage to the controlplane-component reconcile tests. The reconcile logic handles both Deployment and StatefulSet workloads, but only the Deployment path was previously covered. This resolves the TODO in
controlplane-component_test.gorequesting a StatefulSet test case.Key changes:
statefulset.yamlasset manifest for the test-component fixture, mirroring the existingdeployment.yamlnewComponentandgetWorkloadfunctions so the same builtin HyperShift opinion validations (labels, affinity, volume permissions, image pull policy, service account kubeconfig injection, etc.) are exercised for both Deployment and StatefulSet workload typesworkloadResultstruct to extract common fields (labels, PodTemplateSpec, replicas) from either workload kindWhich issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/CNTRLPLANE-2789
Special notes for your reviewer:
The existing Deployment tests are unchanged in behavior — they are simply restructured into the parameterized table alongside the new StatefulSet test case.
Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via
/jira:solve [CNTRLPLANE-2789](https://issues.redhat.com/browse/CNTRLPLANE-2789)Summary by CodeRabbit
Tests
Chores