CNTRLPLANE-3693: Refactor nodepool platformConditions signature and extract common platform conditions#9018
Conversation
… common platform conditions CNTRLPLANE-3693 The setPlatformConditions function had a different signature from other condition-setting functions in conditions.go. This refactoring: - Renames setPlatformConditions to platformConditions with the standard (ctx, nodePool, hcluster) (*ctrl.Result, error) signature, matching other condition setters in the signalConditions slice. - Extracts the common NodePoolValidPlatformImageType condition into a new setValidPlatformImageCondition function that dispatches to per-platform image resolution. - Separates platform-specific conditions (e.g. AWS security group) into setPlatformSpecificConditions. - Splits setAWSConditions into setAWSValidPlatformImage (image condition) and setAWSSecurityGroupCondition (AWS-specific). - Renames setKubevirtConditions, setPowerVSconditions, and setOpenStackConditions to setXxxValidPlatformImage to clarify their role in the common image validation flow. No behavioral change — the same conditions are set under the same circumstances, and all existing tests pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ions Pass already-resolved releaseImage and controlPlaneNamespace into platformConditions instead of re-deriving them, eliminating a redundant getReleaseImage call per reconcile. Simplify return type from (*ctrl.Result, error) to error since the function never signals a requeue. Remove unused controlPlaneNamespace parameter from setOpenStackValidPlatformImage. Rename TestSetAWSConditions to match its target function and add test coverage for setAWSSecurityGroupCondition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-3693 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. |
📝 WalkthroughWalkthroughThe NodePool reconciliation flow now separates valid platform image validation from platform-specific condition handling. Dedicated handlers are used for KubeVirt, AWS, OpenStack, and PowerVS image validation. AWS security group availability is handled independently, with new tests covering missing and present default worker security group IDs. Existing AWS image validation tests now invoke the dedicated handler. Sequence Diagram(s)sequenceDiagram
participant NodePoolReconciler
participant platformConditions
participant setValidPlatformImageCondition
participant setAWSValidPlatformImage
participant setAWSSecurityGroupCondition
NodePoolReconciler->>platformConditions: process platform conditions
platformConditions->>setValidPlatformImageCondition: dispatch image validation
setValidPlatformImageCondition->>setAWSValidPlatformImage: validate AWS platform image
setAWSValidPlatformImage->>NodePoolReconciler: update image condition
platformConditions->>setAWSSecurityGroupCondition: evaluate AWS security group
setAWSSecurityGroupCondition->>NodePoolReconciler: update security group condition
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hypershift-jira-solve-ci[bot] 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 |
openshift-ci
Bot
added
area/hypershift-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 `@hypershift-operator/controllers/nodepool/conditions.go`:
- Around line 160-165: Update NodePoolReconciler.platformConditions to invoke
both setValidPlatformImageCondition and setPlatformSpecificConditions regardless
of whether image validation returns an error. Capture each result, preserve
platform-specific condition updates, and return the image-validation error if
present, otherwise return the platform-specific result.
🪄 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: bf56f93a-1bac-45fb-8311-bb5a5c2740f2
📒 Files selected for processing (7)
hypershift-operator/controllers/nodepool/aws.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/kubevirt.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/openstack.gohypershift-operator/controllers/nodepool/powervs.go
| func (r *NodePoolReconciler) platformConditions(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, controlPlaneNamespace string, releaseImage *releaseinfo.ReleaseImage) error { | ||
| if err := r.setValidPlatformImageCondition(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return r.setPlatformSpecificConditions(nodePool, hcluster) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run platform-specific conditions even when image validation fails.
The early return at Line 162 prevents setAWSSecurityGroupCondition from updating NodePoolAWSSecurityGroupAvailableConditionType whenever platform-image validation returns an error. Compute both results before returning so AWS security-group status remains independent of image validation.
Suggested fix
func (r *NodePoolReconciler) platformConditions(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, controlPlaneNamespace string, releaseImage *releaseinfo.ReleaseImage) error {
- if err := r.setValidPlatformImageCondition(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage); err != nil {
- return err
- }
-
- return r.setPlatformSpecificConditions(nodePool, hcluster)
+ imageErr := r.setValidPlatformImageCondition(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage)
+ platformErr := r.setPlatformSpecificConditions(nodePool, hcluster)
+ if imageErr != nil {
+ return imageErr
+ }
+ return platformErr
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *NodePoolReconciler) platformConditions(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, controlPlaneNamespace string, releaseImage *releaseinfo.ReleaseImage) error { | |
| if err := r.setValidPlatformImageCondition(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage); err != nil { | |
| return err | |
| } | |
| return r.setPlatformSpecificConditions(nodePool, hcluster) | |
| func (r *NodePoolReconciler) platformConditions(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, controlPlaneNamespace string, releaseImage *releaseinfo.ReleaseImage) error { | |
| imageErr := r.setValidPlatformImageCondition(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage) | |
| platformErr := r.setPlatformSpecificConditions(nodePool, hcluster) | |
| if imageErr != nil { | |
| return imageErr | |
| } | |
| return platformErr | |
| } |
🤖 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 `@hypershift-operator/controllers/nodepool/conditions.go` around lines 160 -
165, Update NodePoolReconciler.platformConditions to invoke both
setValidPlatformImageCondition and setPlatformSpecificConditions regardless of
whether image validation returns an error. Capture each result, preserve
platform-specific condition updates, and return the image-validation error if
present, otherwise return the platform-specific result.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9018 +/- ##
=======================================
Coverage 44.13% 44.13%
=======================================
Files 772 772
Lines 96255 96267 +12
=======================================
+ Hits 42478 42488 +10
- Misses 50831 50835 +4
+ Partials 2946 2944 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@hypershift-jira-solve-ci[bot]: 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. |
1 participant
What this PR does / why we need it:
Aligns the
setPlatformConditionsfunction signature with other condition-setting functions inconditions.goand extracts common platform image validation into a shared flow.Refactoring (commit 1):
setPlatformConditionstoplatformConditionswith the standard(ctx, nodePool, hcluster) (*ctrl.Result, error)signature, matching other condition setters in thesignalConditionsslice.NodePoolValidPlatformImageTypecondition into a newsetValidPlatformImageConditionfunction that dispatches to per-platform image resolution.setPlatformSpecificConditions.setAWSConditionsintosetAWSValidPlatformImage(image condition) andsetAWSSecurityGroupCondition(AWS-specific).setKubevirtConditions,setPowerVSconditions, andsetOpenStackConditionstosetXxxValidPlatformImageto clarify their role in the common image validation flow.Follow-up fixes (commit 2):
releaseImageandcontrolPlaneNamespaceintoplatformConditionsinstead of re-deriving them, eliminating a redundantgetReleaseImagecall per reconcile.(*ctrl.Result, error)toerrorsince the function never signals a requeue.controlPlaneNamespaceparameter fromsetOpenStackValidPlatformImage.TestSetAWSConditionsto match its target function and adds test coverage forsetAWSSecurityGroupCondition.No behavioral change — the same conditions are set under the same circumstances, and all existing tests pass.
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3693
Special notes for your reviewer:
This is a pure refactoring with no behavioral changes. The primary motivation is to align the platform conditions function signature with the rest of the condition setters and to make the common image validation flow explicit.
Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin
Summary by CodeRabbit
Bug Fixes
Tests