CNTRLPLANE-3575: test(controlplane-component): verify setDefaultOptions preserves exis… - #8731
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mgencur: This pull request references CNTRLPLANE-3575 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. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates TestSetDefaultOptions: it adds an imageprovider import and refactors the etcd SecurityContext assertions into a subtest. It introduces a table-driven kube-apiserver resources subtest that constructs a ReleaseImageProvider, builds a kube-apiserver Deployment with initial container Resources, calls controlPlaneWorkload.setDefaultOptions with varying existingResources (including nil), and asserts the resulting container Resources match the expected preservation or replacement behavior. 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
![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
🤖 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 `@support/controlplane-component/defaults_test.go`:
- Around line 563-588: The test case "annotation override takes precedence over
existing resources" only asserts Requests are merged; to ensure Limits from
existingResources survive the annotation-override path, add a Limits field to
existingResources["kube-apiserver"] (e.g., set a Memory/CPU limit) and include
the identical Limits in expectedResources so the test verifies Limits are
preserved after setDefaultOptions/applyRequestsOverrides; update the test
vectors under existingResources and expectedResources accordingly to assert
Limits survive the override.
🪄 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: 09746984-52f5-4c83-b137-932ec57133ac
📒 Files selected for processing (1)
support/controlplane-component/defaults_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8731 +/- ##
==========================================
+ Coverage 41.66% 41.68% +0.02%
==========================================
Files 758 758
Lines 93929 93929
==========================================
+ Hits 39135 39155 +20
+ Misses 52046 52028 -18
+ Partials 2748 2746 -2 see 2 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:
|
2d5f8da to
908d369
Compare
| g.Expect(workloadObject.Spec.Template.Spec.SecurityContext.FSGroup).To(Equal(ptr.To(int64(1002)))) | ||
| } | ||
|
|
||
| func TestSetDefaultOptionsPreservesExistingResources(t *testing.T) { |
There was a problem hiding this comment.
can we please make this test cases of the existing TestSetDefaultOptions?
fwiw we have also TestApplyRequestsOverrides
There was a problem hiding this comment.
Alright. I removed the annotation override test because it was already tested (it simply takes precedence no matter what the original values are).
Also put the remaining tests under TestSetDefaultOptions
…ting resource requirements Adds unit tests for the resource preservation behavior in setDefaultOptions (defaults.go:159-164). Covers three scenarios: annotation-based overrides taking precedence over existing resources, full ResourceRequirements struct (Requests + Limits) surviving reconciliation, and manifest defaults kept when no existing resources are present. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
908d369 to
04c943c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
support/controlplane-component/defaults_test.go (1)
530-530: ⚡ Quick winStandardize new subtest names to the required “When ... it should ...” format.
The new subtest titles don’t follow the mandated test-case naming pattern.
Proposed rename patch
- t.Run("etcd sets SecurityContext with custom UID", func(t *testing.T) { + t.Run("When SetDefaultSecurityContext is enabled for etcd it should set RunAsUser and FSGroup to the custom UID", func(t *testing.T) { ... - name: "existing resources with both requests and limits are fully preserved", + name: "When existing resources include requests and limits it should preserve both", ... - name: "without existing resources container keeps its manifest defaults", + name: "When existing resources are absent it should keep manifest defaults",As per coding guidelines, “Always use "When ... it should ..." format for describing test cases when creating unit tests.”
Also applies to: 563-563, 594-594
🤖 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 `@support/controlplane-component/defaults_test.go` at line 530, Rename the subtest titles to follow the "When ... it should ..." naming convention: locate the t.Run calls in defaults_test.go (e.g., the current subtest titled "etcd sets SecurityContext with custom UID" and the other two at the same file around the other reported lines) and change their names to the required pattern — for example: "When etcd has a custom UID it should set SecurityContext with that UID" — ensuring each t.Run string starts with "When" and contains "it should" describing the expected behavior.Source: Coding guidelines
🤖 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 `@support/controlplane-component/defaults_test.go`:
- Line 530: Rename the subtest titles to follow the "When ... it should ..."
naming convention: locate the t.Run calls in defaults_test.go (e.g., the current
subtest titled "etcd sets SecurityContext with custom UID" and the other two at
the same file around the other reported lines) and change their names to the
required pattern — for example: "When etcd has a custom UID it should set
SecurityContext with that UID" — ensuring each t.Run string starts with "When"
and contains "it should" describing the expected behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 190d5021-7ad2-47ec-9b1b-a798feb7d1ab
📒 Files selected for processing (1)
support/controlplane-component/defaults_test.go
muraee
left a comment
muraee
left a comment
There was a problem hiding this comment.
Review
The tests are correct and cover the right behavior — existingResources overriding container defaults, and nil existingResources falling back to manifest defaults. Good refactoring of the existing etcd test into a named sub-test too.
Naming convention
Test case names should follow Gherkin When...it should... syntax per project convention:
| Current | Suggested |
|---|---|
"existing resources with both requests and limits are fully preserved" |
"When existing resources with both requests and limits exist it should fully preserve them" |
"without existing resources container keeps its manifest defaults" |
"When no existing resources are present it should keep manifest defaults" |
Minor notes
imageProviderlocal variable shadows the imported packageimageprovider— consider renaming toimgProviderorreleaseProvider.- Consider adding
t.Parallel()to sub-tests for consistency with other tests in this file.
Coverage suggestion (non-blocking)
- Init containers aren't covered —
applyRequestsOverrideshandles bothInitContainersandContainers(defaults.go:427–438). Adding a case with init containers would strengthen coverage.
| expectedResources corev1.ResourceRequirements | ||
| }{ | ||
| { | ||
| name: "existing resources with both requests and limits are fully preserved", |
There was a problem hiding this comment.
plese use gherkin when it should convention
…d parallelism Adopt Gherkin "When...it should..." convention for subtest names to match TestApplyRequestsOverrides style. Add t.Parallel() to subtests and rename imageProvider to releaseProvider to avoid shadowing the imported package. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre, mgencur 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 |
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Test Resultse2e-aws
e2e-aks
Failed TestsTotal failed tests: 5
|
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
Now I have the complete picture. Let me produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 7 test suites (416 tests each across private, public, external-oidc, oauth-lb, upgrade, etcd-chaos, autoscaling) passed with zero failures. The job was marked as failed solely due to post-phase cleanup step failures that are completely unrelated to the PR's code changes. The self-managed management cluster's kube-apiserver at Root CauseThe failure is an infrastructure-level issue in the CI post-phase cleanup, not a test failure. The root cause chain is:
Key insight: The CI cluster itself (accessible via AWS ELB This failure is not related to PR #8731 which only adds a unit test for Recommendations
Evidence
|
|
/override "ci/prow/e2e-aks" |
|
@celebdor: Overrode contexts on behalf of celebdor: ci/prow/e2e-aks, ci/prow/e2e-aws, ci/prow/e2e-azure-v2-self-managed, ci/prow/e2e-kubevirt-aws-ovn-reduced 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. |
|
/verified by unit tests |
openshift-ci-robot
added
the
verified
|
@mgencur: 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. |
|
@mgencur: 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. |
6 participants
…ting resource requirements
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3575
The existing E2E test in openshift-tests-private is converted to a unit test in openshift/hypershift.
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit