fix(vm): preserve disk deletions in update request#2614
Conversation
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Summary of ChangesHello @bpg-dev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the Proxmox Terraform provider that prevented the successful deletion of secondary disks from virtual machine configurations. Previously, deletion requests for disks were inadvertently discarded during the update process, leading to discrepancies between the Terraform state and the actual Proxmox environment. The implemented fix ensures that all specified deletions, including those for disks, are correctly processed and sent to the Proxmox API, thereby maintaining data integrity and expected behavior. A new acceptance test has been added to prevent regressions and confirm the fix. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request provides a solid fix for a critical bug where disk deletions were not being propagated to Proxmox during an update. The change from overwriting the Delete slice to appending to it correctly resolves the issue, and the addition of the TestAccResourceVMDiskRemoval acceptance test ensures robustness and prevents future regressions. The implementation is clean and well-documented, and no security vulnerabilities were found.
|
That was very quick, thank you! ❤️ |
What does this PR do?
Fixes a bug where removing a secondary disk from a VM configuration would not actually delete the disk in Proxmox. In
vmUpdate(), disk interface names (e.g.scsi1) were correctly appended toupdateBody.Deleteearly in the function, but a later assignmentupdateBody.Delete = deloverwrote them with only non-disk deletions. The Proxmox API never receiveddelete=scsi1, so the disk remained.The fix changes the assignment to
append(updateBody.Delete, del...)to merge both disk and non-disk deletions.Contributor's Note
make lintand fixed any issues.make docs; SDK: manual/docs/edits).!in PR title).make exampleto verify the change works (mainly for SDK / provider config changes).Proof of Work
Acceptance test:
The test creates a VM with two disks (scsi0 + scsi1), removes scsi1, then verifies via the Proxmox API that scsi1 no longer exists in the VM configuration.
Mitmproxy verification —
delete: scsi1is sent in the PUT request:Existing tests still pass:
Community Note
Closes #2596