[Issue 71] Adds convert_to_template option to proxmox builders #72

LethalServant · 2022-03-07T01:37:31Z

Adds convert_to_template bool option to proxmox-iso and proxmox-clone builders configuration.
This gives the user more control over the machine image, whether it stays a VM or is converted to a VM Template.
The convert_to_template option is optional, and the default value is false, resulting in the VM not being converted to a template.
In practice the option is only need if the user wants the machine image to become a VM Template.

Tests:
Given working proxmox-iso and proxmox-clone packer file configurations, the following values were changed and run for each builder:

convert_to_template      = true  // result: vm is a template
convert_to_template      = false // result: vm is not a template 
remove convert_to_template option, testing default value // result: vm is not a template

Closes #71

…lone and iso configs

…tion

hashicorp-cla · 2022-03-07T01:37:35Z

All committers have signed the CLA.

carlpett · 2022-04-07T16:41:32Z

Hi @LethalServant,
So this would change the default behaviour of the builder quite a bit, is there a compelling reason for this?

sylviamoss · 2022-05-06T08:03:01Z

@LethalServant do you have any update on @carlpett's question?

adrian-hoasted · 2022-05-11T09:46:48Z

It should keep the default behaviour of converting into a template.
and if the option is set, it should change the default behaviour to leave the VM.

ie.
The convert_to_template option is optional, and the default value is TRUE, resulting in the VM being converted to a template. If the value is set to FALSE, no template is created. This would prevent a breaking change.

sylviamoss

I see this is indeed changing the default behavior and I figure that's not your intention as you mentioned in our previous comment.

sylviamoss · 2022-05-18T08:06:16Z

builder/proxmox/common/config.go

@@ -64,6 +64,7 @@ type Config struct {

 	TemplateName        string `mapstructure:"template_name"`
 	TemplateDescription string `mapstructure:"template_description"`
+	ConvertToTemplate   bool   `mapstructure:"convert_to_template"`


The default of this configuration will always be false which will indeed change the default behavior of the builder. Also, in the prepare method you're explicitly setting it to false as default.

If the intention is to not change the default behavior, the default should be true. In this case, you can:

Use Trilian, a custom type available in the SDK for this type of configuration

Rename it to skip_convert_to_template that will skip converting to template when set to true. The default will be automatically false.

@sylviamoss Thanks for the feedback, this should be resolved now.

sylviamoss · 2022-05-18T08:16:51Z

builder/proxmox/common/builder.go

+	coreSteps = append(coreSteps, &stepStopVM{})
+
+	if b.config.ConvertToTemplate {
+		coreSteps = append(coreSteps, &stepConvertToTemplate{})


I like better when we pass the configuration to the step and within the step is determined if it should be skipped or not based on the configuration. It makes it easier to log "Skipping ..." and keeps this step slice clean.

zakx · 2022-09-27T23:44:48Z

It's been two months, any update on this? Would love to have this functionality.

nywilken · 2022-11-01T16:28:54Z

It's been two months, any update on this? Would love to have this functionality.

Hi @LethalServant thanks for working to push this forward. We haven't had a chance to review this since you last updated. But I'm adding it to our planning board so we can give it some cycles next week. Quickly looking at the approach I don't know if skip_convert_to_template is the best way to avoid changing the default behavior.

I will be releasing a new version of the plugin today v1.1.0 to address #119. After that change we can work to get this change in. Thanks again for pushing this change forward and for sticking with us.

lbajolet-hashicorp

Hi @LethalServant,

First off, apologies for letting this PR open without activity for so long, let's get the ball rolling on this one and get this feature in!

I left a couple of comments on your code, I think the Trilean is not necessary in the current state as you inverted the condition, if it can safely default to false, we can keep it as a bool, which simplifies the interactions with it later on.

Also the code is out of sync since it was untouched for a while, so you will unfortunately need to rebase on top of the current main if we want to merge this later.

Please let me know if you're interested in continuing this work, and if you need help also feel free to reach out, I'll surely be able to help.

Thanks!

lbajolet-hashicorp · 2023-01-20T21:30:58Z

builder/proxmox/common/config.go

-	TemplateDescription string `mapstructure:"template_description"`
+	TemplateName          string         `mapstructure:"template_name"`
+	TemplateDescription   string         `mapstructure:"template_description"`
+	SkipConvertToTemplate config.Trilean `mapstructure:"skip_convert_to_template"`


If there a reason why this is a Trilean? If the intent is that this is false by default, bool would work this way, and this would simplify using the option

lbajolet-hashicorp · 2023-01-20T21:32:36Z

builder/proxmox/common/step_convert_to_template.go

-		ui.Error(err.Error())
-		return multistep.ActionHalt
-	}
+	if !c.SkipConvertToTemplate.True() {


I think this can be inverted here since this is the last step here

Suggested change

if !c.SkipConvertToTemplate.True() {

if c.SkipConvertToTemplate {

ui.Say("Skipping VM template conversion")

return multistep.ActionContinue

}

The rest of the code can be left unchanged if we do it this way

lbajolet-hashicorp · 2023-10-24T18:03:34Z

builder/proxmox/iso/config_test.go

@@ -109,6 +110,9 @@ func TestBasicExampleFromDocsIsValid(t *testing.T) {
 	if b.config.CloudInit != false {
 		t.Errorf("Expected CloudInit to be false, got %t", b.config.CloudInit)
 	}
+	if b.config.SkipConvertToTemplate.False() {
+		t.Errorf("Expected ConvertToTemplate to be true, got %t", b.config.SkipConvertToTemplate.True())


This should be rephrased, also I suspect the error was introduced in the first draft, so the value does not match what the condition highlights.

lbajolet-hashicorp · 2023-10-24T18:04:29Z

builder/proxmox/iso/config_test.go

-    }
+      "template_description": "Fedora 29-1.2, generated on {{ isotime \"2006-01-02T15:04:05Z\" }}",
+	  "skip_convert_to_template": true
+	}


The indentation is not consistent with the rest of the template here, the verbatim template being only indented with spaces.

lbajolet-hashicorp · 2023-10-24T18:06:37Z

builder/proxmox/common/step_stop_vm.go

+
+var _ templateConverter = &proxmox.Client{}
+
+func (s *stepStopVM) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction {


I wonder why this becomes a step on its own here, it seems that the step_convert_to_template does this already, why not move the check after the VM is shutdown so we don't have to introduce another step?
For the record, I don't hate this change by itself, it's just that this brings more changes to this PR which makes the diff larger and harder to understand.

lbajolet-hashicorp · 2023-10-24T18:08:04Z

builder/proxmox/common/step_finalize_vm_config.go

 // the VM has been converted into a template, such as updating name and description, or
 // unmounting the installation ISO.
-type stepFinalizeTemplateConfig struct{}
+type stepFinalizeVMConfig struct{}


Since we can either produce a template or a VM, might I suggest renaming this step as stepFinalize instead? Just to avoid confusion

lbajolet-hashicorp · 2023-10-24T18:11:23Z

builder/proxmox/common/artifact.go

@@ -11,7 +11,8 @@ import (

 type Artifact struct {
 	builderID     string
-	templateID    int
+	vmID          int
+	isTemplate    bool


While I don't think there's other possible types than VM and template, I would maybe suggest changing this to a string, and have an enumeration to the side.

Something like

type ArtifactType string const ( TemplateArtifact ArtifactType = "Template" VMArtifact ArtifactType = "VM" ) [...] type Artifact struct { builderID string artifactID int artifactType ArtifactType [...] }

Doing it this way also makes it possible to inject this in the String() and Destroy() methods without needing conditionals.

lbajolet-hashicorp · 2023-10-24T18:13:24Z

builder/proxmox/common/builder.go

+	coreSteps = append(coreSteps, &stepConvertToTemplate{})
+
+	coreSteps = append(coreSteps, &stepFinalizeVMConfig{})
+	coreSteps = append(coreSteps, &stepSuccess{})


Is there a reason for this change? This might be me only but I don't find this more readable than the original way of defining the steps, and it complexifies the diff so I would consider leaving this change out of the current patchset.

lbajolet-hashicorp · 2023-10-24T18:15:31Z

builder/proxmox/common/builder.go

 	}

 	artifact := &Artifact{
 		builderID:     b.id,
-		templateID:    tplID,
+		vmID:          vmRef.VmId(),
+		isTemplate:    !b.config.SkipConvertToTemplate.True(),


If we decide to keep it as a Trilean, this could be changed to

Suggested change

isTemplate: !b.config.SkipConvertToTemplate.True(),

isTemplate: b.config.SkipConvertToTemplate.False(),

lbajolet-hashicorp · 2023-10-24T18:15:40Z

builder/proxmox/common/builder.go

-	// Verify that the template_id was set properly, otherwise we didn't progress through the last step
-	tplID, ok := state.Get("template_id").(int)
+	// Get vmRef for vmid in artifact storage
+	vmRef, ok := state.Get("vmRef").(*proxmox.VmRef)


To be sure, if the artifact is a template, do we still get the appropriate ID from the vmRef in state?

Note: yes we do, the current way this is implemented is by writing vmRef.VmId() to the state as template_id, which is not necessary since the vmRef is already part of the state.

lbajolet-hashicorp · 2023-10-24T18:39:18Z

Note: removed the bump minor label since in the current state, this does not change how the plugin works, so this can be a patch release.

mfisher87 · 2024-01-28T08:27:07Z

The author of this PR hasn't had any GitHub activity in a year or two. Is there a path forward to merge this?

AnthonyTippy · 2024-03-27T14:45:04Z

checking in on this. Is it possible to merge in this feature? As I understand the current builder methods (ISO, and Clone) only allow you to create templates instead of going straight to a running VM?

Im looking to have packer build out a functional VM from ISO instead of a template. Is this possible in the proxmox environment? Currently we have a functional process using Vsphere but I am currently exploring proxmox as an alternative.

tacoman · 2024-03-27T16:03:32Z

Would love to see this merged in for our current workflow.

lbajolet-hashicorp · 2024-03-28T13:43:59Z

Hi @AnthonyTippy @tacoman @mfisher87,

This looks pretty stale still, haven't heard of feedback from @LethalServant yet (to be fair to them, we waited a really long time for reviewing the code so there's definitely a lot of blame on us).

If someone wants to pickup that PR and add contributions on top to address the comments (that will likely need to be in a separate PR to supersede this one) I posted in my review, that would be very appreciated for sure, please let us know and we'll make sure to review the changes with minimal delay this time around.

LethalServant · 2024-03-29T01:48:47Z

Hey @lbajolet-hashicorp

I started working on this feature back around v1.0.4 so I think the code base has changed a lot and it would take some effort to get this back in mergeable state. Not sure when I'll be able to get to this, if someone else is in a better position to add this feature either by picking this up or starting from scratch, go for it. Cheers!

LethalServant added 4 commits February 27, 2022 18:36

adds convert_to_template option to config and updates docs

cdd14b5

adds convert_to_template to iso config tests and ran go generate on c…

2dfa394

…lone and iso configs

updates proxmox builder steps to follow convert_to_template config op…

fc68e7d

…tion

updates step convert to template and stop vm tests

fc876b4

LethalServant requested a review from a team as a code owner March 7, 2022 01:37

LethalServant changed the title ~~Issue 71~~ [Issue 71] Adds convert_to_template bool option Mar 7, 2022

LethalServant changed the title ~~[Issue 71] Adds convert_to_template bool option~~ [Issue 71] Adds convert_to_template option Mar 7, 2022

LethalServant changed the title ~~[Issue 71] Adds convert_to_template option~~ [Issue 71] Adds convert_to_template option to proxmox builders Mar 7, 2022

sylviamoss suggested changes May 18, 2022

View reviewed changes

nywilken assigned sylviamoss Jun 10, 2022

LethalServant added 2 commits July 30, 2022 21:02

moves convert to template config check to within step

3ca1ef1

Changes ConvertToTemplate option to SkipConvertToTemplate

fcb4b79

LethalServant requested a review from sylviamoss August 1, 2022 23:41

nywilken added the enhancement label Nov 1, 2022

nywilken added the version/bump minor A PR that changes behavior or contains breaking changes template configuration options. label Nov 1, 2022

nywilken requested review from sylviamoss and removed request for sylviamoss November 1, 2022 16:29

nywilken unassigned sylviamoss Nov 1, 2022

lbajolet-hashicorp requested changes Oct 24, 2023

View reviewed changes

lbajolet-hashicorp removed the version/bump minor A PR that changes behavior or contains breaking changes template configuration options. label Oct 24, 2023

mpywell mentioned this pull request Sep 27, 2024

[Issue 71] add skip_convert_to_template option #280

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue 71] Adds convert_to_template option to proxmox builders #72

[Issue 71] Adds convert_to_template option to proxmox builders #72

LethalServant commented Mar 7, 2022 •

edited

Loading

hashicorp-cla commented Mar 7, 2022 •

edited

Loading

carlpett commented Apr 7, 2022

sylviamoss commented May 6, 2022

adrian-hoasted commented May 11, 2022

sylviamoss left a comment

sylviamoss May 18, 2022

LethalServant Aug 1, 2022

sylviamoss May 18, 2022

zakx commented Sep 27, 2022

nywilken commented Nov 1, 2022

lbajolet-hashicorp left a comment

lbajolet-hashicorp Jan 20, 2023

lbajolet-hashicorp Jan 20, 2023

lbajolet-hashicorp Oct 24, 2023

lbajolet-hashicorp Oct 24, 2023

lbajolet-hashicorp Oct 24, 2023

lbajolet-hashicorp Oct 24, 2023

lbajolet-hashicorp Oct 24, 2023

lbajolet-hashicorp Oct 24, 2023

lbajolet-hashicorp Oct 24, 2023

lbajolet-hashicorp Oct 24, 2023

lbajolet-hashicorp Oct 24, 2023

lbajolet-hashicorp commented Oct 24, 2023

mfisher87 commented Jan 28, 2024

AnthonyTippy commented Mar 27, 2024

tacoman commented Mar 27, 2024

lbajolet-hashicorp commented Mar 28, 2024

LethalServant commented Mar 29, 2024

-	if !c.SkipConvertToTemplate.True() {
+	if c.SkipConvertToTemplate {
+		ui.Say("Skipping VM template conversion")
+		return multistep.ActionContinue
+	}


		var _ templateConverter = &proxmox.Client{}

		func (s *stepStopVM) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction {

	isTemplate: !b.config.SkipConvertToTemplate.True(),
	isTemplate: b.config.SkipConvertToTemplate.False(),

[Issue 71] Adds convert_to_template option to proxmox builders #72

Are you sure you want to change the base?

[Issue 71] Adds convert_to_template option to proxmox builders #72

Conversation

LethalServant commented Mar 7, 2022 • edited Loading

hashicorp-cla commented Mar 7, 2022 • edited Loading

carlpett commented Apr 7, 2022

sylviamoss commented May 6, 2022

adrian-hoasted commented May 11, 2022

sylviamoss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zakx commented Sep 27, 2022

nywilken commented Nov 1, 2022

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbajolet-hashicorp commented Oct 24, 2023

mfisher87 commented Jan 28, 2024

AnthonyTippy commented Mar 27, 2024

tacoman commented Mar 27, 2024

lbajolet-hashicorp commented Mar 28, 2024

LethalServant commented Mar 29, 2024

LethalServant commented Mar 7, 2022 •

edited

Loading

hashicorp-cla commented Mar 7, 2022 •

edited

Loading