[Issue 71] add skip_convert_to_template option #283
Conversation
There was a problem hiding this comment.
Overall LGTM, thanks @mpywell
Just left a very small nit and a question about a mock signature changing, but overall the code looks good to me, once you've had a chance to address my comments we should be good to go!
Also apologies for leaving this one aside for so long, I'll do a full review of the open PRs on Proxmox today, with the aim of scheduling a release for later this week if possible.
Thanks again!
| @@ -17,18 +17,19 @@ import ( | |||
| // stepFinalizeTemplateConfig does any required modifications to the configuration _after_ | |||
There was a problem hiding this comment.
Minor nit, I'd suggest updating the documentation here just so the name of the step is correctly reflected.
| GetVmConfig(*proxmox.VmRef) (map[string]interface{}, error) | ||
| SetVmConfig(*proxmox.VmRef, map[string]interface{}) (interface{}, error) | ||
| StartVm(*proxmox.VmRef) (string, error) |
There was a problem hiding this comment.
Is there a reason to add StartVm to that contract? It seems that the function is not invoked in the step, and AFAICT this is not used elsewhere, so I wonder if that might be a leftover from a WIP state of the PR?
Feel free to correct me if I'm wrong though, I might've missed something
There was a problem hiding this comment.
It was left over from a WIP version of the PR where the returned artifact was a running VM. I noted in #72 (comment) a desire to have a running VM returned, so I've rerolled with those changes.
There was a problem hiding this comment.
Ah yeah understood, I missed the resume VM part.
I wonder though, the current workflow appears to work (can't test it as I don't have a proxmox machine on hand), but I would think we can keep the VM running throughout the process if we're not exporting to a template, don't we?
In this case we can change a bit the Cleanup method of the step_start_vm so it doesn't kill and delete the VM, unless it cannot?
Also to be sure, since the cleanup for the VM will run after step_finalize_config, will the VM still exist? I'd assume this will be stopped and removed, won't it?
There was a problem hiding this comment.
A stop and start of the VM is required if there are VM hardware changes for the VM's Cloud-Init storage and CD-ROM devices (step step_finalize_config).
These changes aren't picked up by the VM unless it's completely stopped then started again, they can't be hot-swapped or mounted on an ACPI reboot. This is a limitation on the Proxmox backend.
I've reworked the logic to only stop the VM artifact if there are changes to be done in step_finalize_config (if len(changes) > 0) that way if Cloud-Init isn't being configured or CD-ROM devices are being kept, the VM won't restart.
mpywell
force-pushed
the
feat/skip_convert_to_template
branch
from
b8c44e9
to
f2a0326
Compare
|
Hi @lbajolet-hashicorp, I've rerolled and addressed comments |
| } | ||
| return vmRef, nil | ||
| return vmRef, "VM template", nil |
There was a problem hiding this comment.
Regarding the change to the signature here; since the two options are statically known and only depend on SkipConvertToTemplate, we can probably leave it out, and do the change in the one place it's relevant: in Run directly.
There was a problem hiding this comment.
Agreed on reflection this was a bit untidy, have removed it and where c.PackerForce is concerned have updated the wording of ui messages to say 'resource' instead of 'VM' or 'VM template'.
| GetVmConfig(*proxmox.VmRef) (map[string]interface{}, error) | ||
| SetVmConfig(*proxmox.VmRef, map[string]interface{}) (interface{}, error) | ||
| StartVm(*proxmox.VmRef) (string, error) |
There was a problem hiding this comment.
Ah yeah understood, I missed the resume VM part.
I wonder though, the current workflow appears to work (can't test it as I don't have a proxmox machine on hand), but I would think we can keep the VM running throughout the process if we're not exporting to a template, don't we?
In this case we can change a bit the Cleanup method of the step_start_vm so it doesn't kill and delete the VM, unless it cannot?
Also to be sure, since the cleanup for the VM will run after step_finalize_config, will the VM still exist? I'd assume this will be stopped and removed, won't it?
- updated VM stop/start logic for hardware changes to a VM build - reverted getExistingTemplate signature changes
|
Hi @lbajolet-hashicorp pushed another reroll and comments addressed |
Rewrite of #72 on current codebase.
Implements config option skip_convert_to_template which skips converting the build to a Template and leaves the build artifact as a VM.
Closes #71