Return `diag.Diagnostics` from mutators by pietern · Pull Request #1305 · databricks/cli

pietern · 2024-03-22T16:23:23Z

Changes

This diagnostics type allows us to capture multiple warnings as well as errors in the return value. This is a preparation for returning additional warnings from mutators in case we detect non-fatal problems.

All return statements that previously returned an error now return diag.FromErr
All return statements that previously returned fmt.Errorf now return diag.Errorf
All err != nil checks now use diags.HasError() or diags.Error()

Tests

Existing tests pass.
I confirmed no call site under ./bundle or ./cmd/bundle uses errors.Is on the return value from mutators. This is relevant because we cannot wrap errors with %w when calling diag.Errorf (like fmt.Errorf; context in x/tools/go/analysis/passes/printf: Errorf functions are treated as fmt.Errorf-wrappers golang/go#47641).

libs/diag/diagnostic.go

pietern · 2024-03-22T16:27:14Z

bundle/config/mutator/process_include.go

+		return diag.FromErr(err)
 	}
-	return b.Config.Merge(this)
+	// TODO: Return actual warnings.


This is to be addressed in later PR.

bundle/mutator.go

pietern · 2024-03-22T16:33:20Z

For whoever finds this interesting:

I used gopatch to automate most of these rewrites. I incrementally added patches as I found new rewrite patterns and kept running the patch command. Finally, I did a manual pass to confirm everything looked good.

Patch file (some duplication and not necessarily optimal):

@@
@@
-func (...) Apply(ctx context.Context, b *bundle.Bundle) error {
+func (...) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
    ...
}

@@
var b identifier
@@
-func (...) Apply(_ context.Context, b *bundle.Bundle) error {
+func (...) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnostics {
    ...
}

@@
var x identifier
var y identifier
@@
func (...) Apply(x context.Context, y *bundle.Bundle) diag.Diagnostics {
    ...
- return err
+ return diag.FromErr(err)
    ...
}

@@
@@
import "github.com/databricks/cli/libs/diag"

 type Mutator interface {
   ...
-  Apply(context.Context, *Bundle) error
+  Apply(context.Context, *Bundle) diag.Diagnostics
 }

@@
var x identifier
@@
-err := bundle.Apply(...)
-x.NoError(t, err)

+diags := bundle.Apply(...)
+x.Empty(t, diags)

@@
@@
func (...) Apply(...) diag.Diagnostics {
    ...
-   return b.Config.Mutate(...)
+   err := b.Config.Mutate(...)
+
+   return diag.FromErr(err)
}

@@
var x identifier
@@
-err := bundle.Apply(...)
-x.Error(t, err)
+diags := bundle.Apply(...)
+x.True(t, diags.HasError())

@@
var x identifier
var exp expression
@@
-err := bundle.Apply(...)
-x.ErrorContains(t, err, exp)
+diags := bundle.Apply(...)
+x.ErrorContains(t, diags.Error(), exp)

@@
var x identifier
var exp expression
@@
-err := bundle.Apply(...)
-x.Error(t, err, exp)
+diags := bundle.Apply(...)
+x.Error(t, diags.Error(), exp)

andrewnester · 2024-03-25T12:38:21Z

bundle/mutator.go

+	// Note: errors should be logged when constructing them
+	// such that they are not logged multiple times.
+	// If this is done, we can omit this block.
+	if err := diags.Error(); err != nil {


If I understand correctly by looking at diag.FromErr, diags can be nil if err passed is nil. In this case diags.Error() will wail, is it so?

Nvm, it's a valid value for Diagnostics type so it works

andrewnester · 2024-03-25T12:43:18Z

Do integration tests pass?

pietern · 2024-03-25T14:18:42Z

Integration tests pass.

CLI: * Added `auth describe` command ([#1244](#1244)). * Fixed message for successful auth describe run ([#1336](#1336)). Bundles: * Make bundle validation print text output by default ([#1335](#1335)). * Use UserName field to identify if service principal is used ([#1310](#1310)). * Allow unknown properties in the config file for template initialization ([#1315](#1315)). * Remove support for DATABRICKS_BUNDLE_INCLUDES ([#1317](#1317)). * Make `bundle.deployment` optional in the bundle schema ([#1321](#1321)). * Add allow list for resources when bundle `run_as` is set ([#1233](#1233)). * Fix the generated DABs JSON schema ([#1322](#1322)). * Make bundle loaders return diagnostics ([#1319](#1319)). * Add `bundle debug terraform` command ([#1294](#1294)). * Allow specifying CLI version constraints required to run the bundle ([#1320](#1320)). Internal: * Retain location information of variable reference ([#1333](#1333)). * Define `dyn.Mapping` to represent maps ([#1301](#1301)). * Return `diag.Diagnostics` from mutators ([#1305](#1305)). * Fix flaky test in `libs/process` ([#1314](#1314)). * Move path field to bundle type ([#1316](#1316)). * Load bundle configuration from mutator ([#1318](#1318)). * Return diagnostics from `config.Load` ([#1324](#1324)). * Return warning for nil primitive types during normalization ([#1329](#1329)). * Include `dyn.Path` in normalization warnings and errors ([#1332](#1332)). * Make normalization return warnings instead of errors ([#1334](#1334)). API Changes: * Added `databricks lakeview migrate` command. * Added `databricks lakeview unpublish` command. * Changed `databricks ip-access-lists get` command . New request type is . OpenAPI commit e316cc3d78d087522a74650e26586088da9ac8cb (2024-04-03) Dependency updates: * Bump github.com/databricks/databricks-sdk-go from 0.36.0 to 0.37.0 ([#1326](#1326)).

Breaking Change: * Add allow list for resources when bundle `run_as` is set ([#1233](#1233)). * Make bundle validation print text output by default ([#1335](#1335)). CLI: * Added `auth describe` command ([#1244](#1244)). * Fixed message for successful auth describe run ([#1336](#1336)). Bundles: * Use UserName field to identify if service principal is used ([#1310](#1310)). * Allow unknown properties in the config file for template initialization ([#1315](#1315)). * Remove support for DATABRICKS_BUNDLE_INCLUDES ([#1317](#1317)). * Make `bundle.deployment` optional in the bundle schema ([#1321](#1321)). * Fix the generated DABs JSON schema ([#1322](#1322)). * Make bundle loaders return diagnostics ([#1319](#1319)). * Add `bundle debug terraform` command ([#1294](#1294)). * Allow specifying CLI version constraints required to run the bundle ([#1320](#1320)). Internal: * Retain location information of variable reference ([#1333](#1333)). * Define `dyn.Mapping` to represent maps ([#1301](#1301)). * Return `diag.Diagnostics` from mutators ([#1305](#1305)). * Fix flaky test in `libs/process` ([#1314](#1314)). * Move path field to bundle type ([#1316](#1316)). * Load bundle configuration from mutator ([#1318](#1318)). * Return diagnostics from `config.Load` ([#1324](#1324)). * Return warning for nil primitive types during normalization ([#1329](#1329)). * Include `dyn.Path` in normalization warnings and errors ([#1332](#1332)). * Make normalization return warnings instead of errors ([#1334](#1334)). API Changes: * Added `databricks lakeview migrate` command. * Added `databricks lakeview unpublish` command. * Changed `databricks ip-access-lists get` command . New request type is . OpenAPI commit e316cc3d78d087522a74650e26586088da9ac8cb (2024-04-03) Dependency updates: * Bump github.com/databricks/databricks-sdk-go from 0.36.0 to 0.37.0 ([#1326](#1326)).

pietern added 11 commits March 22, 2024 15:56

tmp

08ea4af

wip

585aa92

wip

8572aff

wip

61b28c0

wip

9c28481

wip

8e7eb8e

wip

398bef4

Replace %w with %v

6289f67

wip

7176fd9

wip

8f65288

wip

fa7c64e

pietern changed the title x Return diag.Diagnostics from mutators Mar 22, 2024

pietern commented Mar 22, 2024

View reviewed changes

wip

f77e9c9

pietern added 4 commits March 25, 2024 12:18

Comments

62d0286

More NoError

0e949db

Capture diag error in if statement

2180212

Merge branch 'main' into bundle-diags

b009c0a

pietern marked this pull request as ready for review March 25, 2024 12:09

pietern requested review from andrewnester and shreyas-goenka and removed request for andrewnester March 25, 2024 12:09

andrewnester reviewed Mar 25, 2024

View reviewed changes

andrewnester self-requested a review March 25, 2024 12:42

andrewnester approved these changes Mar 25, 2024

View reviewed changes

pietern added this pull request to the merge queue Mar 25, 2024

Merged via the queue into main with commit ed19466 Mar 25, 2024

pietern deleted the bundle-diags branch March 25, 2024 14:25

andrewnester mentioned this pull request Apr 3, 2024

Release v0.217.0 #1337

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return `diag.Diagnostics` from mutators#1305

Return `diag.Diagnostics` from mutators#1305
pietern merged 16 commits intomainfrom
bundle-diags

pietern commented Mar 22, 2024 •

edited

Loading

Uh oh!

Uh oh!

pietern Mar 22, 2024 •

edited

Loading

Uh oh!

Uh oh!

pietern commented Mar 22, 2024

Uh oh!

andrewnester Mar 25, 2024

Uh oh!

andrewnester Mar 25, 2024

Uh oh!

andrewnester commented Mar 25, 2024

Uh oh!

pietern commented Mar 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

pietern commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

Uh oh!

pietern Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pietern commented Mar 22, 2024

Uh oh!

andrewnester Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

andrewnester Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

andrewnester commented Mar 25, 2024

Uh oh!

pietern commented Mar 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pietern commented Mar 22, 2024 •

edited

Loading

pietern Mar 22, 2024 •

edited

Loading