databricks / cli

Databricks CLI
Other
132 stars 50 forks source link

Support multiple locations for diagnostics #1610

Closed shreyas-goenka closed 2 months ago

shreyas-goenka commented 2 months ago

Changes

This PR changes diag.Diagnostics to allow including multiple locations associated with the diagnostic message. The diagnostics that now return multiple locations with this PR are:

  1. Warning for unknown keys in config.
  2. Use of experimental.run_as
  3. Accidental sync.exludes that exclude all files.

Tests

Existing unit tests pass. New unit test case to assert on error message when multiple locations are included.

Example output:

➜  bundle-playground-2 ~/cli2/cli/cli bundle validate              
Warning: You are using the legacy mode of run_as. The support for this mode is experimental and might be removed in a future release of the CLI. In order to run the DLT pipelines in your DAB as the run_as user this mode changes the owners of the pipelines to the run_as identity, which requires the user deploying the bundle to be a workspace admin, and also a Metastore admin if the pipeline target is in UC.
  at experimental.use_legacy_run_as
  in resources.yml:10:22
     databricks.yml:13:22

Name: fix run_if
Target: default
Workspace:
  User: shreyas.goenka@databricks.com
  Path: /Users/shreyas.goenka@databricks.com/.bundle/fix run_if/default

Found 1 warning
shreyas-goenka commented 2 months ago

Just FYI, gopatch used to get refactor and get to a starting point for more custom changes. There's something satisfying about writing patch files:

@@
var t expression
@@
diag.Diagnostic{
    ...,
-   Locations: t,
+   Locations: []dyn.Location{t},
    ...,
}

@@
@@
diag.Diagnostics{
    {
        ...,
-       Locations: dyn.Location{...},
+       Locations: []dyn.Location{{...}},
    },
}

@@
@@
package python

-assert.Equal(t, dyn.Location{...}, diags[0].Locations)
+assert.Equal(t, []dyn.Location{{...}}, diags[0].Locations)

@@
@@
-b.Config.GetLocation("experimental.use_legacy_run_as")
+b.Config.GetLocations("experimental.use_legacy_run_as")

@@
var x identifier
@@
package validate

diag.Diagnostic{
    ...,
-   Locations: []dyn.Location{x.Location()},
+   Locations: x.Locations(),
    ...,
}

@@
@@
-[]dyn.Location{dyn.Location{...}}
+[]dyn.Location{{...}}
shreyas-goenka commented 2 months ago

Integration tests have been triggered, just in case.

shreyas-goenka commented 2 months ago

Also addressing the applicable comments in https://github.com/databricks/cli/pull/1616