From af975ca64ba16cbad58a657d39bc6063b4d71311 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Wed, 10 Jul 2024 13:14:57 +0200 Subject: [PATCH] Print diagnostics in 'bundle deploy' (#1579) ## Changes Print diagnostics in 'bundle deploy' similar to 'bundle validate'. This way if a bundle has any errors or warnings, they are going to be easy to notice. NB: due to how we render errors, there is one extra trailing new line in output, preserved in examples below ## Example: No errors or warnings ``` % databricks bundle deploy Building default... Deploying resources... Updating deployment state... Deployment complete! ``` ## Example: Error on load ``` % databricks bundle deploy Error: Databricks CLI version constraint not satisfied. Required: >= 1337.0.0, current: 0.0.0-dev ``` ## Example: Warning on load ``` % databricks bundle deploy Building default... Deploying resources... Updating deployment state... Deployment complete! Warning: unknown field: foo in databricks.yml:6:1 ``` ## Example: Error + warning on load ``` % databricks bundle deploy Warning: unknown field: foo in databricks.yml:6:1 Error: something went wrong ``` ## Example: Warning on load + error in init ``` % databricks bundle deploy Warning: unknown field: foo in databricks.yml:6:1 Error: Failed to xxx in yyy.yml Detailed explanation in multiple lines ``` ## Tests Tested manually --- bundle/render/render_text_output.go | 19 +++++--- bundle/render/render_text_output_test.go | 46 ++++++++++++++++++- bundle/scripts/scripts.go | 9 +++- cmd/bundle/deploy.go | 57 ++++++++++++++---------- cmd/bundle/validate.go | 3 +- 5 files changed, 102 insertions(+), 32 deletions(-) diff --git a/bundle/render/render_text_output.go b/bundle/render/render_text_output.go index 37ea188f7c..439ae61323 100644 --- a/bundle/render/render_text_output.go +++ b/bundle/render/render_text_output.go @@ -142,7 +142,7 @@ func renderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) } // Make file relative to bundle root - if d.Location.File != "" { + if d.Location.File != "" && b != nil { out, err := filepath.Rel(b.RootPath, d.Location.File) // if we can't relativize the path, just use path as-is if err == nil { @@ -160,16 +160,25 @@ func renderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) return nil } +// RenderOptions contains options for rendering diagnostics. +type RenderOptions struct { + // variable to include leading new line + + RenderSummaryTable bool +} + // RenderTextOutput renders the diagnostics in a human-readable format. -func RenderTextOutput(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) error { +func RenderTextOutput(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics, opts RenderOptions) 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) + if opts.RenderSummaryTable { + 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 index 4ae86ded70..b7aec88648 100644 --- a/bundle/render/render_text_output_test.go +++ b/bundle/render/render_text_output_test.go @@ -17,6 +17,7 @@ type renderTestOutputTestCase struct { name string bundle *bundle.Bundle diags diag.Diagnostics + opts RenderOptions expected string } @@ -39,6 +40,7 @@ func TestRenderTextOutput(t *testing.T) { Summary: "failed to load xxx", }, }, + opts: RenderOptions{RenderSummaryTable: true}, expected: "Error: failed to load xxx\n" + "\n" + "Found 1 error\n", @@ -47,6 +49,7 @@ func TestRenderTextOutput(t *testing.T) { name: "bundle during 'load' and 1 error", bundle: loadingBundle, diags: diag.Errorf("failed to load bundle"), + opts: RenderOptions{RenderSummaryTable: true}, expected: "Error: failed to load bundle\n" + "\n" + "Name: test-bundle\n" + @@ -58,6 +61,7 @@ func TestRenderTextOutput(t *testing.T) { name: "bundle during 'load' and 1 warning", bundle: loadingBundle, diags: diag.Warningf("failed to load bundle"), + opts: RenderOptions{RenderSummaryTable: true}, expected: "Warning: failed to load bundle\n" + "\n" + "Name: test-bundle\n" + @@ -69,6 +73,7 @@ func TestRenderTextOutput(t *testing.T) { name: "bundle during 'load' and 2 warnings", bundle: loadingBundle, diags: diag.Warningf("warning (1)").Extend(diag.Warningf("warning (2)")), + opts: RenderOptions{RenderSummaryTable: true}, expected: "Warning: warning (1)\n" + "\n" + "Warning: warning (2)\n" + @@ -113,6 +118,7 @@ func TestRenderTextOutput(t *testing.T) { }, }, }, + opts: RenderOptions{RenderSummaryTable: true}, expected: "Error: error (1)\n" + " in foo.py:1:1\n" + "\n" + @@ -153,6 +159,7 @@ func TestRenderTextOutput(t *testing.T) { }, }, diags: nil, + opts: RenderOptions{RenderSummaryTable: true}, expected: "Name: test-bundle\n" + "Target: test-target\n" + "Workspace:\n" + @@ -162,13 +169,50 @@ func TestRenderTextOutput(t *testing.T) { "\n" + "Validation OK!\n", }, + { + name: "nil bundle without summary with 1 error and 1 warning", + bundle: nil, + 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.Warning, + Summary: "warning (2)", + Detail: "detail (2)", + Location: dyn.Location{ + File: "foo.py", + Line: 3, + Column: 1, + }, + }, + }, + opts: RenderOptions{RenderSummaryTable: false}, + expected: "Error: error (1)\n" + + " in foo.py:1:1\n" + + "\n" + + "detail (1)\n" + + "\n" + + "Warning: warning (2)\n" + + " in foo.py:3:1\n" + + "\n" + + "detail (2)\n" + + "\n", + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { writer := &bytes.Buffer{} - err := RenderTextOutput(writer, tc.bundle, tc.diags) + err := RenderTextOutput(writer, tc.bundle, tc.diags, tc.opts) require.NoError(t, err) assert.Equal(t, tc.expected, writer.String()) diff --git a/bundle/scripts/scripts.go b/bundle/scripts/scripts.go index 38d204f99e..629b3a8ab7 100644 --- a/bundle/scripts/scripts.go +++ b/bundle/scripts/scripts.go @@ -37,7 +37,7 @@ func (m *script) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { cmd, out, err := executeHook(ctx, executor, b, m.scriptHook) if err != nil { - return diag.FromErr(err) + return diag.FromErr(fmt.Errorf("failed to execute script: %w", err)) } if cmd == nil { log.Debugf(ctx, "No script defined for %s, skipping", m.scriptHook) @@ -53,7 +53,12 @@ func (m *script) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { line, err = reader.ReadString('\n') } - return diag.FromErr(cmd.Wait()) + err = cmd.Wait() + if err != nil { + return diag.FromErr(fmt.Errorf("failed to execute script: %w", err)) + } + + return nil } func executeHook(ctx context.Context, executor *exec.Executor, b *bundle.Bundle, hook config.ScriptHook) (exec.Command, io.Reader, error) { diff --git a/cmd/bundle/deploy.go b/cmd/bundle/deploy.go index 919b15a723..1232c8de51 100644 --- a/cmd/bundle/deploy.go +++ b/cmd/bundle/deploy.go @@ -2,9 +2,11 @@ package bundle import ( "context" + "fmt" "github.com/databricks/cli/bundle" "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" @@ -30,32 +32,41 @@ func newDeployCommand() *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 !diags.HasError() { + bundle.ApplyFunc(ctx, b, func(context.Context, *bundle.Bundle) diag.Diagnostics { + b.Config.Bundle.Force = force + b.Config.Bundle.Deployment.Lock.Force = forceLock + if cmd.Flag("compute-id").Changed { + b.Config.Bundle.ComputeID = computeID + } + + if cmd.Flag("fail-on-active-runs").Changed { + b.Config.Bundle.Deployment.FailOnActiveRuns = failOnActiveRuns + } + + return nil + }) + + diags = diags.Extend( + bundle.Apply(ctx, b, bundle.Seq( + phases.Initialize(), + phases.Build(), + phases.Deploy(), + )), + ) } - bundle.ApplyFunc(ctx, b, func(context.Context, *bundle.Bundle) diag.Diagnostics { - b.Config.Bundle.Force = force - b.Config.Bundle.Deployment.Lock.Force = forceLock - if cmd.Flag("compute-id").Changed { - b.Config.Bundle.ComputeID = computeID - } - - if cmd.Flag("fail-on-active-runs").Changed { - b.Config.Bundle.Deployment.FailOnActiveRuns = failOnActiveRuns - } - - return nil - }) - - diags = bundle.Apply(ctx, b, bundle.Seq( - phases.Initialize(), - phases.Build(), - phases.Deploy(), - )) - if err := diags.Error(); err != nil { - return err + renderOpts := render.RenderOptions{RenderSummaryTable: false} + err := render.RenderTextOutput(cmd.OutOrStdout(), b, diags, renderOpts) + if err != nil { + return fmt.Errorf("failed to render output: %w", err) } + + if diags.HasError() { + return root.ErrAlreadyPrinted + } + return nil } diff --git a/cmd/bundle/validate.go b/cmd/bundle/validate.go index 59a9770476..496d5d2b50 100644 --- a/cmd/bundle/validate.go +++ b/cmd/bundle/validate.go @@ -53,7 +53,8 @@ func newValidateCommand() *cobra.Command { switch root.OutputType(cmd) { case flags.OutputText: - err := render.RenderTextOutput(cmd.OutOrStdout(), b, diags) + renderOpts := render.RenderOptions{RenderSummaryTable: true} + err := render.RenderTextOutput(cmd.OutOrStdout(), b, diags, renderOpts) if err != nil { return fmt.Errorf("failed to render output: %w", err) }