From d63d3500fb5dc59cad361f3120dc0219a1a7feea Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Wed, 26 Jun 2024 15:39:06 +0200 Subject: [PATCH 01/10] Improve `bundle validate` output --- cmd/bundle/render/render_text_output.go | 168 +++++++++++++++++++ cmd/bundle/render/render_text_output_test.go | 159 ++++++++++++++++++ cmd/bundle/utils/utils.go | 7 +- cmd/bundle/validate.go | 135 +++------------ cmd/root/bundle.go | 6 +- cmd/root/root.go | 2 +- cmd/root/silent_err.go | 7 + 7 files changed, 362 insertions(+), 122 deletions(-) create mode 100644 cmd/bundle/render/render_text_output.go create mode 100644 cmd/bundle/render/render_text_output_test.go create mode 100644 cmd/root/silent_err.go diff --git a/cmd/bundle/render/render_text_output.go b/cmd/bundle/render/render_text_output.go new file mode 100644 index 0000000000..c09e082097 --- /dev/null +++ b/cmd/bundle/render/render_text_output.go @@ -0,0 +1,168 @@ +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 }} +` + +const summaryTemplate = `{{- if .Name }} +Name: {{ .Name | bold }} +{{- end }} +{{- 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 }} + +{{ .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 { + 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. +// +// It prints errors and returns root.AlreadyPrintedErr if there are any errors. +// If there are unexpected errors during rendering, it returns an error different from root.AlreadyPrintedErr. +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/cmd/bundle/render/render_text_output_test.go b/cmd/bundle/render/render_text_output_test.go new file mode 100644 index 0000000000..9553953ebf --- /dev/null +++ b/cmd/bundle/render/render_text_output_test.go @@ -0,0 +1,159 @@ +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: "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 error\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: "\n" + + "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 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" + + "'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" + + "'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()) + }) + } +} 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..4843e4e65c 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/cmd/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 { + panic("failed to load bundle") + } } - 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.AlreadyPrintedErr + } + + 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..7da03edcda 100644 --- a/cmd/root/root.go +++ b/cmd/root/root.go @@ -97,7 +97,7 @@ func Execute(cmd *cobra.Command) { // Run the command cmd, err := cmd.ExecuteContextC(ctx) - if err != nil { + if err != nil && err != AlreadyPrintedErr { // 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..dd2fa44a9a --- /dev/null +++ b/cmd/root/silent_err.go @@ -0,0 +1,7 @@ +package root + +import "errors" + +// AlreadyPrintedErr 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 AlreadyPrintedErr = errors.New("AlreadyPrintedErr") From 112beac3cef4c75d5e88f11d5fe1af0af9c8cdc4 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Thu, 27 Jun 2024 13:24:26 +0200 Subject: [PATCH 02/10] Add more tests for whitespace --- cmd/bundle/render/render_text_output.go | 4 +- cmd/bundle/render/render_text_output_test.go | 61 ++++++++++++++++++-- 2 files changed, 60 insertions(+), 5 deletions(-) diff --git a/cmd/bundle/render/render_text_output.go b/cmd/bundle/render/render_text_output.go index c09e082097..2f58072cc7 100644 --- a/cmd/bundle/render/render_text_output.go +++ b/cmd/bundle/render/render_text_output.go @@ -39,6 +39,7 @@ const errorTemplate = `{{ "Error" | red }}: {{ .Summary }} {{ .Detail }} {{- end }} + ` const warningTemplate = `{{ "Warning" | yellow }}: {{ .Summary }} @@ -48,9 +49,10 @@ const warningTemplate = `{{ "Warning" | yellow }}: {{ .Summary }} {{- if .Location.File }} {{ "in " }}{{ .Location.String | cyan }} {{- end }} + ` -const summaryTemplate = `{{- if .Name }} +const summaryTemplate = `{{- if .Name -}} Name: {{ .Name | bold }} {{- end }} {{- if .Target }} diff --git a/cmd/bundle/render/render_text_output_test.go b/cmd/bundle/render/render_text_output_test.go index 9553953ebf..c77e64b908 100644 --- a/cmd/bundle/render/render_text_output_test.go +++ b/cmd/bundle/render/render_text_output_test.go @@ -51,7 +51,60 @@ func TestRenderTextOutput(t *testing.T) { "Name: test-bundle\n" + "Target: test-target\n" + "\n" + - "Found 1 error\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 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, + }, + }, + }, + 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" + + "Name: test-bundle\n" + + "Target: test-target\n" + + "\n" + + "Found 2 errors\n", }, { name: "bundle during 'init'", @@ -73,8 +126,7 @@ func TestRenderTextOutput(t *testing.T) { }, }, diags: nil, - expected: "\n" + - "Name: test-bundle\n" + + expected: "Name: test-bundle\n" + "Target: test-target\n" + "Workspace:\n" + " Host: https://localhost/\n" + @@ -127,7 +179,7 @@ func TestRenderDiagnostics(t *testing.T) { }, }, expected: "Error: failed to load xxx\n" + - " in foo.yaml:1:2\n" + + " in foo.yaml:1:2\n\n" + "'name' is required\n\n", }, { @@ -142,6 +194,7 @@ func TestRenderDiagnostics(t *testing.T) { }, expected: "Error: failed to load xxx\n" + " at resources.jobs.xxx\n" + + "\n" + "'name' is required\n\n", }, } From 9de21bb5655b268857e21452f312e63e17b31436 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Thu, 27 Jun 2024 13:35:38 +0200 Subject: [PATCH 03/10] Fix more whitespace problems --- cmd/bundle/render/render_text_output.go | 7 ++++- cmd/bundle/render/render_text_output_test.go | 31 ++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/cmd/bundle/render/render_text_output.go b/cmd/bundle/render/render_text_output.go index 2f58072cc7..97a311ad38 100644 --- a/cmd/bundle/render/render_text_output.go +++ b/cmd/bundle/render/render_text_output.go @@ -54,7 +54,6 @@ const warningTemplate = `{{ "Warning" | yellow }}: {{ .Summary }} const summaryTemplate = `{{- if .Name -}} Name: {{ .Name | bold }} -{{- end }} {{- if .Target }} Target: {{ .Target | bold }} {{- end }} @@ -71,6 +70,8 @@ Workspace: {{- end }} {{- end }} +{{ end -}} + {{ .Trailer }} ` @@ -97,6 +98,10 @@ func buildTrailer(diags diag.Diagnostics) string { } 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 { diff --git a/cmd/bundle/render/render_text_output_test.go b/cmd/bundle/render/render_text_output_test.go index c77e64b908..e39a95b461 100644 --- a/cmd/bundle/render/render_text_output_test.go +++ b/cmd/bundle/render/render_text_output_test.go @@ -31,6 +31,18 @@ func TestRenderTextOutput(t *testing.T) { } 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, @@ -164,6 +176,16 @@ func TestRenderDiagnostics(t *testing.T) { 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{ @@ -210,3 +232,12 @@ func TestRenderDiagnostics(t *testing.T) { }) } } + +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()) +} From e0a74d6171381a93232e5881e36ecb02450bc57f Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Thu, 27 Jun 2024 13:36:26 +0200 Subject: [PATCH 04/10] Move cmd/bundle/render -> bundle/render --- {cmd/bundle => bundle}/render/render_text_output.go | 0 {cmd/bundle => bundle}/render/render_text_output_test.go | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename {cmd/bundle => bundle}/render/render_text_output.go (100%) rename {cmd/bundle => bundle}/render/render_text_output_test.go (100%) diff --git a/cmd/bundle/render/render_text_output.go b/bundle/render/render_text_output.go similarity index 100% rename from cmd/bundle/render/render_text_output.go rename to bundle/render/render_text_output.go diff --git a/cmd/bundle/render/render_text_output_test.go b/bundle/render/render_text_output_test.go similarity index 100% rename from cmd/bundle/render/render_text_output_test.go rename to bundle/render/render_text_output_test.go From a461d9378b313198e498517b27e29265d7a50f9d Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Thu, 27 Jun 2024 13:40:29 +0200 Subject: [PATCH 05/10] Fix build --- cmd/bundle/validate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/bundle/validate.go b/cmd/bundle/validate.go index 4843e4e65c..501514e85f 100644 --- a/cmd/bundle/validate.go +++ b/cmd/bundle/validate.go @@ -7,7 +7,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config/validate" "github.com/databricks/cli/bundle/phases" - "github.com/databricks/cli/cmd/bundle/render" + "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" From 7b2fb10038edcca37b2a9ee2ac0717c18bb078c1 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Thu, 27 Jun 2024 13:43:07 +0200 Subject: [PATCH 06/10] Fix lint --- cmd/bundle/validate.go | 2 +- cmd/root/root.go | 2 +- cmd/root/silent_err.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/bundle/validate.go b/cmd/bundle/validate.go index 501514e85f..4d272257e9 100644 --- a/cmd/bundle/validate.go +++ b/cmd/bundle/validate.go @@ -59,7 +59,7 @@ func newValidateCommand() *cobra.Command { } if diags.HasError() { - return root.AlreadyPrintedErr + return root.ErrAlreadyPrinted } return nil diff --git a/cmd/root/root.go b/cmd/root/root.go index 7da03edcda..cc15a27cfe 100644 --- a/cmd/root/root.go +++ b/cmd/root/root.go @@ -97,7 +97,7 @@ func Execute(cmd *cobra.Command) { // Run the command cmd, err := cmd.ExecuteContextC(ctx) - if err != nil && err != AlreadyPrintedErr { + if err != nil && 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 index dd2fa44a9a..b361cc6b40 100644 --- a/cmd/root/silent_err.go +++ b/cmd/root/silent_err.go @@ -2,6 +2,6 @@ package root import "errors" -// AlreadyPrintedErr is not printed to the user. It's used to signal that the command should exit with an error, +// 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 AlreadyPrintedErr = errors.New("AlreadyPrintedErr") +var ErrAlreadyPrinted = errors.New("AlreadyPrinted") From 7e964f7f09344569ffcf44e70413a58add0e9c6a Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Thu, 27 Jun 2024 15:16:12 +0200 Subject: [PATCH 07/10] Fix tests --- bundle/tests/suggest_target_test.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/bundle/tests/suggest_target_test.go b/bundle/tests/suggest_target_test.go index 924d6a4e14..2667e9dafb 100644 --- a/bundle/tests/suggest_target_test.go +++ b/bundle/tests/suggest_target_test.go @@ -1,17 +1,21 @@ package config_tests import ( + "github.com/databricks/cli/cmd/root" + assert "github.com/databricks/cli/libs/dyn/dynassert" "path/filepath" "testing" "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") } From 04caf10aefe30d5686bd28371ccd7fd8e077070e Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Thu, 27 Jun 2024 15:17:36 +0200 Subject: [PATCH 08/10] Fix fmt --- bundle/tests/suggest_target_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bundle/tests/suggest_target_test.go b/bundle/tests/suggest_target_test.go index 2667e9dafb..8fb1304090 100644 --- a/bundle/tests/suggest_target_test.go +++ b/bundle/tests/suggest_target_test.go @@ -1,11 +1,12 @@ package config_tests import ( - "github.com/databricks/cli/cmd/root" - assert "github.com/databricks/cli/libs/dyn/dynassert" "path/filepath" "testing" + "github.com/databricks/cli/cmd/root" + assert "github.com/databricks/cli/libs/dyn/dynassert" + "github.com/databricks/cli/internal" ) From 6282a4e2281ce36968b522add20e76ab758b3cbb Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Mon, 1 Jul 2024 10:41:34 +0200 Subject: [PATCH 09/10] Address CR comments --- bundle/render/render_text_output.go | 7 ++++--- bundle/render/render_text_output_test.go | 19 +++++++++++++++++-- cmd/root/root.go | 3 ++- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/bundle/render/render_text_output.go b/bundle/render/render_text_output.go index 97a311ad38..37ea188f7c 100644 --- a/bundle/render/render_text_output.go +++ b/bundle/render/render_text_output.go @@ -49,6 +49,10 @@ const warningTemplate = `{{ "Warning" | yellow }}: {{ .Summary }} {{- if .Location.File }} {{ "in " }}{{ .Location.String | cyan }} {{- end }} +{{- if .Detail }} + +{{ .Detail }} +{{- end }} ` @@ -157,9 +161,6 @@ func renderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) } // RenderTextOutput renders the diagnostics in a human-readable format. -// -// It prints errors and returns root.AlreadyPrintedErr if there are any errors. -// If there are unexpected errors during rendering, it returns an error different from root.AlreadyPrintedErr. func RenderTextOutput(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) error { err := renderDiagnostics(out, b, diags) if err != nil { diff --git a/bundle/render/render_text_output_test.go b/bundle/render/render_text_output_test.go index e39a95b461..4ae86ded70 100644 --- a/bundle/render/render_text_output_test.go +++ b/bundle/render/render_text_output_test.go @@ -79,7 +79,7 @@ func TestRenderTextOutput(t *testing.T) { "Found 2 warnings\n", }, { - name: "bundle during 'load' and 2 errors with details", + name: "bundle during 'load' and 2 errors, 1 warning with details", bundle: loadingBundle, diags: diag.Diagnostics{ diag.Diagnostic{ @@ -102,6 +102,16 @@ func TestRenderTextOutput(t *testing.T) { 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" + @@ -113,10 +123,15 @@ func TestRenderTextOutput(t *testing.T) { "\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\n", + "Found 2 errors and 1 warning\n", }, { name: "bundle during 'init'", diff --git a/cmd/root/root.go b/cmd/root/root.go index cc15a27cfe..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 && err != ErrAlreadyPrinted { + 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) From 76b76a1b5d503e0cfa7c8b739bebdc50b1630d50 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Mon, 1 Jul 2024 10:53:29 +0200 Subject: [PATCH 10/10] Don't panic --- cmd/bundle/validate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/bundle/validate.go b/cmd/bundle/validate.go index 4d272257e9..59a9770476 100644 --- a/cmd/bundle/validate.go +++ b/cmd/bundle/validate.go @@ -39,7 +39,7 @@ func newValidateCommand() *cobra.Command { if err := diags.Error(); err != nil { return diags.Error() } else { - panic("failed to load bundle") + return fmt.Errorf("invariant failed: returned bundle is nil") } }