Improve `bundle validate` output #1532

kanterov · 2024-06-26T14:51:55Z

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

pietern

This is great, thanks.

Please move the render package under the top level bundle package.

Does the output change at all for existing warnings?

cmd/bundle/utils/utils.go

kanterov · 2024-06-27T13:22:50Z

@pietern I've manually checked if there is a diff for warnings. There were a couple of extra new lines, and I've added tests for these cases. I also added tests in case the bundle is nil so we don't crash.

I also moved the package as you suggested. Please take a look again.

pietern · 2024-07-01T07:10:49Z

bundle/render/render_text_output.go

+// 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.


The error isn't returned. Stale comment?

Oh, right. Initially I implemented as described, but then realized it's cleaner not to have this behaviour, and return nil instead.

pietern · 2024-07-01T07:11:26Z

bundle/render/render_text_output.go

+{{- if .Location.File }}
+  {{ "in " }}{{ .Location.String | cyan }}
+{{- end }}
+


No detail for warning?

I can add it for consistency, but it wasn't there before, and there is no use case for it right now. We simply put the whole body into "summary". I will add it.

Added detail + unit test for it

pietern · 2024-07-01T07:31:28Z

cmd/bundle/validate.go

+			if err := diags.Error(); err != nil {
+				return diags.Error()
+			} else {
+				panic("failed to load bundle")


Please return fmt.Errorf with some description. Always better (and friendlier) than a panic.

The reason why I added panic is because it should never happen. If this happens, invariant is violated, and we have a bug in our code, so we should crash immediately. I can change it to fmt.Errorf if you think it's more appropriate.

pietern · 2024-07-01T07:33:25Z

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 != ErrAlreadyPrinted {


This should use errors.Is so that it also works if (for whatever reason) the error is wrapped.

Bundles: As of this release you can interact with bundles when running the CLI on DBR (e.g. via the Web Terminal). * Fix non-default project names not working in dbt-sql template ([#1500](#1500)). * Improve `bundle validate` output ([#1532](#1532)). * Fixed resolving variable references inside slice variable ([#1550](#1550)). * Fixed bundle not loading when empty variable is defined ([#1552](#1552)). * Use `vfs.Path` for filesystem interaction ([#1554](#1554)). * Replace `vfs.Path` with extension-aware filer when running on DBR ([#1556](#1556)). Internal: * merge.Override: Fix handling of dyn.NilValue ([#1530](#1530)). * Compare `.Kind()` instead of direct equality checks on a `dyn.Value` ([#1520](#1520)). * PythonMutator: register product in user agent extra ([#1533](#1533)). * Ignore `dyn.NilValue` when traversing value from `dyn.Map` ([#1547](#1547)). * Add extra tests for the sync block ([#1548](#1548)). * PythonMutator: add diagnostics ([#1531](#1531)). * PythonMutator: support omitempty in PyDABs ([#1513](#1513)). * PythonMutator: allow insert 'resources' and 'resources.jobs' ([#1555](#1555)).

## Changes This snuck into #1532 right before merging. The result is that error output is no longer logged. This includes actual execution errors as well as help output if arguments or flags are incorrectly specified. We don't have test coverage for the `root.Execute` function. This is to be fixed later. ## Tests Manually confirmed we observe error output again.

This bugfix release fixes missing error messages in v0.223.0. CLI: * Fix logic error in [#1532](#1532) ([#1564](#1564)).

Improve bundle validate output

d63d350

kanterov force-pushed the improve-validate-output branch from 44a64a2 to d63d350 Compare June 26, 2024 14:54

kanterov marked this pull request as draft June 26, 2024 14:54

pietern reviewed Jun 27, 2024

View reviewed changes

cmd/bundle/utils/utils.go Show resolved Hide resolved

kanterov added 7 commits June 27, 2024 13:24

Add more tests for whitespace

112beac

Fix more whitespace problems

9de21bb

Move cmd/bundle/render -> bundle/render

e0a74d6

Fix build

a461d93

Fix lint

7b2fb10

Fix tests

7e964f7

Fix fmt

04caf10

kanterov marked this pull request as ready for review June 27, 2024 13:21

kanterov requested a review from pietern June 27, 2024 13:22

pietern reviewed Jul 1, 2024

View reviewed changes

Address CR comments

6282a4e

kanterov force-pushed the improve-validate-output branch from 2f63090 to 6282a4e Compare July 1, 2024 08:42

kanterov and others added 2 commits July 1, 2024 10:53

Don't panic

76b76a1

Merge branch 'main' into improve-validate-output

9c2e7f9

pietern approved these changes Jul 1, 2024

View reviewed changes

pietern enabled auto-merge July 1, 2024 08:56

pietern added this pull request to the merge queue Jul 1, 2024

Merged via the queue into databricks:main with commit e8b76a7 Jul 1, 2024
5 checks passed

pietern mentioned this pull request Jul 3, 2024

Release v0.223.0 #1557

Merged

pietern added a commit that referenced this pull request Jul 3, 2024

Fix logic error in #1532

367d4f8

pietern mentioned this pull request Jul 3, 2024

Fix logic error in #1532 #1564

Merged

pietern added a commit that referenced this pull request Jul 3, 2024

Release v0.223.1

df41b4d

This bugfix release fixes missing error messages in v0.223.0. CLI: * Fix logic error in [#1532](#1532) ([#1564](#1564)).

pietern mentioned this pull request Jul 3, 2024

Release v0.223.1 #1565

Merged

github-merge-queue bot pushed a commit that referenced this pull request Jul 3, 2024

Release v0.223.1 (#1565)

bf27542

This bugfix release fixes missing error messages in v0.223.0. CLI: * Fix logic error in [#1532](#1532) ([#1564](#1564)).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `bundle validate` output #1532

Improve `bundle validate` output #1532

kanterov commented Jun 26, 2024 •

edited by pietern

Loading

pietern left a comment

kanterov commented Jun 27, 2024

pietern Jul 1, 2024

kanterov Jul 1, 2024

pietern Jul 1, 2024

kanterov Jul 1, 2024

kanterov Jul 1, 2024

pietern Jul 1, 2024

kanterov Jul 1, 2024

pietern Jul 1, 2024

Improve bundle validate output #1532

Improve bundle validate output #1532

Conversation

kanterov commented Jun 26, 2024 • edited by pietern Loading

Changes

Tests

pietern left a comment

Choose a reason for hiding this comment

kanterov commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Improve `bundle validate` output #1532

Improve `bundle validate` output #1532

kanterov commented Jun 26, 2024 •

edited by pietern

Loading