Support multiple locations for diagnostics by shreyas-goenka · Pull Request #1610 · databricks/cli

shreyas-goenka · 2024-07-18T16:04:49Z

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:

Warning for unknown keys in config.
Use of experimental.run_as
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

…ction and modify all diagnostics paths to be relative to the bundle root path

…rnings

shreyas-goenka · 2024-07-18T17:58:46Z

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 · 2024-07-19T12:49:21Z

Integration tests have been triggered, just in case.

shreyas-goenka · 2024-07-18T17:49:04Z

+ Severity: diag.Warning,
+ Summary: "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.",
+ Path: dyn.MustPathFromString("experimental.use_legacy_run_as"),
+Locations: b.Config.GetLocations("experimental.use_legacy_run_as"),


Note that we display multiple locations here since they are all relevant.

shreyas-goenka · 2024-07-18T18:01:55Z

- Path: loc.Path(),
+ Severity: diag.Warning,
+ Summary: fmt.Sprintf("Pattern %s does not match any files", p),
+Locations: []dyn.Location{loc.Location()},


Does not matter if we use loc.Locations() or just a single location here since array values can only have a single location during merging. Still only using a single location here to keep it easy to reason about.

shreyas-goenka · 2024-07-19T12:09:09Z

+ // Show only the location where the job_cluster_key is defined.
+ // Other associated locations are not relevant since they are
+ // overridden during merging.
+ Locations: []dyn.Location{loc.Location()},


Eventually I'll followup with a PR to show warnings when scalar configuration is being overridden.

shreyas-goenka · 2024-07-23T17:08:39Z

Also addressing the applicable comments in #1616

Bundles: * Add UUID function to bundle template functions ([#1612](#1612)). * Upgrade TF provider to 1.49.0 ([#1617](#1617)). * Upgrade TF provider to 1.49.1 ([#1626](#1626)). * Support multiple locations for diagnostics ([#1610](#1610)). * Split artifact cleanup into prepare step before build ([#1618](#1618)). * Move to a single prompt during bundle destroy ([#1583](#1583)). Internal: * Add tests for the Workspace API readahead cache ([#1605](#1605)). * Update Python dependencies before install when upgrading a labs project ([#1624](#1624)).

## 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 ```

Bundles: * Add UUID function to bundle template functions ([#1612](#1612)). * Upgrade TF provider to 1.49.0 ([#1617](#1617)). * Upgrade TF provider to 1.49.1 ([#1626](#1626)). * Support multiple locations for diagnostics ([#1610](#1610)). * Split artifact cleanup into prepare step before build ([#1618](#1618)). * Move to a single prompt during bundle destroy ([#1583](#1583)). Internal: * Add tests for the Workspace API readahead cache ([#1605](#1605)). * Update Python dependencies before install when upgrading a labs project ([#1624](#1624)).

shreyas-goenka added 9 commits July 18, 2024 18:01

rename Location -> Locations. Also add bundle.config.GetLocations fun…
4bedb54
…ction and modify all diagnostics paths to be relative to the bundle root path

validate.Location() -> validate.Locations()
7ab131f

fix TestSyncIncludeExcludeNoMatchesTest
39b3eea

patch to transform location -> locations
14a200a

add support for printing multiple locations in validate errors and wa…
b380344
…rnings

newlines in python_diagnostics_test.go
50b8fa5

newlines in python_mutator_test.go
4d1e2a0

clean up
3b8fc61

only select single location for undefined job_cluster_key
83558a6

shreyas-goenka added 6 commits July 18, 2024 20:00

single location for validate_sync_patterns.go
d9b8902

add cyan color to location
5b94edd

Fix test in python_diagnostics_test.go
52c2d2e

fix comment for Diagnostic.Locations
6ed74d0

include multiple locations for unknown field warning
1202bcc

fmt
16072f6

shreyas-goenka commented Jul 19, 2024
View reviewed changes

shreyas-goenka marked this pull request as ready for review July 19, 2024 12:50

shreyas-goenka requested review from andrewnester and pietern July 19, 2024 12:50

shreyas-goenka mentioned this pull request Jul 19, 2024
Introduce new type for a slice of dyn.Location #1611
Closed

Merge remote-tracking branch 'origin' into diag-multiple-locations
a602858

pietern reviewed Jul 23, 2024
View reviewed changes

Comment threadbundle/config/mutator/python/python_diagnostics_test.go Outdated
Comment threadbundle/config/root.go
Comment threadbundle/render/render_text_output.go
Comment threadlibs/dyn/convert/normalize.go

shreyas-goenka added 3 commits July 23, 2024 18:47

nil locations in python_diagnostics
9aedd3d

comment why plural is needed:
2ca95a0

inline printing locations
4beb8f9

shreyas-goenka requested a review from pietern July 23, 2024 17:06

pietern approved these changes Jul 23, 2024
View reviewed changes

shreyas-goenka added this pull request to the merge queue Jul 23, 2024

Merged via the queue into main with commit 4bf88b4Jul 23, 2024

shreyas-goenka deleted the diag-multiple-locations branch July 23, 2024 17:34

andrewnester mentioned this pull request Jul 25, 2024
Release v0.224.1 #1627
Merged

Navigation Menu

Uh oh!

Support multiple locations for diagnostics - #1610

Support multiple locations for diagnostics#1610
shreyas-goenka merged 19 commits into
mainfrom
diag-multiple-locations

shreyas-goenka commented Jul 18, 2024•
edited
Loading

Uh oh!

shreyas-goenka commented Jul 18, 2024

Uh oh!

shreyas-goenka commented Jul 19, 2024•
edited
Loading

Uh oh!

shreyas-goenkaJul 18, 2024

Uh oh!

shreyas-goenkaJul 18, 2024

Uh oh!

shreyas-goenkaJul 19, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shreyas-goenka commented Jul 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

shreyas-goenka commented Jul 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

shreyas-goenka commented Jul 18, 2024

Uh oh!

shreyas-goenka commented Jul 19, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shreyas-goenkaJul 18, 2024

Choose a reason for hiding this comment

Uh oh!

shreyas-goenkaJul 18, 2024

Choose a reason for hiding this comment

Uh oh!

shreyas-goenkaJul 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shreyas-goenka commented Jul 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shreyas-goenka commented Jul 18, 2024•
edited
Loading

shreyas-goenka commented Jul 19, 2024•
edited
Loading