fix: improve logs messages and error handling (#139)

- Refactor the error handling using a helper function.
- Improve the logs messages and error messages.

builder/hcloud/helper.go

@@ -0,0 +1,21 @@
+package hcloud
+
+import (
+	"fmt"
+
+	"github.com/hashicorp/packer-plugin-sdk/multistep"
+	packersdk "github.com/hashicorp/packer-plugin-sdk/packer"
+)
+
+// errorHandler is a helper function to reduce the amount of bloat and complexity
+// caused by redundant error handling logic.
+func errorHandler(state multistep.StateBag, ui packersdk.Ui, prefix string, err error) multistep.StepAction {
+	wrappedError := err
+	if prefix != "" {
+		wrappedError = fmt.Errorf("%s: %w", prefix, err)
+	}
+
+	state.Put("error", wrappedError)
+	ui.Error(wrappedError.Error())
+	return multistep.ActionHalt
+}

builder/hcloud/step_create_server.go

@@ -33,8 +33,7 @@ func (s *stepCreateServer) Run(ctx context.Context, state multistep.StateBag) mu
 	if c.UserDataFile != "" {
 		contents, err := os.ReadFile(c.UserDataFile)
 		if err != nil {
-			state.Put("error", fmt.Errorf("Problem reading user data file: %s", err))
-			return multistep.ActionHalt
+			return errorHandler(state, ui, "Could not read user data file", err)
 		}
 
 		userData = string(contents)
@@ -44,13 +43,10 @@ func (s *stepCreateServer) Run(ctx context.Context, state multistep.StateBag) mu
 	for _, k := range c.SSHKeys {
 		sshKey, _, err := client.SSHKey.Get(ctx, k)
 		if err != nil {
-			ui.Error(err.Error())
-			state.Put("error", fmt.Errorf("Error fetching SSH key: %s", err))
-			return multistep.ActionHalt
+			return errorHandler(state, ui, fmt.Sprintf("Could not fetch SSH key '%s'", k), err)
 		}
 		if sshKey == nil {
-			state.Put("error", fmt.Errorf("Could not find key: %s", k))
-			return multistep.ActionHalt
+			return errorHandler(state, ui, fmt.Sprintf("Could not find SSH key '%s'", k), err)
 		}
 		sshKeys = append(sshKeys, sshKey)
 	}
@@ -63,9 +59,7 @@ func (s *stepCreateServer) Run(ctx context.Context, state multistep.StateBag) mu
 		var err error
 		image, err = getImageWithSelectors(ctx, client, c, serverType)
 		if err != nil {
-			ui.Error(err.Error())
-			state.Put("error", err)
-			return multistep.ActionHalt
+			return errorHandler(state, ui, "Could not find image", err)
 		}
 		ui.Message(fmt.Sprintf("Using image %s with ID %d", image.Description, image.ID))
 	}
@@ -92,10 +86,7 @@ func (s *stepCreateServer) Run(ctx context.Context, state multistep.StateBag) mu
 
 	serverCreateResult, _, err := client.Server.Create(ctx, serverCreateOpts)
 	if err != nil {
-		err := fmt.Errorf("Error creating server: %s", err)
-		state.Put("error", err)
-		ui.Error(err.Error())
-		return multistep.ActionHalt
+		return errorHandler(state, ui, "Could not create server", err)
 	}
 	state.Put("server_ip", serverCreateResult.Server.PublicNet.IPv4.IP.String())
 	// We use this in cleanup
@@ -108,79 +99,52 @@ func (s *stepCreateServer) Run(ctx context.Context, state multistep.StateBag) mu
 	state.Put("instance_id", serverCreateResult.Server.ID)
 
 	if err := waitForAction(ctx, client, serverCreateResult.Action); err != nil {
-		err := fmt.Errorf("Error creating server: %s", err)
-		state.Put("error", err)
-		ui.Error(err.Error())
-		return multistep.ActionHalt
+		return errorHandler(state, ui, "Could not create server", err)
 	}
 	for _, nextAction := range serverCreateResult.NextActions {
 		if err := waitForAction(ctx, client, nextAction); err != nil {
-			err := fmt.Errorf("Error creating server: %s", err)
-			state.Put("error", err)
-			ui.Error(err.Error())
-			return multistep.ActionHalt
+			return errorHandler(state, ui, "Could not create server", err)
 		}
 	}
 
 	if c.UpgradeServerType != "" {
-		ui.Say("Changing server-type...")
+		ui.Say("Upgrading server type...")
 		serverChangeTypeAction, _, err := client.Server.ChangeType(ctx, serverCreateResult.Server, hcloud.ServerChangeTypeOpts{
 			ServerType:  &hcloud.ServerType{Name: c.UpgradeServerType},
 			UpgradeDisk: false,
 		})
 		if err != nil {
-			err := fmt.Errorf("Error changing server-type: %s", err)
-			state.Put("error", err)
-			ui.Error(err.Error())
-			return multistep.ActionHalt
+			return errorHandler(state, ui, "Could not upgrade server type", err)
 		}
 
 		if err := waitForAction(ctx, client, serverChangeTypeAction); err != nil {
-			err := fmt.Errorf("Error changing server-type: %s", err)
-			state.Put("error", err)
-			ui.Error(err.Error())
-			return multistep.ActionHalt
+			return errorHandler(state, ui, "Could not upgrade server type", err)
 		}
 
 		ui.Say("Starting server...")
 		serverPoweronAction, _, err := client.Server.Poweron(ctx, serverCreateResult.Server)
 		if err != nil {
-			err := fmt.Errorf("Error starting server: %s", err)
-			state.Put("error", err)
-			ui.Error(err.Error())
-			return multistep.ActionHalt
+			return errorHandler(state, ui, "Could not start server", err)
 		}
 
 		if err := waitForAction(ctx, client, serverPoweronAction); err != nil {
-			err := fmt.Errorf("Error starting server: %s", err)
-			state.Put("error", err)
-			ui.Error(err.Error())
-			return multistep.ActionHalt
+			return errorHandler(state, ui, "Could not start server", err)
 		}
 	}
 
 	if c.RescueMode != "" {
 		ui.Say("Enabling Rescue Mode...")
 		_, err := setRescue(ctx, client, serverCreateResult.Server, c.RescueMode, sshKeys)
 		if err != nil {
-			err := fmt.Errorf("Error enabling rescue mode: %s", err)
-			state.Put("error", err)
-			ui.Error(err.Error())
-			return multistep.ActionHalt
+			return errorHandler(state, ui, "Could not enable rescue mode", err)
 		}
-		ui.Say("Reboot server...")
+		ui.Say("Rebooting server...")
 		action, _, err := client.Server.Reset(ctx, serverCreateResult.Server)
 		if err != nil {
-			err := fmt.Errorf("Error rebooting server: %s", err)
-			state.Put("error", err)
-			ui.Error(err.Error())
-			return multistep.ActionHalt
+			return errorHandler(state, ui, "Could not reboot server", err)
 		}
 		if err := waitForAction(ctx, client, action); err != nil {
-			err := fmt.Errorf("Error rebooting server: %s", err)
-			state.Put("error", err)
-			ui.Error(err.Error())
-			return multistep.ActionHalt
+			return errorHandler(state, ui, "Could not reboot server", err)
 		}
 	}
 

builder/hcloud/step_create_snapshot.go

@@ -24,29 +24,23 @@ func (s *stepCreateSnapshot) Run(ctx context.Context, state multistep.StateBag)
 	c := state.Get("config").(*Config)
 	serverID := state.Get("server_id").(int64)
 
-	ui.Say("Creating snapshot ...")
+	ui.Say("Creating snapshot...")
 	ui.Say("This can take some time")
 	result, _, err := client.Server.CreateImage(ctx, &hcloud.Server{ID: serverID}, &hcloud.ServerCreateImageOpts{
 		Type:        hcloud.ImageTypeSnapshot,
 		Labels:      c.SnapshotLabels,
 		Description: hcloud.Ptr(c.SnapshotName),
 	})
 	if err != nil {
-		err := fmt.Errorf("Error creating snapshot: %s", err)
-		state.Put("error", err)
-		ui.Error(err.Error())
-		return multistep.ActionHalt
+		return errorHandler(state, ui, "Could not create snapshot", err)
 	}
 	state.Put("snapshot_id", result.Image.ID)
 	state.Put("snapshot_name", c.SnapshotName)
 	_, errCh := client.Action.WatchProgress(ctx, result.Action)
 
 	err1 := <-errCh
 	if err1 != nil {
-		err := fmt.Errorf("Error creating snapshot: %s", err1)
-		state.Put("error", err)
-		ui.Error(err.Error())
-		return multistep.ActionHalt
+		return errorHandler(state, ui, "Could not create snapshot", err)
 	}
 
 	oldSnap, found := state.GetOk(OldSnapshotID)
@@ -63,10 +57,7 @@ func (s *stepCreateSnapshot) Run(ctx context.Context, state multistep.StateBag)
 	image := &hcloud.Image{ID: oldSnapID}
 	_, err = client.Image.Delete(ctx, image)
 	if err != nil {
-		err := fmt.Errorf("Error deleting old snapshot with ID: %d: %s", oldSnapID, err)
-		state.Put("error", err)
-		ui.Error(err.Error())
-		return multistep.ActionHalt
+		return errorHandler(state, ui, fmt.Sprintf("Could not delete old snapshot id=%d", oldSnapID), err)
 	}
 	return multistep.ActionContinue
 }

builder/hcloud/step_create_sshkey.go

@@ -23,11 +23,10 @@ func (s *stepCreateSSHKey) Run(ctx context.Context, state multistep.StateBag) mu
 	client := state.Get("hcloudClient").(*hcloud.Client)
 	ui := state.Get("ui").(packersdk.Ui)
 	c := state.Get("config").(*Config)
-	ui.Say("Creating temporary ssh key for server...")
+	ui.Say("Uploading temporary SSH key for instance...")
 
 	if c.Comm.SSHPublicKey == nil {
-		ui.Say("No public SSH key found")
-		return multistep.ActionHalt
+		return errorHandler(state, ui, "", fmt.Errorf("missing SSH public key in communicator"))
 	}
 
 	// The name of the public key on the Hetzner Cloud
@@ -40,10 +39,7 @@ func (s *stepCreateSSHKey) Run(ctx context.Context, state multistep.StateBag) mu
 		Labels:    c.SSHKeysLabels,
 	})
 	if err != nil {
-		err := fmt.Errorf("Error creating temporary SSH key: %s", err)
-		state.Put("error", err)
-		ui.Error(err.Error())
-		return multistep.ActionHalt
+		return errorHandler(state, ui, "Could not upload temporary SSH key", err)
 	}
 
 	// We use this to check cleanup
@@ -66,11 +62,9 @@ func (s *stepCreateSSHKey) Cleanup(state multistep.StateBag) {
 	client := state.Get("hcloudClient").(*hcloud.Client)
 	ui := state.Get("ui").(packersdk.Ui)
 
-	ui.Say("Deleting temporary ssh key...")
+	ui.Say("Deleting temporary SSH key...")
 	_, err := client.SSHKey.Delete(context.TODO(), &hcloud.SSHKey{ID: s.keyId})
 	if err != nil {
-		log.Printf("Error cleaning up ssh key: %s", err)
-		ui.Error(fmt.Sprintf(
-			"Error cleaning up ssh key. Please delete the key manually: %s", err))
+		errorHandler(state, ui, "Could not cleanup temporary SSH key", err)
 	}
 }

builder/hcloud/step_pre_validate.go

@@ -25,48 +25,34 @@ func (s *stepPreValidate) Run(ctx context.Context, state multistep.StateBag) mul
 	ui := state.Get("ui").(packersdk.Ui)
 	c := state.Get("config").(*Config)
 
-	ui.Say("Prevalidating server types")
+	ui.Say(fmt.Sprintf("Validating server types: %s", c.ServerType))
 	serverType, _, err := client.ServerType.Get(ctx, c.ServerType)
 	if err != nil {
-		err = fmt.Errorf("Error: getting server type: %w", err)
-		state.Put("error", err)
-		ui.Error(err.Error())
-		return multistep.ActionHalt
+		return errorHandler(state, ui, fmt.Sprintf("Could not fetch server type '%s'", c.ServerType), err)
 	}
 	if serverType == nil {
-		err = fmt.Errorf("Error: server type '%s' not found", c.ServerType)
-		state.Put("error", err)
-		ui.Error(err.Error())
-		return multistep.ActionHalt
+		return errorHandler(state, ui, fmt.Sprintf("Could not find server type '%s'", c.ServerType), err)
 	}
 	state.Put("serverType", serverType)
 
 	if c.UpgradeServerType != "" {
+		ui.Say(fmt.Sprintf("Validating upgrade server types: %s", c.UpgradeServerType))
 		upgradeServerType, _, err := client.ServerType.Get(ctx, c.UpgradeServerType)
 		if err != nil {
-			err = fmt.Errorf("Error: getting upgrade server type: %w", err)
-			state.Put("error", err)
-			ui.Error(err.Error())
-			return multistep.ActionHalt
+			return errorHandler(state, ui, fmt.Sprintf("Could not fetch upgrade server type '%s'", c.UpgradeServerType), err)
 		}
 		if serverType == nil {
-			err = fmt.Errorf("Error: upgrade server type '%s' not found", c.UpgradeServerType)
-			state.Put("error", err)
-			ui.Error(err.Error())
-			return multistep.ActionHalt
+			return errorHandler(state, ui, fmt.Sprintf("Could not find upgrade server type '%s'", c.UpgradeServerType), err)
 		}
 
 		if serverType.Architecture != upgradeServerType.Architecture {
 			// This is also validated by API, but if we validate it here, its faster and we never have to create
 			// a server in the first place. Saving users to first hour of billing.
-			err = fmt.Errorf("Error: server_type and upgrade_server_type have incompatible architectures")
-			state.Put("error", err)
-			ui.Error(err.Error())
-			return multistep.ActionHalt
+			return errorHandler(state, ui, "", fmt.Errorf("server_type and upgrade_server_type have incompatible architectures"))
 		}
 	}
 
-	ui.Say(fmt.Sprintf("Prevalidating snapshot name: %s", s.SnapshotName))
+	ui.Say(fmt.Sprintf("Validating snapshot name: %s", s.SnapshotName))
 
 	// We would like to ask only for snapshots with a certain name using
 	// ImageListOpts{Name: s.SnapshotName}, but snapshots do not have name, they
@@ -77,25 +63,23 @@ func (s *stepPreValidate) Run(ctx context.Context, state multistep.StateBag) mul
 	}
 	snapshots, err := client.Image.AllWithOpts(ctx, opts)
 	if err != nil {
-		err := fmt.Errorf("Error: getting snapshot list: %s", err)
-		state.Put("error", err)
-		ui.Error(err.Error())
-		return multistep.ActionHalt
+		return errorHandler(state, ui, "Could not fetch snapshots", err)
 	}
 
 	for _, snap := range snapshots {
 		if snap.Description == s.SnapshotName {
-			snapMsg := fmt.Sprintf("snapshot name: '%s' is used by existing snapshot with ID %d (arch=%s)",
-				s.SnapshotName, snap.ID, serverType.Architecture)
+			msg := fmt.Sprintf(
+				"Found existing snapshot (id=%d, arch=%s) with name '%s'",
+				snap.ID,
+				serverType.Architecture,
+				s.SnapshotName,
+			)
 			if s.Force {
-				ui.Say(snapMsg + ". Force flag specified, will safely overwrite this snapshot")
+				ui.Say(msg + ". Force flag specified, will safely overwrite this snapshot")
 				state.Put(OldSnapshotID, snap.ID)
 				return multistep.ActionContinue
 			}
-			err := fmt.Errorf("Error: " + snapMsg)
-			state.Put("error", err)
-			ui.Error(err.Error())
-			return multistep.ActionHalt
+			return errorHandler(state, ui, "", fmt.Errorf(msg))
 		}
 	}
 

builder/hcloud/step_shutdown_server.go

@@ -5,7 +5,6 @@ package hcloud
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/hashicorp/packer-plugin-sdk/multistep"
 	packersdk "github.com/hashicorp/packer-plugin-sdk/packer"
@@ -26,10 +25,7 @@ func (s *stepShutdownServer) Run(ctx context.Context, state multistep.StateBag)
 	action, _, err := client.Server.Shutdown(ctx, &hcloud.Server{ID: serverID})
 
 	if err != nil {
-		err := fmt.Errorf("Error stopping server: %s", err)
-		state.Put("error", err)
-		ui.Error(err.Error())
-		return multistep.ActionHalt
+		return errorHandler(state, ui, "Error stopping server", err)
 	}
 
 	_, errCh := client.Action.WatchProgress(ctx, action)
@@ -39,10 +35,7 @@ func (s *stepShutdownServer) Run(ctx context.Context, state multistep.StateBag)
 			if err1 == nil {
 				return multistep.ActionContinue
 			} else {
-				err := fmt.Errorf("Error stopping server: %s", err)
-				state.Put("error", err)
-				ui.Error(err.Error())
-				return multistep.ActionHalt
+				return errorHandler(state, ui, "Error stopping server", err)
 			}
 		}
 	}