Improve `bundle validate` output (#1532)

## Changes
This combination of changes allows pretty-printing errors happening
during the "load" and "init" phases, including their locations.

Move to render code into a separate module dedicated to rendering
`diag.Diagnostics` in a human-readable format. This will be used for the
`bundle deploy` command.

Preserve the "bundle" value if an error occurs in mutators. Rewrite the
go templates to handle the case when the bundle isn't yet loaded if an
error occurs during loading, that is possible now.

Improve rendering for errors and warnings:
- don't render empty locations
- render "details" for errors if they exist

Add `root.ErrAlreadyPrinted` indicating that the error was already
printed, and the CLI entry point shouldn't print it again.

## Tests
Add tests for output, that are especially handy to detect extra newlines

bundle/render/render_text_output.go

@@ -0,0 +1,176 @@
+package render
+
+import (
+	"fmt"
+	"io"
+	"path/filepath"
+	"strings"
+	"text/template"
+
+	"github.com/databricks/cli/bundle"
+	"github.com/databricks/cli/libs/diag"
+	"github.com/databricks/databricks-sdk-go/service/iam"
+	"github.com/fatih/color"
+)
+
+var renderFuncMap = template.FuncMap{
+	"red":     color.RedString,
+	"green":   color.GreenString,
+	"blue":    color.BlueString,
+	"yellow":  color.YellowString,
+	"magenta": color.MagentaString,
+	"cyan":    color.CyanString,
+	"bold": func(format string, a ...interface{}) string {
+		return color.New(color.Bold).Sprintf(format, a...)
+	},
+	"italic": func(format string, a ...interface{}) string {
+		return color.New(color.Italic).Sprintf(format, a...)
+	},
+}
+
+const errorTemplate = `{{ "Error" | red }}: {{ .Summary }}
+{{- if .Path.String }}
+  {{ "at " }}{{ .Path.String | green }}
+{{- end }}
+{{- if .Location.File }}
+  {{ "in " }}{{ .Location.String | cyan }}
+{{- end }}
+{{- if .Detail }}
+
+{{ .Detail }}
+{{- end }}
+
+`
+
+const warningTemplate = `{{ "Warning" | yellow }}: {{ .Summary }}
+{{- if .Path.String }}
+  {{ "at " }}{{ .Path.String | green }}
+{{- end }}
+{{- if .Location.File }}
+  {{ "in " }}{{ .Location.String | cyan }}
+{{- end }}
+{{- if .Detail }}
+
+{{ .Detail }}
+{{- end }}
+
+`
+
+const summaryTemplate = `{{- if .Name -}}
+Name: {{ .Name | bold }}
+{{- if .Target }}
+Target: {{ .Target | bold }}
+{{- end }}
+{{- if or .User .Host .Path }}
+Workspace:
+{{- if .Host }}
+  Host: {{ .Host | bold }}
+{{- end }}
+{{- if .User }}
+  User: {{ .User | bold }}
+{{- end }}
+{{- if .Path }}
+  Path: {{ .Path | bold }}
+{{- end }}
+{{- end }}
+
+{{ end -}}
+
+{{ .Trailer }}
+`
+
+func pluralize(n int, singular, plural string) string {
+	if n == 1 {
+		return fmt.Sprintf("%d %s", n, singular)
+	}
+	return fmt.Sprintf("%d %s", n, plural)
+}
+
+func buildTrailer(diags diag.Diagnostics) string {
+	parts := []string{}
+	if errors := len(diags.Filter(diag.Error)); errors > 0 {
+		parts = append(parts, color.RedString(pluralize(errors, "error", "errors")))
+	}
+	if warnings := len(diags.Filter(diag.Warning)); warnings > 0 {
+		parts = append(parts, color.YellowString(pluralize(warnings, "warning", "warnings")))
+	}
+	if len(parts) > 0 {
+		return fmt.Sprintf("Found %s", strings.Join(parts, " and "))
+	} else {
+		return color.GreenString("Validation OK!")
+	}
+}
+
+func renderSummaryTemplate(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) error {
+	if b == nil {
+		return renderSummaryTemplate(out, &bundle.Bundle{}, diags)
+	}
+
+	var currentUser = &iam.User{}
+
+	if b.Config.Workspace.CurrentUser != nil {
+		if b.Config.Workspace.CurrentUser.User != nil {
+			currentUser = b.Config.Workspace.CurrentUser.User
+		}
+	}
+
+	t := template.Must(template.New("summary").Funcs(renderFuncMap).Parse(summaryTemplate))
+	err := t.Execute(out, map[string]any{
+		"Name":    b.Config.Bundle.Name,
+		"Target":  b.Config.Bundle.Target,
+		"User":    currentUser.UserName,
+		"Path":    b.Config.Workspace.RootPath,
+		"Host":    b.Config.Workspace.Host,
+		"Trailer": buildTrailer(diags),
+	})
+
+	return err
+}
+
+func renderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) error {
+	errorT := template.Must(template.New("error").Funcs(renderFuncMap).Parse(errorTemplate))
+	warningT := template.Must(template.New("warning").Funcs(renderFuncMap).Parse(warningTemplate))
+
+	// Print errors and warnings.
+	for _, d := range diags {
+		var t *template.Template
+		switch d.Severity {
+		case diag.Error:
+			t = errorT
+		case diag.Warning:
+			t = warningT
+		}
+
+		// Make file relative to bundle root
+		if d.Location.File != "" {
+			out, err := filepath.Rel(b.RootPath, d.Location.File)
+			// if we can't relativize the path, just use path as-is
+			if err == nil {
+				d.Location.File = out
+			}
+		}
+
+		// Render the diagnostic with the appropriate template.
+		err := t.Execute(out, d)
+		if err != nil {
+			return fmt.Errorf("failed to render template: %w", err)
+		}
+	}
+
+	return nil
+}
+
+// RenderTextOutput renders the diagnostics in a human-readable format.
+func RenderTextOutput(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) error {
+	err := renderDiagnostics(out, b, diags)
+	if err != nil {
+		return fmt.Errorf("failed to render diagnostics: %w", err)
+	}
+
+	err = renderSummaryTemplate(out, b, diags)
+	if err != nil {
+		return fmt.Errorf("failed to render summary: %w", err)
+	}
+
+	return nil
+}

bundle/render/render_text_output_test.go

@@ -0,0 +1,258 @@
+package render
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/databricks/cli/bundle"
+	"github.com/databricks/cli/bundle/config"
+	"github.com/databricks/cli/libs/diag"
+	"github.com/databricks/cli/libs/dyn"
+	assert "github.com/databricks/cli/libs/dyn/dynassert"
+	"github.com/databricks/databricks-sdk-go/service/iam"
+	"github.com/stretchr/testify/require"
+)
+
+type renderTestOutputTestCase struct {
+	name     string
+	bundle   *bundle.Bundle
+	diags    diag.Diagnostics
+	expected string
+}
+
+func TestRenderTextOutput(t *testing.T) {
+	loadingBundle := &bundle.Bundle{
+		Config: config.Root{
+			Bundle: config.Bundle{
+				Name:   "test-bundle",
+				Target: "test-target",
+			},
+		},
+	}
+
+	testCases := []renderTestOutputTestCase{
+		{
+			name: "nil bundle and 1 error",
+			diags: diag.Diagnostics{
+				{
+					Severity: diag.Error,
+					Summary:  "failed to load xxx",
+				},
+			},
+			expected: "Error: failed to load xxx\n" +
+				"\n" +
+				"Found 1 error\n",
+		},
+		{
+			name:   "bundle during 'load' and 1 error",
+			bundle: loadingBundle,
+			diags:  diag.Errorf("failed to load bundle"),
+			expected: "Error: failed to load bundle\n" +
+				"\n" +
+				"Name: test-bundle\n" +
+				"Target: test-target\n" +
+				"\n" +
+				"Found 1 error\n",
+		},
+		{
+			name:   "bundle during 'load' and 1 warning",
+			bundle: loadingBundle,
+			diags:  diag.Warningf("failed to load bundle"),
+			expected: "Warning: failed to load bundle\n" +
+				"\n" +
+				"Name: test-bundle\n" +
+				"Target: test-target\n" +
+				"\n" +
+				"Found 1 warning\n",
+		},
+		{
+			name:   "bundle during 'load' and 2 warnings",
+			bundle: loadingBundle,
+			diags:  diag.Warningf("warning (1)").Extend(diag.Warningf("warning (2)")),
+			expected: "Warning: warning (1)\n" +
+				"\n" +
+				"Warning: warning (2)\n" +
+				"\n" +
+				"Name: test-bundle\n" +
+				"Target: test-target\n" +
+				"\n" +
+				"Found 2 warnings\n",
+		},
+		{
+			name:   "bundle during 'load' and 2 errors, 1 warning with details",
+			bundle: loadingBundle,
+			diags: diag.Diagnostics{
+				diag.Diagnostic{
+					Severity: diag.Error,
+					Summary:  "error (1)",
+					Detail:   "detail (1)",
+					Location: dyn.Location{
+						File:   "foo.py",
+						Line:   1,
+						Column: 1,
+					},
+				},
+				diag.Diagnostic{
+					Severity: diag.Error,
+					Summary:  "error (2)",
+					Detail:   "detail (2)",
+					Location: dyn.Location{
+						File:   "foo.py",
+						Line:   2,
+						Column: 1,
+					},
+				},
+				diag.Diagnostic{
+					Severity: diag.Warning,
+					Summary:  "warning (3)",
+					Detail:   "detail (3)",
+					Location: dyn.Location{
+						File:   "foo.py",
+						Line:   3,
+						Column: 1,
+					},
+				},
+			},
+			expected: "Error: error (1)\n" +
+				"  in foo.py:1:1\n" +
+				"\n" +
+				"detail (1)\n" +
+				"\n" +
+				"Error: error (2)\n" +
+				"  in foo.py:2:1\n" +
+				"\n" +
+				"detail (2)\n" +
+				"\n" +
+				"Warning: warning (3)\n" +
+				"  in foo.py:3:1\n" +
+				"\n" +
+				"detail (3)\n" +
+				"\n" +
+				"Name: test-bundle\n" +
+				"Target: test-target\n" +
+				"\n" +
+				"Found 2 errors and 1 warning\n",
+		},
+		{
+			name: "bundle during 'init'",
+			bundle: &bundle.Bundle{
+				Config: config.Root{
+					Bundle: config.Bundle{
+						Name:   "test-bundle",
+						Target: "test-target",
+					},
+					Workspace: config.Workspace{
+						Host: "https://localhost/",
+						CurrentUser: &config.User{
+							User: &iam.User{
+								UserName: "test-user",
+							},
+						},
+						RootPath: "/Users/test-user@databricks.com/.bundle/examples/test-target",
+					},
+				},
+			},
+			diags: nil,
+			expected: "Name: test-bundle\n" +
+				"Target: test-target\n" +
+				"Workspace:\n" +
+				"  Host: https://localhost/\n" +
+				"  User: test-user\n" +
+				"  Path: /Users/test-user@databricks.com/.bundle/examples/test-target\n" +
+				"\n" +
+				"Validation OK!\n",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			writer := &bytes.Buffer{}
+
+			err := RenderTextOutput(writer, tc.bundle, tc.diags)
+			require.NoError(t, err)
+
+			assert.Equal(t, tc.expected, writer.String())
+		})
+	}
+}
+
+type renderDiagnosticsTestCase struct {
+	name     string
+	diags    diag.Diagnostics
+	expected string
+}
+
+func TestRenderDiagnostics(t *testing.T) {
+	bundle := &bundle.Bundle{}
+
+	testCases := []renderDiagnosticsTestCase{
+		{
+			name:     "empty diagnostics",
+			diags:    diag.Diagnostics{},
+			expected: "",
+		},
+		{
+			name: "error with short summary",
+			diags: diag.Diagnostics{
+				{
+					Severity: diag.Error,
+					Summary:  "failed to load xxx",
+				},
+			},
+			expected: "Error: failed to load xxx\n\n",
+		},
+		{
+			name: "error with source location",
+			diags: diag.Diagnostics{
+				{
+					Severity: diag.Error,
+					Summary:  "failed to load xxx",
+					Detail:   "'name' is required",
+					Location: dyn.Location{
+						File:   "foo.yaml",
+						Line:   1,
+						Column: 2,
+					},
+				},
+			},
+			expected: "Error: failed to load xxx\n" +
+				"  in foo.yaml:1:2\n\n" +
+				"'name' is required\n\n",
+		},
+		{
+			name: "error with path",
+			diags: diag.Diagnostics{
+				{
+					Severity: diag.Error,
+					Detail:   "'name' is required",
+					Summary:  "failed to load xxx",
+					Path:     dyn.MustPathFromString("resources.jobs.xxx"),
+				},
+			},
+			expected: "Error: failed to load xxx\n" +
+				"  at resources.jobs.xxx\n" +
+				"\n" +
+				"'name' is required\n\n",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			writer := &bytes.Buffer{}
+
+			err := renderDiagnostics(writer, bundle, tc.diags)
+			require.NoError(t, err)
+
+			assert.Equal(t, tc.expected, writer.String())
+		})
+	}
+}
+
+func TestRenderSummaryTemplate_nilBundle(t *testing.T) {
+	writer := &bytes.Buffer{}
+
+	err := renderSummaryTemplate(writer, nil, nil)
+	require.NoError(t, err)
+
+	assert.Equal(t, "Validation OK!\n", writer.String())
+}

bundle/tests/suggest_target_test.go

@@ -4,14 +4,19 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/databricks/cli/cmd/root"
+	assert "github.com/databricks/cli/libs/dyn/dynassert"
+
 	"github.com/databricks/cli/internal"
-	"github.com/stretchr/testify/require"
 )
 
 func TestSuggestTargetIfWrongPassed(t *testing.T) {
 	t.Setenv("BUNDLE_ROOT", filepath.Join("target_overrides", "workspace"))
-	_, _, err := internal.RequireErrorRun(t, "bundle", "validate", "-e", "incorrect")
+	stdoutBytes, _, err := internal.RequireErrorRun(t, "bundle", "validate", "-e", "incorrect")
-	require.ErrorContains(t, err, "Available targets:")
+	stdout := stdoutBytes.String()
-	require.ErrorContains(t, err, "development")
+
-	require.ErrorContains(t, err, "staging")
+	assert.Error(t, root.ErrAlreadyPrinted, err)
+	assert.Contains(t, stdout, "Available targets:")
+	assert.Contains(t, stdout, "development")
+	assert.Contains(t, stdout, "staging")
 }

cmd/bundle/utils/utils.go

@@ -20,19 +20,16 @@ func ConfigureBundleWithVariables(cmd *cobra.Command) (*bundle.Bundle, diag.Diag
 	// Load bundle config and apply target
 	b, diags := root.MustConfigureBundle(cmd)
 	if diags.HasError() {
-		return nil, diags
+		return b, diags
 	}
 
 	variables, err := cmd.Flags().GetStringSlice("var")
 	if err != nil {
-		return nil, diag.FromErr(err)
+		return b, diag.FromErr(err)
 	}
 
 	// Initialize variables by assigning them values passed as command line flags
 	diags = diags.Extend(configureVariables(cmd, b, variables))
-	if diags.HasError() {
-		return nil, diags
-	}
 
 	return b, diags
 }

cmd/bundle/validate.go

@@ -3,121 +3,18 @@ package bundle
 import (
 	"encoding/json"
 	"fmt"
-	"path/filepath"
-	"strings"
-	"text/template"
 
 	"github.com/databricks/cli/bundle"
 	"github.com/databricks/cli/bundle/config/validate"
 	"github.com/databricks/cli/bundle/phases"
+	"github.com/databricks/cli/bundle/render"
 	"github.com/databricks/cli/cmd/bundle/utils"
 	"github.com/databricks/cli/cmd/root"
 	"github.com/databricks/cli/libs/diag"
 	"github.com/databricks/cli/libs/flags"
-	"github.com/fatih/color"
 	"github.com/spf13/cobra"
 )
 
-var validateFuncMap = template.FuncMap{
-	"red":     color.RedString,
-	"green":   color.GreenString,
-	"blue":    color.BlueString,
-	"yellow":  color.YellowString,
-	"magenta": color.MagentaString,
-	"cyan":    color.CyanString,
-	"bold": func(format string, a ...interface{}) string {
-		return color.New(color.Bold).Sprintf(format, a...)
-	},
-	"italic": func(format string, a ...interface{}) string {
-		return color.New(color.Italic).Sprintf(format, a...)
-	},
-}
-
-const errorTemplate = `{{ "Error" | red }}: {{ .Summary }}
-  {{ "at " }}{{ .Path.String | green }}
-  {{ "in " }}{{ .Location.String | cyan }}
-
-`
-
-const warningTemplate = `{{ "Warning" | yellow }}: {{ .Summary }}
-  {{ "at " }}{{ .Path.String | green }}
-  {{ "in " }}{{ .Location.String | cyan }}
-
-`
-
-const summaryTemplate = `Name: {{ .Config.Bundle.Name | bold }}
-Target: {{ .Config.Bundle.Target | bold }}
-Workspace:
-  Host: {{ .WorkspaceClient.Config.Host | bold }}
-  User: {{ .Config.Workspace.CurrentUser.UserName | bold }}
-  Path: {{ .Config.Workspace.RootPath | bold }}
-
-{{ .Trailer }}
-`
-
-func pluralize(n int, singular, plural string) string {
-	if n == 1 {
-		return fmt.Sprintf("%d %s", n, singular)
-	}
-	return fmt.Sprintf("%d %s", n, plural)
-}
-
-func buildTrailer(diags diag.Diagnostics) string {
-	parts := []string{}
-	if errors := len(diags.Filter(diag.Error)); errors > 0 {
-		parts = append(parts, color.RedString(pluralize(errors, "error", "errors")))
-	}
-	if warnings := len(diags.Filter(diag.Warning)); warnings > 0 {
-		parts = append(parts, color.YellowString(pluralize(warnings, "warning", "warnings")))
-	}
-	if len(parts) > 0 {
-		return fmt.Sprintf("Found %s", strings.Join(parts, " and "))
-	} else {
-		return color.GreenString("Validation OK!")
-	}
-}
-
-func renderTextOutput(cmd *cobra.Command, b *bundle.Bundle, diags diag.Diagnostics) error {
-	errorT := template.Must(template.New("error").Funcs(validateFuncMap).Parse(errorTemplate))
-	warningT := template.Must(template.New("warning").Funcs(validateFuncMap).Parse(warningTemplate))
-
-	// Print errors and warnings.
-	for _, d := range diags {
-		var t *template.Template
-		switch d.Severity {
-		case diag.Error:
-			t = errorT
-		case diag.Warning:
-			t = warningT
-		}
-
-		// Make file relative to bundle root
-		if d.Location.File != "" {
-			out, _ := filepath.Rel(b.RootPath, d.Location.File)
-			d.Location.File = out
-		}
-
-		// Render the diagnostic with the appropriate template.
-		err := t.Execute(cmd.OutOrStdout(), d)
-		if err != nil {
-			return err
-		}
-	}
-
-	// Print validation summary.
-	t := template.Must(template.New("summary").Funcs(validateFuncMap).Parse(summaryTemplate))
-	err := t.Execute(cmd.OutOrStdout(), map[string]any{
-		"Config":          b.Config,
-		"Trailer":         buildTrailer(diags),
-		"WorkspaceClient": b.WorkspaceClient(),
-	})
-	if err != nil {
-		return err
-	}
-
-	return diags.Error()
-}
-
 func renderJsonOutput(cmd *cobra.Command, b *bundle.Bundle, diags diag.Diagnostics) error {
 	buf, err := json.MarshalIndent(b.Config.Value().AsAny(), "", "  ")
 	if err != nil {
@@ -137,19 +34,35 @@ func newValidateCommand() *cobra.Command {
 	cmd.RunE = func(cmd *cobra.Command, args []string) error {
 		ctx := cmd.Context()
 		b, diags := utils.ConfigureBundleWithVariables(cmd)
-		if err := diags.Error(); err != nil {
+
-			return diags.Error()
+		if b == nil {
+			if err := diags.Error(); err != nil {
+				return diags.Error()
+			} else {
+				return fmt.Errorf("invariant failed: returned bundle is nil")
+			}
 		}
 
-		diags = diags.Extend(bundle.Apply(ctx, b, phases.Initialize()))
+		if !diags.HasError() {
-		diags = diags.Extend(bundle.Apply(ctx, b, validate.Validate()))
+			diags = diags.Extend(bundle.Apply(ctx, b, phases.Initialize()))
-		if err := diags.Error(); err != nil {
+		}
-			return err
+
+		if !diags.HasError() {
+			diags = diags.Extend(bundle.Apply(ctx, b, validate.Validate()))
 		}
 
 		switch root.OutputType(cmd) {
 		case flags.OutputText:
-			return renderTextOutput(cmd, b, diags)
+			err := render.RenderTextOutput(cmd.OutOrStdout(), b, diags)
+			if err != nil {
+				return fmt.Errorf("failed to render output: %w", err)
+			}
+
+			if diags.HasError() {
+				return root.ErrAlreadyPrinted
+			}
+
+			return nil
 		case flags.OutputJSON:
 			return renderJsonOutput(cmd, b, diags)
 		default:

cmd/root/bundle.go

@@ -76,15 +76,11 @@ func configureBundle(cmd *cobra.Command, b *bundle.Bundle) (*bundle.Bundle, diag
 	ctx := cmd.Context()
 	diags := bundle.Apply(ctx, b, m)
 	if diags.HasError() {
-		return nil, diags
+		return b, diags
 	}
 
 	// Configure the workspace profile if the flag has been set.
 	diags = diags.Extend(configureProfile(cmd, b))
-	if diags.HasError() {
-		return nil, diags
-	}
-
 	return b, diags
 }
 

cmd/root/root.go

@@ -2,6 +2,7 @@ package root
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"strings"
@@ -97,7 +98,7 @@ func Execute(cmd *cobra.Command) {
 
 	// Run the command
 	cmd, err := cmd.ExecuteContextC(ctx)
-	if err != nil {
+	if err != nil && errors.Is(err, ErrAlreadyPrinted) {
 		// If cmdio logger initialization succeeds, then this function logs with the
 		// initialized cmdio logger, otherwise with the default cmdio logger
 		cmdio.LogError(cmd.Context(), err)

cmd/root/silent_err.go

@@ -0,0 +1,7 @@
+package root
+
+import "errors"
+
+// ErrAlreadyPrinted is not printed to the user. It's used to signal that the command should exit with an error,
+// but the error message was already printed.
+var ErrAlreadyPrinted = errors.New("AlreadyPrinted")