Move error wrapping to overrides

Author: pkosiec

cmd/workspace/apps/apps.go

@@ -1,13 +1,43 @@
 package apps
 
 import (
+	"errors"
 	"fmt"
 
+	"github.com/databricks/databricks-sdk-go/apierr"
+	"github.com/databricks/databricks-sdk-go/retries"
 	"github.com/spf13/cobra"
 )
 
 const tailLinesSuggestedValue = 100
 
+// isDeploymentWaitError checks if the error is from the deployment wait phase.
+// These are errors wrapped by retries.Halt() during GetWithTimeout().
+// Excludes API client errors (4xx) which are validation errors before deployment starts.
+func isDeploymentWaitError(err error) bool {
+	var retriesErr *retries.Err
+	if !errors.As(err, &retriesErr) || !retriesErr.Halt {
+		return false
+	}
+
+	// Exclude API client errors (4xx) (e.g. app not found)
+	var apiErr *apierr.APIError
+	if errors.As(err, &apiErr) && apiErr.StatusCode >= 400 && apiErr.StatusCode < 500 {
+		return false
+	}
+
+	return true
+}
+
+// wrapDeploymentError wraps the error with logs hint if it's a deployment wait error.
+// Returns the original error unchanged if it's not a deployment wait error.
+func wrapDeploymentError(cmd *cobra.Command, appName string, err error) error {
+	if err != nil && isDeploymentWaitError(err) {
+		return newAppDeploymentError(cmd, appName, err)
+	}
+	return err
+}
+
 // AppDeploymentError wraps deployment errors with a helpful logs command suggestion.
 type AppDeploymentError struct {
 	Underlying error

cmd/workspace/apps/errors.go

@@ -2,8 +2,12 @@ package apps
 
 import (
 	"errors"
+	"fmt"
 	"testing"
 
+	"github.com/databricks/databricks-sdk-go/apierr"
+	"github.com/databricks/databricks-sdk-go/retries"
+	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -52,3 +56,113 @@ func TestAppDeploymentError_Unwrap(t *testing.T) {
 	require.Equal(t, originalErr, unwrapped)
 	assert.ErrorIs(t, appErr, originalErr, "errors.Is should work with wrapped error")
 }
+
+func TestWrapDeploymentError(t *testing.T) {
+	tests := []struct {
+		name          string
+		err           error
+		appName       string
+		expectWrapped bool
+		description   string
+	}{
+		{
+			name:          "nil error",
+			err:           nil,
+			appName:       "test-app",
+			expectWrapped: false,
+			description:   "nil error should return nil unchanged",
+		},
+		{
+			name:          "plain error without retries wrapper",
+			err:           errors.New("some error"),
+			appName:       "test-app",
+			expectWrapped: false,
+			description:   "plain errors should not be wrapped",
+		},
+		{
+			name:          "retries.Err with Halt=false",
+			err:           retries.Continues("still in progress"),
+			appName:       "test-app",
+			expectWrapped: false,
+			description:   "transient retries errors should not be wrapped",
+		},
+		{
+			name: "retries.Err with 404 API error (not found)",
+			err: retries.Halt(fmt.Errorf("API error: %w", &apierr.APIError{
+				StatusCode: 404,
+				ErrorCode:  "NOT_FOUND",
+				Message:    "App with name test-app does not exist or is deleted.",
+			})),
+			appName:       "test-app",
+			expectWrapped: false,
+			description:   "404 not found errors should not be wrapped with logs hint",
+		},
+		{
+			name: "retries.Err with 400 API error (bad request)",
+			err: retries.Halt(fmt.Errorf("API error: %w", &apierr.APIError{
+				StatusCode: 400,
+				ErrorCode:  "BAD_REQUEST",
+				Message:    "Invalid request parameters",
+			})),
+			appName:       "test-app",
+			expectWrapped: false,
+			description:   "400 bad request errors should not be wrapped with logs hint",
+		},
+		{
+			name: "retries.Err with 403 API error (forbidden)",
+			err: retries.Halt(fmt.Errorf("API error: %w", &apierr.APIError{
+				StatusCode: 403,
+				ErrorCode:  "FORBIDDEN",
+				Message:    "Access denied",
+			})),
+			appName:       "test-app",
+			expectWrapped: false,
+			description:   "403 forbidden errors should not be wrapped with logs hint",
+		},
+		{
+			name: "retries.Err with 500 API error (server error)",
+			err: retries.Halt(fmt.Errorf("API error: %w", &apierr.APIError{
+				StatusCode: 500,
+				ErrorCode:  "INTERNAL_ERROR",
+				Message:    "Internal server error",
+			})),
+			appName:       "test-app",
+			expectWrapped: true,
+			description:   "500 server errors during wait should be wrapped with logs hint",
+		},
+		{
+			name:          "retries.Err without API error (deployment failure)",
+			err:           retries.Halt(errors.New("failed to reach SUCCEEDED, got FAILED: Error building app")),
+			appName:       "test-app",
+			expectWrapped: true,
+			description:   "deployment failures should be wrapped with logs hint",
+		},
+		{
+			name:          "retries.Err without API error (timeout)",
+			err:           retries.Halt(errors.New("timeout waiting for deployment")),
+			appName:       "test-app",
+			expectWrapped: true,
+			description:   "timeout errors should be wrapped with logs hint",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cmd := &cobra.Command{}
+			result := wrapDeploymentError(cmd, tt.appName, tt.err)
+
+			if tt.expectWrapped {
+				var appErr *AppDeploymentError
+				require.ErrorAs(t, result, &appErr, tt.description)
+				assert.Equal(t, tt.appName, appErr.appName)
+				assert.ErrorIs(t, result, tt.err, "wrapped error should unwrap to original")
+				assert.Contains(t, result.Error(), "To view app logs, run:", "should contain logs hint")
+			} else {
+				assert.Equal(t, tt.err, result, tt.description)
+				if tt.err != nil {
+					assert.NotContains(t, result.Error(), "To view app logs", "should not contain logs hint")
+				}
+			}
+		})
+	}
+}

cmd/workspace/apps/errors_test.go

@@ -22,10 +22,46 @@ func listDeploymentsOverride(listDeploymentsCmd *cobra.Command, listDeploymentsR
 	{{end}}`)
 }
 
+func createOverride(createCmd *cobra.Command, createReq *apps.CreateAppRequest) {
+	originalRunE := createCmd.RunE
+	createCmd.RunE = func(cmd *cobra.Command, args []string) error {
+		err := originalRunE(cmd, args)
+		return wrapDeploymentError(cmd, createReq.App.Name, err)
+	}
+}
+
+func deployOverride(deployCmd *cobra.Command, deployReq *apps.CreateAppDeploymentRequest) {
+	originalRunE := deployCmd.RunE
+	deployCmd.RunE = func(cmd *cobra.Command, args []string) error {
+		err := originalRunE(cmd, args)
+		return wrapDeploymentError(cmd, deployReq.AppName, err)
+	}
+}
+
+func createUpdateOverride(createUpdateCmd *cobra.Command, createUpdateReq *apps.AsyncUpdateAppRequest) {
+	originalRunE := createUpdateCmd.RunE
+	createUpdateCmd.RunE = func(cmd *cobra.Command, args []string) error {
+		err := originalRunE(cmd, args)
+		return wrapDeploymentError(cmd, createUpdateReq.AppName, err)
+	}
+}
+
+func startOverride(startCmd *cobra.Command, startReq *apps.StartAppRequest) {
+	originalRunE := startCmd.RunE
+	startCmd.RunE = func(cmd *cobra.Command, args []string) error {
+		err := originalRunE(cmd, args)
+		return wrapDeploymentError(cmd, startReq.Name, err)
+	}
+}
+
 func init() {
 	cmdOverrides = append(cmdOverrides, func(cmd *cobra.Command) {
 		cmd.AddCommand(newLogsCommand())
 	})
 	listOverrides = append(listOverrides, listOverride)
 	listDeploymentsOverrides = append(listDeploymentsOverrides, listDeploymentsOverride)
+	createOverrides = append(createOverrides, createOverride)
+	deployOverrides = append(deployOverrides, deployOverride)
+	createUpdateOverrides = append(createUpdateOverrides, createUpdateOverride)
+	startOverrides = append(startOverrides, startOverride)
 }