Add more finalizetemplate tests, fix found bug

builder/proxmox/step_finalize_template_config.go

@@ -45,7 +45,7 @@ func (s *stepFinalizeTemplateConfig) Run(ctx context.Context, state multistep.St
 			return multistep.ActionHalt
 		}
 
-		if !strings.HasSuffix(vmParams["ide2"].(string), "media=cdrom") {
+		if vmParams["ide2"] == nil || !strings.HasSuffix(vmParams["ide2"].(string), "media=cdrom") {
 			err := fmt.Errorf("Cannot eject ISO from cdrom drive, ide2 is not present, or not a cdrom media")
 			state.Put("error", err)
 			ui.Error(err.Error())

builder/proxmox/step_finalize_template_config_test.go

@@ -2,6 +2,7 @@ package proxmox
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/Telmate/proxmox-api-go/proxmox"
@@ -22,42 +23,127 @@ func (m finalizerMock) SetVmConfig(vmref *proxmox.VmRef, c map[string]interface{
 }
 
 func TestTemplateFinalize(t *testing.T) {
-	finalizer := finalizerMock{
-		getConfig: func() (map[string]interface{}, error) {
-			return map[string]interface{}{
+	cs := []struct {
+		name                string
+		builderConfig       *Config
+		initialVMConfig     map[string]interface{}
+		getConfigErr        error
+		expectCallSetConfig bool
+		expectedVMConfig    map[string]interface{}
+		setConfigErr        error
+		expectedAction      multistep.StepAction
+	}{
+		{
+			name:          "empty config changes only description",
+			builderConfig: &Config{},
+			initialVMConfig: map[string]interface{}{
 				"name":        "dummy",
 				"description": "Packer ephemeral build VM",
 				"ide2":        "local:iso/Fedora-Server-dvd-x86_64-29-1.2.iso,media=cdrom",
-			}, nil
+			},
+			expectCallSetConfig: true,
+			expectedVMConfig: map[string]interface{}{
+				"name":        nil,
+				"description": "",
+				"ide2":        nil,
+			},
+			expectedAction: multistep.ActionContinue,
 		},
-		setConfig: func(c map[string]interface{}) (string, error) {
-			if c["name"] != "my-template" {
-				t.Errorf("Expected name to be my-template, got %q", c["name"])
-			}
-			if c["description"] != "foo" {
-				t.Errorf("Expected description to be foo, got %q", c["description"])
-			}
-			if c["ide2"] != "none,media=cdrom" {
-				t.Errorf("Expected ide2 to be none,media=cdrom, got %q", c["ide2"])
-			}
-
-			return "", nil
+		{
+			name: "all options",
+			builderConfig: &Config{
+				TemplateName:        "my-template",
+				TemplateDescription: "some-description",
+				UnmountISO:          true,
+			},
+			initialVMConfig: map[string]interface{}{
+				"name":        "dummy",
+				"description": "Packer ephemeral build VM",
+				"ide2":        "local:iso/Fedora-Server-dvd-x86_64-29-1.2.iso,media=cdrom",
+			},
+			expectCallSetConfig: true,
+			expectedVMConfig: map[string]interface{}{
+				"name":        "my-template",
+				"description": "some-description",
+				"ide2":        "none,media=cdrom",
+			},
+			expectedAction: multistep.ActionContinue,
+		},
+		{
+			name: "no cd-drive with unmount=true should returns halt",
+			builderConfig: &Config{
+				TemplateName:        "my-template",
+				TemplateDescription: "some-description",
+				UnmountISO:          true,
+			},
+			initialVMConfig: map[string]interface{}{
+				"name":        "dummy",
+				"description": "Packer ephemeral build VM",
+				"ide1":        "local:iso/Fedora-Server-dvd-x86_64-29-1.2.iso,media=cdrom",
+			},
+			expectCallSetConfig: false,
+			expectedAction:      multistep.ActionHalt,
+		},
+		{
+			name: "GetVmConfig error should return halt",
+			builderConfig: &Config{
+				TemplateName:        "my-template",
+				TemplateDescription: "some-description",
+				UnmountISO:          true,
+			},
+			getConfigErr:        fmt.Errorf("some error"),
+			expectCallSetConfig: false,
+			expectedAction:      multistep.ActionHalt,
+		},
+		{
+			name: "SetVmConfig error should return halt",
+			builderConfig: &Config{
+				TemplateName:        "my-template",
+				TemplateDescription: "some-description",
+				UnmountISO:          true,
+			},
+			initialVMConfig: map[string]interface{}{
+				"name":        "dummy",
+				"description": "Packer ephemeral build VM",
+				"ide2":        "local:iso/Fedora-Server-dvd-x86_64-29-1.2.iso,media=cdrom",
+			},
+			expectCallSetConfig: true,
+			setConfigErr:        fmt.Errorf("some error"),
+			expectedAction:      multistep.ActionHalt,
 		},
 	}
 
-	state := new(multistep.BasicStateBag)
-	state.Put("ui", packer.TestUi(t))
-	state.Put("config", &Config{
-		TemplateName:        "my-template",
-		TemplateDescription: "foo",
-		UnmountISO:          true,
-	})
-	state.Put("vmRef", proxmox.NewVmRef(1))
-	state.Put("proxmoxClient", finalizer)
-
-	step := stepFinalizeTemplateConfig{}
-	action := step.Run(context.TODO(), state)
-	if action != multistep.ActionContinue {
-		t.Error("Expected action to be Continue, got Halt")
+	for _, c := range cs {
+		t.Run(c.name, func(t *testing.T) {
+			finalizer := finalizerMock{
+				getConfig: func() (map[string]interface{}, error) {
+					return c.initialVMConfig, c.getConfigErr
+				},
+				setConfig: func(cfg map[string]interface{}) (string, error) {
+					if !c.expectCallSetConfig {
+						t.Error("Did not expect SetVmConfig to be called")
+					}
+					for key, val := range c.expectedVMConfig {
+						if cfg[key] != val {
+							t.Errorf("Expected %q to be %q, got %q", key, val, cfg[key])
+						}
+					}
+
+					return "", c.setConfigErr
+				},
+			}
+
+			state := new(multistep.BasicStateBag)
+			state.Put("ui", packer.TestUi(t))
+			state.Put("config", c.builderConfig)
+			state.Put("vmRef", proxmox.NewVmRef(1))
+			state.Put("proxmoxClient", finalizer)
+
+			step := stepFinalizeTemplateConfig{}
+			action := step.Run(context.TODO(), state)
+			if action != c.expectedAction {
+				t.Errorf("Expected action to be %v, got %v", c.expectedAction, action)
+			}
+		})
 	}
 }