ROSAENG-62084: fix: sync autoRepair and labels into passthrough fiel… - #332
ROSAENG-62084: fix: sync autoRepair and labels into passthrough fiel…#332gdbranco wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift-online/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughNodePool conversion now propagates ChangesNodePool synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The change synchronizes node pool response fields and has no actionable merge-blocking risk remaining after normal checks and review. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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)
platform-api/pkg/clients/hyperfleetdb/convert_test.go (2)
128-139: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover stale passthrough labels in the empty case.
The test creates a zero-valued nested
NodePool, so it passes even whenNodePoolCRToPlatformleaves oldNodeLabelsunchanged. Pre-populateSpec.NodePool.NodeLabels, keepSpec.Labelsnil or empty, and assert that the returned map is empty.🤖 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 `@platform-api/pkg/clients/hyperfleetdb/convert_test.go` around lines 128 - 139, Update TestNodePoolCRToPlatform_LabelsEmpty to initialize Spec.NodePool.NodeLabels with a non-empty stale map while keeping Spec.Labels nil or empty, then assert NodePoolCRToPlatform clears it and returns an empty NodeLabels map.
70-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the explicit-false case test an override.
NodePoolSpecPassthrough.Management.AutoRepairstarts asfalse. A converter that ignores an explicitfalsetherefore still passes this test row. Initialize the nested value totruefor this case, or add aninitialMgmttable field, before callingNodePoolCRToPlatform.🤖 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 `@platform-api/pkg/clients/hyperfleetdb/convert_test.go` around lines 70 - 85, Update the “explicit false propagates to passthrough” test in the NodePoolCRToPlatform test table so the nested management AutoRepair value starts as true before conversion, or introduce and apply an initialMgmt fixture field. Verify the conversion overrides that initial true value to false when autoRepair is explicitly false.
🤖 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 `@platform-api/pkg/clients/hyperfleetdb/convert.go`:
- Around line 136-139: The converter must synchronize empty top-level labels as
well as non-empty ones. In platform-api/pkg/clients/hyperfleetdb/convert.go
lines 136-139, update the label conversion to assign NodeLabels unconditionally
from Labels. In platform-api/pkg/clients/hyperfleetdb/convert_test.go lines
128-139, pre-populate stale NodeLabels and assert that empty top-level Labels
clears them.
---
Nitpick comments:
In `@platform-api/pkg/clients/hyperfleetdb/convert_test.go`:
- Around line 128-139: Update TestNodePoolCRToPlatform_LabelsEmpty to initialize
Spec.NodePool.NodeLabels with a non-empty stale map while keeping Spec.Labels
nil or empty, then assert NodePoolCRToPlatform clears it and returns an empty
NodeLabels map.
- Around line 70-85: Update the “explicit false propagates to passthrough” test
in the NodePoolCRToPlatform test table so the nested management AutoRepair value
starts as true before conversion, or introduce and apply an initialMgmt fixture
field. Verify the conversion overrides that initial true value to false when
autoRepair is explicitly false.
🪄 Autofix
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-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 286aea32-3a43-4716-8576-7bf67c82723f
📒 Files selected for processing (3)
hyperfleet-operator/internal/render/nodepool_test.goplatform-api/pkg/clients/hyperfleetdb/convert.goplatform-api/pkg/clients/hyperfleetdb/convert_test.go
|
/retest-konflux |
d32345d to
7aa72b3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
platform-api/go.mod (1)
36-36: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftAdd or document the remaining supply-chain controls.
go.sumcontains both hashes, OSV reports no vulnerabilities, andk8s.io/utilsuses Apache-2.0. Record license compatibility and provide SBOM, provenance attestations, and Sigstore/cosign signing for release artifacts.🤖 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 `@platform-api/go.mod` at line 36, Document the supply-chain controls for the k8s.io/utils dependency: record Apache-2.0 license compatibility and add or reference SBOM generation, provenance attestations, and Sigstore/cosign signatures for release artifacts. Keep the existing dependency version and checksum handling unchanged.Source: Path instructions
🤖 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 `@platform-api/go.mod`:
- Line 36: Document the supply-chain controls for the k8s.io/utils dependency:
record Apache-2.0 license compatibility and add or reference SBOM generation,
provenance attestations, and Sigstore/cosign signatures for release artifacts.
Keep the existing dependency version and checksum handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 81638c23-9bc6-4ceb-84d8-0eabee72aebc
📒 Files selected for processing (1)
platform-api/go.mod
f04c31d to
944d792
Compare
cdoan1
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@gdbranco: This pull request references ROSAENG-62084 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 epic 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoan1, gdbranco 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
|
/test on-demand-e2e |
…ds on read NodePoolCRToPlatform now populates nodePool.management.autoRepair from the top-level autoRepair field (defaulting to true when unset, matching the operator default) and nodePool.nodeLabels from the top-level labels field, keeping the API response internally consistent.
944d792 to
1e6fb14
Compare
|
/test on-demand-e2e |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@gdbranco rebase and konflux should work |
3 participants
…ds on read
NodePoolCRToPlatform now populates nodePool.management.autoRepair from the top-level autoRepair field (defaulting to true when unset, matching the operator default) and nodePool.nodeLabels from the top-level labels field, keeping the API response internally consistent.
Description
Type of Change
Testing
make test)Checklist
Summary by CodeRabbit
Bug Fixes
Tests