fix(proxmox): use default values when possible #10208
Conversation
Codecov Report
|
|
cc @carlpett |
| QemuCpu: c.CPUType, | ||
| Description: "Packer ephemeral build VM", |
There was a problem hiding this comment.
This should be automatically either removed or replaced with the configured template description as part of the template-conversion step. Is this not happening for you?
I believe it is a good thing to have this in place during the build so others seeing the VM can understand what it is.
There was a problem hiding this comment.
I tried again today and I can confirm that the description parameter is passed to the final VM. I've the following template.
$ qm config 200
name: ubuntu-20.04
description: Packer ephemeral build VM
template: 1
[...]
I've created a VM from this template.
$ qm config 108
name: templated
description: Packer ephemeral build VM
[...]
This is not critical but if the description is passed to the final VM, it's not that useful.
There was a problem hiding this comment.
What do you think of this change @carlpett ? Should we go on with it ? 🙂 cheers !
There was a problem hiding this comment.
Ooops, must have missed this notification in the christmas mail heaps. Sorry!
What I would have expected in this is that the template with VMID 200 shouldn't have a description at all, it's supposed to be removed before being converted to a template. If it has stuck around somehow, then it's expected that it'll get cloned onto VMs built from the template.
I'm unable to reproduce this locally, though. Could you tell me what version of Packer and Proxmox you have, and a sample build configuration?
There was a problem hiding this comment.
bump @aerialls -- did you see Carl's comment above?
|
@aerialls It's been a month, so I'm checking in -- any interest in addressing the review suggestions, or should I close this PR? |
|
@SwampDragons Sorry I missed the reply, I'll check this week-end, thanks! |
ghost
removed
|
Is this ready for a re-review? |
|
@SwampDragons Yes, good for me! |
azr
added
the
stage/thinking
|
@carlpett are you happy for me to merge? |
|
@SwampDragons Well, no, not really... @aerialls could you respond in the thread? I believe that the behaviour you are describing would be a bug. But I can't reproduce it. |
|
Sorry, I forgot about the missing review. I'm going to close this in two weeks if we don't hear back, since I pinged a month ago on that thread. |
|
@aerialls I closed this PR because of inactivity. If you find that you want to revisit this change please feel free to follow up int the Promox plugin repo, currently at github.com/hashicorp/packer-plugin-proxmox |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Use default values when possible for the Proxmox provider.
Descriptionas the template will be used for "final" (non-ephemeral) VMs and the description will be wrong.Bootparameter. From my tests, this is breaking the booting order and will always load the CDROM every time. Let the API use the default value (hard-drive, cd-rom and network).