HIVE-2946: Remove empty vSphere dataDisks from install-config#2904
HIVE-2946: Remove empty vSphere dataDisks from install-config#2904chdeshpa-hue wants to merge 2 commits intoopenshift:masterfrom
Conversation
Strip empty vSphere dataDisks fields after install-config secret mutation to avoid unknown-field warnings from older installer payloads. Add focused unit coverage to ensure empty dataDisks are removed while explicitly configured dataDisks are preserved. Assisted-by: Codex 5.3 Co-authored-by: Cursor <cursoragent@cursor.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@chdeshpa-hue: This pull request references HIVE-2946 which is a valid jira issue. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chdeshpa-hue 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
the
needs-ok-to-test
|
Hi @chdeshpa-hue. 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 Regular contributors should join the org to skip this step. 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. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ChangesvSphere DataDisks Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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 `@pkg/installmanager/installmanager.go`:
- Around line 2035-2038: The call to yamlutils.ApplyPatches in the
installmanager should not abort the install on failure; instead catch the error
from yamlutils.ApplyPatches(icData, patches), log or wrap it as a non-fatal
warning, and fall back to using the original icData bytes (or previously
serialized install-config bytes) rather than returning an error. Update the
block around modifiedBytes, err to handle err by preserving icData (or original
bytes) in modifiedBytes and continuing, referencing yamlutils.ApplyPatches,
modifiedBytes, icData and patches so the patch removal failure becomes a
warning/fallback rather than a hard return.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e766d14e-94e1-49e5-af8b-742b43b1f855
📒 Files selected for processing (2)
pkg/installmanager/installmanager.gopkg/installmanager/installmanager_test.go
Fall back to original install-config data if YAML patching fails instead of aborting the install, which would be worse than the original unknown-field warning. Addresses CodeRabbit review feedback on PR openshift#2904. Assisted-by: Codex 5.3 Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Summary
dataDiskskeys from install-config after install-manager mutates secrets, avoiding unknown-field warnings on older payloadsdataDisksintact so newer payload behavior is preservedTest plan
go test ./pkg/installmanager -run 'Test_removeUnsupportedVSphereDataDisks|Test_pasteInPullSecret'Made with Cursor