rework of delete/keep after build and updated output message to show details of artifact created

Author: rowansmithau

builder/proxmox/ct/artifact.go

@@ -0,0 +1,84 @@
+// FILE 1: artifact.go - CREATE THIS NEW FILE
+// Location: /builder/proxmox/ct/artifact.go
+// This is a completely new file - copy everything below
+
+package proxmoxct
+
+import (
+	"fmt"
+	"log"
+
+	"github.com/Telmate/proxmox-api-go/proxmox"
+)
+
+// Artifact represents a Proxmox container template
+type Artifact struct {
+	builderID     string
+	templateID    int
+	nodeName      string
+	templateName  string
+	proxmoxClient *proxmox.Client
+	stateData     map[string]interface{}
+}
+
+// BuilderId returns the builder ID
+func (a *Artifact) BuilderId() string {
+	return a.builderID
+}
+
+// Files returns the files associated with the artifact
+// For Proxmox containers, we don't have local files
+func (a *Artifact) Files() []string {
+	return nil
+}
+
+// Id returns a string identifier for the artifact
+func (a *Artifact) Id() string {
+	return fmt.Sprintf("%s:%d", a.nodeName, a.templateID)
+}
+
+// String returns a human-readable string representation
+func (a *Artifact) String() string {
+	if a.templateName != "" {
+		return fmt.Sprintf("Container template '%s' (ID: %d) created on node '%s'",
+			a.templateName, a.templateID, a.nodeName)
+	}
+	return fmt.Sprintf("Container template (ID: %d) created on node '%s'",
+		a.templateID, a.nodeName)
+}
+
+// State returns state data for the artifact
+func (a *Artifact) State(name string) interface{} {
+	if a.stateData != nil {
+		return a.stateData[name]
+	}
+	return nil
+}
+
+// Destroy removes the container template from Proxmox
+func (a *Artifact) Destroy() error {
+	log.Printf("[INFO] Destroying container template %d on node %s", a.templateID, a.nodeName)
+
+	// Create a VM reference for the container
+	vmRef := proxmox.NewVmRef(a.templateID)
+	vmRef.SetNode(a.nodeName)
+	vmRef.SetVmType("lxc")
+
+	// First, try to stop the container if it's running
+	// This is important for containers that might not be templates yet
+	_, stopErr := a.proxmoxClient.StopVm(vmRef)
+	if stopErr != nil {
+		// It's okay if stop fails - container might already be stopped or be a template
+		log.Printf("[WARN] Could not stop container %d (this is normal for templates): %s",
+			a.templateID, stopErr)
+	}
+
+	// Delete the container/template
+	_, err := a.proxmoxClient.DeleteVm(vmRef)
+	if err != nil {
+		return fmt.Errorf("error destroying container template %d: %s", a.templateID, err)
+	}
+
+	log.Printf("[INFO] Successfully destroyed container template %d", a.templateID)
+	return nil
+}

builder/proxmox/ct/builder.go

@@ -1,3 +1,7 @@
+// FILE 2: builder.go - REPLACE YOUR ENTIRE FILE WITH THIS
+// Location: /builder/proxmox/ct/builder.go
+// Replace your entire existing builder.go with everything below
+
 package proxmoxct
 
 import (
@@ -6,13 +10,13 @@ import (
 	"fmt"
 	"net/url"
 	"strings"
-	
+
 	"github.com/hashicorp/hcl/v2/hcldec"
-	packersdk "github.com/hashicorp/packer-plugin-sdk/packer"
+	"github.com/hashicorp/packer-plugin-sdk/communicator"
 	"github.com/hashicorp/packer-plugin-sdk/multistep"
 	"github.com/hashicorp/packer-plugin-sdk/multistep/commonsteps"
-	"github.com/hashicorp/packer-plugin-sdk/communicator"
-	
+	packersdk "github.com/hashicorp/packer-plugin-sdk/packer"
+
 	proxmox "github.com/Telmate/proxmox-api-go/proxmox"
 	common "github.com/hashicorp/packer-plugin-proxmox/builder/proxmox/common"
 )
@@ -50,44 +54,80 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook)
 	steps := []multistep.Step{
 		new(stepCtCreate),
 	}
-	
+
 	// Only add communicator steps if not using "none"
 	if b.config.Comm.Type != "none" {
 		steps = append(steps,
 			// Go straight to SSH connection - it will use ssh_host from config
 			&communicator.StepConnect{
 				Config:    &b.config.Comm,
-				Host:      commHost(b.config.Comm.Host()),  // Pass the configured host
+				Host:      commHost(b.config.Comm.Host()), // Pass the configured host
 				SSHConfig: b.config.Comm.SSHConfigFunc(),
 			},
 			new(stepProvision),
 		)
 	}
-	
+
 	// Add our own template conversion step for containers
 	steps = append(steps, new(stepConvertCtToTemplate))
-	
-	// Mark as successful
-	state.Put("success", true)
 
 	// Run the steps
 	runner := commonsteps.NewRunner(steps, b.config.PackerConfig, ui)
 	runner.Run(ctx, state)
 
 	// If there was an error, return that
+	// The stepCtCreate.Cleanup() method will handle container deletion
 	if rawErr, ok := state.GetOk("error"); ok {
 		return nil, rawErr.(error)
 	}
 
-	// Get the container ID
-	if vmRefUntyped, ok := state.GetOk("vmRef"); ok {
-		vmRef := vmRefUntyped.(*proxmox.VmRef)
-		templateID := vmRef.VmId()
-		ui.Say(fmt.Sprintf("Container template created successfully: %d", templateID))
+	// Check if the run was cancelled
+	if _, ok := state.GetOk(multistep.StateCancelled); ok {
+		return nil, fmt.Errorf("build was cancelled")
+	}
+
+	// Check if the run was halted
+	if _, ok := state.GetOk(multistep.StateHalted); ok {
+		return nil, fmt.Errorf("build was halted")
+	}
+
+	// Get the container reference
+	vmRefUntyped, ok := state.GetOk("vmRef")
+	if !ok {
+		return nil, fmt.Errorf("no container reference found in state")
+	}
+
+	vmRef := vmRefUntyped.(*proxmox.VmRef)
+	templateID := vmRef.VmId()
+	nodeName := vmRef.Node()
+
+	// Get template name if it was set
+	var templateName string
+	if name, ok := state.GetOk("template_name"); ok {
+		templateName = name.(string)
+	}
+
+	ui.Say(fmt.Sprintf("Container template created successfully: %d on node %s", templateID, nodeName))
+
+	// Mark that we've created an artifact successfully
+	// This prevents the cleanup from deleting the template
+	state.Put("artifact_created", true)
+
+	// Create and return the artifact
+	artifact := &Artifact{
+		builderID:     BuilderID,
+		templateID:    templateID,
+		nodeName:      nodeName,
+		templateName:  templateName,
+		proxmoxClient: client,
+		stateData: map[string]interface{}{
+			"vmid":        templateID,
+			"node":        nodeName,
+			"proxmox_url": b.config.ProxmoxConnect.ProxmoxURLRaw,
+		},
 	}
-	
-	// Return nil artifact for now - the container was created successfully
-	return nil, nil
+
+	return artifact, nil
 }
 
 func commHost(host string) func(multistep.StateBag) (string, error) {
@@ -142,19 +182,24 @@ func (s *stepConvertCtToTemplate) Run(ctx context.Context, state multistep.State
 
 	if c.Template {
 		ui.Say("Converting container to template")
-		
+
 		// Stop the container first if it's running
 		_, err := client.StopVm(vmRef)
 		if err != nil {
 			// It's ok if it's already stopped
 			ui.Say(fmt.Sprintf("Note: %s", err))
 		}
-		
+
 		// TODO: Implement actual template conversion for containers
 		// For now, we'll just mark it as successful
 		ui.Say(fmt.Sprintf("Container %d marked as template (implementation pending)", vmRef.VmId()))
+
+		// Store template name if provided
+		if c.TemplateName != "" {
+			state.Put("template_name", c.TemplateName)
+		}
 	}
-	
+
 	return multistep.ActionContinue
 }
 

builder/proxmox/ct/config.go

@@ -27,8 +27,8 @@ import (
 
 type Config struct {
 	common.PackerConfig `mapstructure:",squash"`
-	Comm                communicator.Config      `mapstructure:",squash"`
-	ProxmoxConnect      proxmoxcommon.Config     `mapstructure:",squash"`
+	Comm                communicator.Config  `mapstructure:",squash"`
+	ProxmoxConnect      proxmoxcommon.Config `mapstructure:",squash"`
 
 	// Required
 	OsTemplate string `mapstructure:"os_template"`
@@ -69,6 +69,7 @@ type Config struct {
 	Swap               int                       `mapstructure:"swap"`
 	Tags               []string                  `mapstructure:"tags"`
 	Template           bool                      `mapstructure:"template"`
+	TemplateName       string                    `mapstructure:"template_name"`
 	Timezone           string                    `mapstructure:"timezone"`
 	TTY                int                       `mapstructure:"tty"`
 	Unique             bool                      `mapstructure:"unique"`
@@ -189,25 +190,25 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, []string, error) {
 		// Default to packer-[time-ordered-uuid]
 		c.Hostname = fmt.Sprintf("packer-%s", uuid.TimeOrderedUUID())
 	}
-	
+
 	// Enable console by default for containers
 	if !c.Console {
 		c.Console = true
 	}
-	
+
 	if !c.Unprivileged {
-	// Check if it was in the metadata (meaning it was explicitly set)
-	    hasUnprivileged := false
-	    for _, key := range md.Keys {
-	        if key == "unprivileged" {
-	            hasUnprivileged = true
-	            break
-	        }
-	    }
-	    // Only set to true if it wasn't explicitly configured
-	    if !hasUnprivileged {
-	        c.Unprivileged = true
-	    }
+		// Check if it was in the metadata (meaning it was explicitly set)
+		hasUnprivileged := false
+		for _, key := range md.Keys {
+			if key == "unprivileged" {
+				hasUnprivileged = true
+				break
+			}
+		}
+		// Only set to true if it wasn't explicitly configured
+		if !hasUnprivileged {
+			c.Unprivileged = true
+		}
 	}
 
 	// Validation

builder/proxmox/ct/container_test.go

@@ -0,0 +1,68 @@
+// FILE: container_test.go - FIXED VERSION
+// The issue was using "User" instead of "SSHUsername" in communicator.Config
+
+package proxmoxct
+
+import (
+	"testing"
+
+	common "github.com/hashicorp/packer-plugin-proxmox/builder/proxmox/common"
+	"github.com/hashicorp/packer-plugin-sdk/communicator"
+)
+
+func TestConfig_Prepare_Minimal(t *testing.T) {
+	cfg := &Config{
+		Comm: communicator.Config{
+			Type: "none", // No SSH needed for basic container tests
+		},
+		Hostname:     "test-container",
+		OsTemplate:   "local:vztmpl/debian-11-standard_11.0-1_amd64.tar.gz",
+		UserPassword: "testpass",
+		Storage:      "local-lvm",
+		Memory:       512,
+		Cores:        1,
+		VMID:         1001,
+		ProxmoxConnect: common.Config{
+			Node:          "pve-node",
+			ProxmoxURLRaw: "https://proxmox.example.com:8006/api2/json",
+			Username:      "root@pam",
+			Password:      "dummy",
+		},
+		RootFS: &MountPointConfig{
+			StorageId:  "local-lvm",
+			DiskSizeGB: 8,
+		},
+	}
+	warnings, errors, err := cfg.Prepare(nil)
+	if err != nil {
+		t.Fatalf("Prepare returned error: %v", err)
+	}
+	if len(errors) > 0 {
+		t.Errorf("Prepare returned errors: %v", errors)
+	}
+	if len(warnings) > 0 {
+		t.Logf("Prepare returned warnings: %v", warnings)
+	}
+}
+
+func TestBuilder_ConfigSpec(t *testing.T) {
+	b := &Builder{}
+	if b.ConfigSpec() == nil {
+		t.Error("ConfigSpec should not return nil")
+	}
+}
+
+func TestBuilder_Prepare_Invalid(t *testing.T) {
+	b := &Builder{}
+	_, errors, err := b.Prepare(map[string]interface{}{"invalid": true})
+	if err == nil && len(errors) == 0 {
+		t.Error("Expected error or errors for invalid config")
+	}
+}
+
+func TestNewProxmoxClient_InvalidConfig(t *testing.T) {
+	_, err := newProxmoxClient(common.Config{})
+	if err == nil {
+		t.Error("Expected error for invalid config")
+	}
+}