diff --git a/bundle/render/render_text_output.go b/bundle/render/render_text_output.go new file mode 100644 index 0000000000..37ea188f7c --- /dev/null +++ b/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 +} diff --git a/bundle/render/render_text_output_test.go b/bundle/render/render_text_output_test.go new file mode 100644 index 0000000000..4ae86ded70 --- /dev/null +++ b/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()) +} diff --git a/bundle/tests/suggest_target_test.go b/bundle/tests/suggest_target_test.go index 924d6a4e14..8fb1304090 100644 --- a/bundle/tests/suggest_target_test.go +++ b/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") - require.ErrorContains(t, err, "Available targets:") - require.ErrorContains(t, err, "development") - require.ErrorContains(t, err, "staging") + stdoutBytes, _, err := internal.RequireErrorRun(t, "bundle", "validate", "-e", "incorrect") + stdout := stdoutBytes.String() + + assert.Error(t, root.ErrAlreadyPrinted, err) + assert.Contains(t, stdout, "Available targets:") + assert.Contains(t, stdout, "development") + assert.Contains(t, stdout, "staging") } diff --git a/cmd/bundle/utils/utils.go b/cmd/bundle/utils/utils.go index d585c62204..ce3774cf55 100644 --- a/cmd/bundle/utils/utils.go +++ b/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 } diff --git a/cmd/bundle/validate.go b/cmd/bundle/validate.go index a1f8d26819..59a9770476 100644 --- a/cmd/bundle/validate.go +++ b/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())) - diags = diags.Extend(bundle.Apply(ctx, b, validate.Validate())) - if err := diags.Error(); err != nil { - return err + if !diags.HasError() { + diags = diags.Extend(bundle.Apply(ctx, b, phases.Initialize())) + } + + 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: diff --git a/cmd/root/bundle.go b/cmd/root/bundle.go index 4ed89c57b8..8b98f2cf20 100644 --- a/cmd/root/bundle.go +++ b/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 } diff --git a/cmd/root/root.go b/cmd/root/root.go index 38eb42ccb0..91e91d368c 100644 --- a/cmd/root/root.go +++ b/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) diff --git a/cmd/root/silent_err.go b/cmd/root/silent_err.go new file mode 100644 index 0000000000..b361cc6b40 --- /dev/null +++ b/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")