Skip to content

fix(vm): re-use ide, sata, and scsi cloud-init storage (#1083)#1141

Merged
bpg-dev merged 4 commits into
bpg:mainfrom
michaelfranzl:fix_1083
Mar 26, 2024
Merged

fix(vm): re-use ide, sata, and scsi cloud-init storage (#1083)#1141
bpg-dev merged 4 commits into
bpg:mainfrom
michaelfranzl:fix_1083

Conversation

@michaelfranzl

Copy link
Copy Markdown
Contributor

Contributor's Note

  • I have added / updated documentation in /docs for any user-facing features or additions.
  • I have added / updated templates in /example for any new or updated resources / data sources.
  • I have ran make example to verify that the change works as expected.

I'm a first-time contributor, so this PR might not be acceptable, but I'd appreciate if you'd help me along.

Proof of Work

I have manually tested the described use case of #1083 against Proxmox, and found it working.

TODO: Automatic testing: Write an example targeting this use case.

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #1083

@bpg-dev

bpg-dev commented Mar 21, 2024

Copy link
Copy Markdown
Member

Hi @michaelfranzl 👋🏼

Thanks a lot for the PR! I really appreciate contributions! ❤️
I'll take a closer look a bit later in the week, we could probably simplify a few things there. As you mentioned, it would be nice to have this use case captured in the acceptance tests, i may be able to quickly add them as well.

In a meantime, would you mind signing DCO? You can find more information in Details of the failed DCO check.

@michaelfranzl

Copy link
Copy Markdown
Contributor Author

I'll sign the DCO, and see if I can add acceptance tests as well.

…ce (#1083)

Signed-off-by: Michael Franzl <michael@franzl.name>
Signed-off-by: Michael Franzl <michael@franzl.name>
@michaelfranzl

Copy link
Copy Markdown
Contributor Author

@bpg In the first commit, I added a failing acceptance test. The failing test output is:

=== RUN   TestAccResourceVMInitialization                                                                 
=== RUN   TestAccResourceVMInitialization/initialization                                                  
    resource_vm_test.go:184: Step 1/1 error: After applying this test step, the non-refresh plan was not empty.                                                                                                     
        stdout:                                                                                           
                                                                                                          
                                                                                                          
        Terraform used the selected providers to generate the following execution                                                                                                                                   
        plan. Resource actions are indicated with the following symbols:                                                                                                                                            
          ~ update in-place                                                                               
                                                                                                          
        Terraform will perform the following actions:                                                     
                                                                                                          
          # proxmox_virtual_environment_vm.test_vm_network1 will be updated in-place                                                                                                                                
          ~ resource "proxmox_virtual_environment_vm" "test_vm_network1" {                                                                                                                                          
                id                      = "100"                                                           
                # (26 unchanged attributes hidden)                                                        
                                                                                                          
              ~ cpu {                                                                                     
                  + flags        = []                                                                     
                    # (8 unchanged attributes hidden)                                                     
                }                                                                                         
                                                                                                          
              ~ initialization {                                                                          
                  + datastore_id      = "local-lvm"                                                       
                  + interface         = "scsi1"                                                           
                    # (1 unchanged attribute hidden)                                                      
                                                                                                          
                    # (1 unchanged block hidden)                                                          
                }                                                                                         
                                                                                                          
                # (4 unchanged blocks hidden)                                                             
            }                                                                                             
                                                                                                          
        Plan: 0 to add, 1 to change, 0 to destroy.                                                        
--- FAIL: TestAccResourceVMInitialization (57.05s)                                                        
    --- FAIL: TestAccResourceVMInitialization/initialization (57.05s)                                                                                                                                               
FAIL                                                                                                      
FAIL    github.com/bpg/terraform-provider-proxmox/fwprovider/tests      57.052s                                                                                                                                     
FAIL

This output exactly reproduces what is reported in #1083.

The test then passes with the second commit, which only modifies the implementation.

bpg-dev added 2 commits March 25, 2024 21:17
Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>
@bpg-dev bpg-dev changed the title fix(vm): Re-use ide, sata, and scsi cloud-init storage (#1083) fix(vm): re-use ide, sata, and scsi cloud-init storage (#1083) Mar 26, 2024

@bpg-dev bpg-dev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, much appreciated!

LGTM! 🚀

@bpg-dev
bpg-dev merged commit d41463d into bpg:main Mar 26, 2024
@bpg-dev

bpg-dev commented Mar 26, 2024

Copy link
Copy Markdown
Member

@all-contributors please add @michaelfranzl for code, test

@allcontributors

Copy link
Copy Markdown
Contributor

@bpg

I've put up a pull request to add @michaelfranzl! 🎉

@github-actions github-actions Bot added status:in-progress The issue is being worked on and removed status:in-progress The issue is being worked on labels Mar 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

proxmox_virtual_environment_vm is not idempotent with cloud init drive datastore_id + interface

2 participants