-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return diag.Diagnostics
from mutators
#1305
Conversation
} | ||
return b.Config.Merge(this) | ||
// TODO: Return actual warnings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to be addressed in later PR.
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) |
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm, it's a valid value for Diagnostics
type so it works
Do integration tests pass? |
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)).
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.
diag.FromErr
fmt.Errorf
now returndiag.Errorf
err != nil
checks now usediags.HasError()
ordiags.Error()
Tests
./bundle
or./cmd/bundle
useserrors.Is
on the return value from mutators. This is relevant because we cannot wrap errors with%w
when callingdiag.Errorf
(likefmt.Errorf
; context in x/tools/go/analysis/passes/printf: Errorf functions are treated as fmt.Errorf-wrappers golang/go#47641).