Skip to content
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

Merged
merged 16 commits into from
Mar 25, 2024
Merged

Return diag.Diagnostics from mutators #1305

merged 16 commits into from
Mar 25, 2024

Conversation

pietern
Copy link
Contributor

@pietern pietern commented Mar 22, 2024

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

@pietern pietern changed the title x Return diag.Diagnostics from mutators Mar 22, 2024
libs/diag/diagnostic.go Outdated Show resolved Hide resolved
}
return b.Config.Merge(this)
// TODO: Return actual warnings.
Copy link
Contributor Author

@pietern pietern Mar 22, 2024

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.

bundle/mutator.go Outdated Show resolved Hide resolved
@pietern
Copy link
Contributor Author

pietern commented Mar 22, 2024

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)

@pietern pietern marked this pull request as ready for review March 25, 2024 12:09
@pietern pietern requested review from andrewnester and shreyas-goenka and removed request for andrewnester March 25, 2024 12:09
// 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 {
Copy link
Contributor

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?

Copy link
Contributor

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

@andrewnester andrewnester self-requested a review March 25, 2024 12:42
@andrewnester
Copy link
Contributor

Do integration tests pass?

@pietern
Copy link
Contributor Author

pietern commented Mar 25, 2024

Integration tests pass.

@pietern pietern added this pull request to the merge queue Mar 25, 2024
Merged via the queue into main with commit ed19466 Mar 25, 2024
4 checks passed
@pietern pietern deleted the bundle-diags branch March 25, 2024 14:25
andrewnester added a commit that referenced this pull request Apr 3, 2024
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)).
@andrewnester andrewnester mentioned this pull request Apr 3, 2024
github-merge-queue bot pushed a commit that referenced this pull request Apr 3, 2024
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)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants