Parallels: fix and simplify Prlctl/Prlsrvctl handling (#284)

* Simplify Prlctl/Prlsrvctl error handling

* Don't try to find Parallels messages in non-exec.ExitError errors

* runParallelsCommand: fix logic

internal/executor/instance/persistentworker/isolation/parallels/cli.go

@@ -6,9 +6,13 @@ import (
 	"errors"
 	"fmt"
 	"os/exec"
+	"strings"
 )
 
-var ErrParallelsCommandNotFound = errors.New("Parallels command not found")
+var (
+	ErrParallelsCommandNotFound = errors.New("Parallels command not found")
+	ErrParallelsCommandNonZero  = errors.New("Parallels command returned non-zero exit code")
+)
 
 func runParallelsCommand(ctx context.Context, commandName string, args ...string) (string, string, error) {
 	cmd := exec.CommandContext(ctx, commandName, args...)
@@ -23,6 +27,12 @@ func runParallelsCommand(ctx context.Context, commandName string, args ...string
 			return "", "", fmt.Errorf("%w: %q not found in PATH, make sure Parallels is installed",
 				ErrParallelsCommandNotFound, commandName)
 		}
+
+		if _, ok := err.(*exec.ExitError); ok {
+			// Parallels command failed, redefine the error
+			// to be the Parallels-specific output
+			err = fmt.Errorf("%w: %q", ErrParallelsCommandNonZero, firstNonEmptyLine(stderr.String(), stdout.String()))
+		}
 	}
 
 	return stdout.String(), stderr.String(), err
@@ -35,3 +45,15 @@ func Prlctl(ctx context.Context, args ...string) (string, string, error) {
 func Prlsrvctl(ctx context.Context, args ...string) (string, string, error) {
 	return runParallelsCommand(ctx, "prlsrvctl", args...)
 }
+
+func firstNonEmptyLine(outputs ...string) string {
+	for _, output := range outputs {
+		for _, line := range strings.Split(output, "\n") {
+			if line != "" {
+				return line
+			}
+		}
+	}
+
+	return ""
+}

internal/executor/instance/persistentworker/isolation/parallels/cli_test.go

@@ -0,0 +1,32 @@
+package parallels_test
+
+import (
+	"context"
+	"github.com/cirruslabs/cirrus-cli/internal/executor/instance/persistentworker/isolation/parallels"
+	"github.com/stretchr/testify/assert"
+	"os"
+	"testing"
+)
+
+func TestPrlctl(t *testing.T) {
+	ctx := context.Background()
+
+	_, imageOk := os.LookupEnv("CIRRUS_INTERNAL_PARALLELS_VM")
+	_, userOk := os.LookupEnv("CIRRUS_INTERNAL_PARALLELS_SSH_USER")
+	_, passwordOk := os.LookupEnv("CIRRUS_INTERNAL_PARALLELS_SSH_PASSWORD")
+	if !imageOk || !userOk || !passwordOk {
+		t.SkipNow()
+	}
+
+	// Working example
+	_, _, err := parallels.Prlctl(ctx, "list")
+	assert.NoError(t, err)
+
+	// Non-working example
+	expectedErrorMessage := `Parallels command returned non-zero exit code: "Failed to get VM config: The virtual` +
+		` machine could not be found. The virtual machine is not registered in the virtual machine directory on your Mac."`
+
+	_, _, err = parallels.Prlctl(ctx, "list", "non-existent-vm")
+	assert.Error(t, err)
+	assert.Equal(t, expectedErrorMessage, err.Error())
+}

internal/executor/instance/persistentworker/isolation/parallels/clone.go

@@ -27,13 +27,12 @@ func cloneFromSuspended(ctx context.Context, vmPathFrom string) (*VM, error) {
 		return nil, fmt.Errorf("%w: failed to copy VM from %q to %q: %v", ErrVMFailed, vmPathFrom, newHome, err)
 	}
 
-	stdout, stderr, err := Prlctl(ctx, "register", newHome, "--uuid", vm.uuid)
+	_, _, err = Prlctl(ctx, "register", newHome, "--uuid", vm.uuid)
 	if err != nil {
 		// Cleanup
 		_ = os.RemoveAll(newHome)
 
-		return nil, fmt.Errorf("%w: failed to import VM from %q: %q", ErrVMFailed, newHome,
-			firstNonEmptyLine(stderr, stdout))
+		return nil, fmt.Errorf("%w: failed to import VM from %q: %v", ErrVMFailed, newHome, err)
 	}
 
 	return vm, nil
@@ -44,10 +43,9 @@ func cloneFromDefault(ctx context.Context, vmNameFrom string) (*VM, error) {
 		name: fmt.Sprintf("cirrus-%s", uuid.New().String()),
 	}
 
-	stdout, stderr, err := Prlctl(ctx, "clone", vmNameFrom, "--name", vm.Ident())
+	_, _, err := Prlctl(ctx, "clone", vmNameFrom, "--name", vm.Ident())
 	if err != nil {
-		return nil, fmt.Errorf("%w: failed to clone VM %q: %q", ErrVMFailed, vm.Ident(),
-			firstNonEmptyLine(stderr, stdout))
+		return nil, fmt.Errorf("%w: failed to clone VM %q: %v", ErrVMFailed, vm.Ident(), err)
 	}
 
 	return vm, nil

internal/executor/instance/persistentworker/isolation/parallels/server.go

@@ -11,10 +11,9 @@ type ServerInfo struct {
 }
 
 func GetServerInfo(ctx context.Context) (*ServerInfo, error) {
-	stdout, stderr, err := Prlsrvctl(ctx, "info", "--json")
+	stdout, _, err := Prlsrvctl(ctx, "info", "--json")
 	if err != nil {
-		return nil, fmt.Errorf("%w: failed to retrieve Parallels server info: %q",
-			ErrVMFailed, firstNonEmptyLine(stderr, stdout))
+		return nil, fmt.Errorf("%w: failed to retrieve Parallels server info: %v", ErrVMFailed, err)
 	}
 
 	var serverInfo ServerInfo

internal/executor/instance/persistentworker/isolation/parallels/vm.go

@@ -49,10 +49,9 @@ func NewVMClonedFrom(ctx context.Context, vmNameFrom string) (*VM, error) {
 	// Check if VM is packed
 	if strings.HasSuffix(vmInfoFrom.Home, ".pvmp") {
 		// Let's unpack it!
-		stdout, stderr, err := Prlctl(ctx, "unpack", vmNameFrom)
+		_, _, err = Prlctl(ctx, "unpack", vmNameFrom)
 		if err != nil {
-			return nil, fmt.Errorf("%w: failed to unpack VM %q: %q", ErrVMFailed, vmNameFrom,
-				firstNonEmptyLine(stderr, stdout))
+			return nil, fmt.Errorf("%w: failed to unpack VM %q: %v", ErrVMFailed, vmNameFrom, err)
 		}
 		// Update info after unpacking
 		vmInfoFrom, err = retrieveInfo(ctx, vmNameFrom)
@@ -75,10 +74,9 @@ func (vm *VM) Start(ctx context.Context) error {
 		}
 	}
 
-	stdout, stderr, err := Prlctl(ctx, "start", vm.Ident())
+	_, _, err := Prlctl(ctx, "start", vm.Ident())
 	if err != nil {
-		return fmt.Errorf("%w: failed to start VM %q: %q", ErrVMFailed, vm.Ident(),
-			firstNonEmptyLine(stderr, stdout))
+		return fmt.Errorf("%w: failed to start VM %q: %v", ErrVMFailed, vm.Ident(), err)
 	}
 
 	if vm.shouldRenewDHCP {
@@ -109,21 +107,19 @@ func (vm *VM) isolate(ctx context.Context) error {
 	// Ensure that the VM is isolated[1] from the host (e.g. shared folders, clipboard, etc.)
 	// nolint:lll // https://github.com/walle/lll/issues/12
 	// [1]: https://download.parallels.com/desktop/v14/docs/en_US/Parallels%20Desktop%20Pro%20Edition%20Command-Line%20Reference/43645.htm
-	stdout, stderr, err := Prlctl(ctx, "set", vm.Ident(), "--isolate-vm", "on")
+	_, _, err := Prlctl(ctx, "set", vm.Ident(), "--isolate-vm", "on")
 	if err != nil {
-		return fmt.Errorf("%w: failed to isolate VM %q: %q", ErrVMFailed, vm.Ident(),
-			firstNonEmptyLine(stderr, stdout))
+		return fmt.Errorf("%w: failed to isolate VM %q: %v", ErrVMFailed, vm.Ident(), err)
 	}
 
 	return nil
 }
 
 func (vm *VM) renewDHCP(ctx context.Context) error {
 	// Poke DHCP to renew a lease because suspended on another host VMs don't yet have IPs on the current host
-	stdout, stderr, err := Prlctl(ctx, "set", vm.Ident(), "--dhcp", "yes", "--dhcp6", "yes")
+	_, _, err := Prlctl(ctx, "set", vm.Ident(), "--dhcp", "yes", "--dhcp6", "yes")
 	if err != nil {
-		return fmt.Errorf("%w: failed to poke DHCP for VM %q: %q", ErrVMFailed, vm.Ident(),
-			firstNonEmptyLine(stderr, stdout))
+		return fmt.Errorf("%w: failed to poke DHCP for VM %q: %v", ErrVMFailed, vm.Ident(), err)
 	}
 
 	return nil
@@ -132,26 +128,23 @@ func (vm *VM) renewDHCP(ctx context.Context) error {
 func (vm *VM) Close() error {
 	ctx := context.Background()
 
-	stdout, stderr, err := Prlctl(ctx, "stop", vm.Ident(), "--kill")
+	_, _, err := Prlctl(ctx, "stop", vm.Ident(), "--kill")
 	if err != nil {
-		return fmt.Errorf("%w: failed to stop VM %q: %q", ErrVMFailed, vm.Ident(),
-			firstNonEmptyLine(stderr, stdout))
+		return fmt.Errorf("%w: failed to stop VM %q: %q", ErrVMFailed, vm.Ident(), err)
 	}
 
-	_, stderr, err = Prlctl(ctx, "delete", vm.Ident())
+	_, _, err = Prlctl(ctx, "delete", vm.Ident())
 	if err != nil {
-		return fmt.Errorf("%w: failed to delete VM %q: %q", ErrVMFailed, vm.Ident(),
-			firstNonEmptyLine(stderr, stdout))
+		return fmt.Errorf("%w: failed to delete VM %q: %q", ErrVMFailed, vm.Ident(), err)
 	}
 
 	return nil
 }
 
 func retrieveInfo(ctx context.Context, ident string) (*VirtualMachineInfo, error) {
-	stdout, stderr, err := Prlctl(ctx, "list", "--info", "--json", ident)
+	stdout, _, err := Prlctl(ctx, "list", "--info", "--json", ident)
 	if err != nil {
-		return nil, fmt.Errorf("%w: failed to get info for VM with %q UUID or name: %q",
-			ErrVMFailed, ident, firstNonEmptyLine(stderr, stdout))
+		return nil, fmt.Errorf("%w: failed to get info for VM with %q UUID or name: %v", ErrVMFailed, ident, err)
 	}
 
 	var vmInfos []VirtualMachineInfo
@@ -190,15 +183,3 @@ func (vm *VM) RetrieveIP(ctx context.Context) (string, error) {
 
 	return lease.IP, nil
 }
-
-func firstNonEmptyLine(outputs ...string) string {
-	for _, output := range outputs {
-		for _, line := range strings.Split(output, "\n") {
-			if line != "" {
-				return line
-			}
-		}
-	}
-
-	return ""
-}