Support multiple locations for diagnostics (#1610)

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

bundle/config/mutator/python/python_diagnostics.go

@@ -55,12 +55,18 @@ func parsePythonDiagnostics(input io.Reader) (diag.Diagnostics, error) {
 			return nil, fmt.Errorf("failed to parse path: %s", err)
 		}
 
+		var locations []dyn.Location
+		location := convertPythonLocation(parsedLine.Location)
+		if location != (dyn.Location{}) {
+			locations = append(locations, location)
+		}
+
 		diag := diag.Diagnostic{
-			Severity: severity,
-			Summary:  parsedLine.Summary,
-			Detail:   parsedLine.Detail,
-			Location: convertPythonLocation(parsedLine.Location),
-			Path:     path,
+			Severity:  severity,
+			Summary:   parsedLine.Summary,
+			Detail:    parsedLine.Detail,
+			Locations: locations,
+			Path:      path,
 		}
 
 		diags = diags.Append(diag)

bundle/config/mutator/python/python_diagnostics_test.go

@@ -39,10 +39,12 @@ func TestParsePythonDiagnostics(t *testing.T) {
 				{
 					Severity: diag.Error,
 					Summary:  "error summary",
-					Location: dyn.Location{
-						File:   "src/examples/file.py",
-						Line:   1,
-						Column: 2,
+					Locations: []dyn.Location{
+						{
+							File:   "src/examples/file.py",
+							Line:   1,
+							Column: 2,
+						},
 					},
 				},
 			},

bundle/config/mutator/python/python_mutator_test.go

@@ -97,11 +97,14 @@ func TestPythonMutator_load(t *testing.T) {
 
 	assert.Equal(t, 1, len(diags))
 	assert.Equal(t, "job doesn't have any tasks", diags[0].Summary)
-	assert.Equal(t, dyn.Location{
-		File:   "src/examples/file.py",
-		Line:   10,
-		Column: 5,
-	}, diags[0].Location)
+	assert.Equal(t, []dyn.Location{
+		{
+			File:   "src/examples/file.py",
+			Line:   10,
+			Column: 5,
+		},
+	}, diags[0].Locations)
+
 }
 
 func TestPythonMutator_load_disallowed(t *testing.T) {

bundle/config/mutator/run_as.go

@@ -178,10 +178,10 @@ func (m *setRunAs) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnostics {
 		setRunAsForJobs(b)
 		return diag.Diagnostics{
 			{
-				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"),
-				Location: b.Config.GetLocation("experimental.use_legacy_run_as"),
+				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"),
 			},
 		}
 	}

bundle/config/root.go

@@ -524,6 +524,17 @@ func (r Root) GetLocation(path string) dyn.Location {
 	return v.Location()
 }
 
+// Get all locations of the configuration value at the specified path. We need both
+// this function and it's singular version (GetLocation) because some diagnostics just need
+// the primary location and some need all locations associated with a configuration value.
+func (r Root) GetLocations(path string) []dyn.Location {
+	v, err := dyn.Get(r.value, path)
+	if err != nil {
+		return []dyn.Location{}
+	}
+	return v.Locations()
+}
+
 // Value returns the dynamic configuration value of the root object. This value
 // is the source of truth and is kept in sync with values in the typed configuration.
 func (r Root) Value() dyn.Value {

bundle/config/validate/files_to_sync.go

@@ -45,8 +45,10 @@ func (v *filesToSync) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.
 		diags = diags.Append(diag.Diagnostic{
 			Severity: diag.Warning,
 			Summary:  "There are no files to sync, please check your .gitignore and sync.exclude configuration",
-			Location: loc.Location(),
-			Path:     loc.Path(),
+			// Show all locations where sync.exclude is defined, since merging
+			// sync.exclude is additive.
+			Locations: loc.Locations(),
+			Path:      loc.Path(),
 		})
 	}
 

bundle/config/validate/job_cluster_key_defined.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/databricks/cli/bundle"
 	"github.com/databricks/cli/libs/diag"
+	"github.com/databricks/cli/libs/dyn"
 )
 
 func JobClusterKeyDefined() bundle.ReadOnlyMutator {
@@ -41,8 +42,11 @@ func (v *jobClusterKeyDefined) Apply(ctx context.Context, rb bundle.ReadOnlyBund
 					diags = diags.Append(diag.Diagnostic{
 						Severity: diag.Warning,
 						Summary:  fmt.Sprintf("job_cluster_key %s is not defined", task.JobClusterKey),
-						Location: loc.Location(),
-						Path:     loc.Path(),
+						// 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()},
+						Path:      loc.Path(),
 					})
 				}
 			}

bundle/config/validate/validate.go

@@ -20,6 +20,10 @@ func (l location) Location() dyn.Location {
 	return l.rb.Config().GetLocation(l.path)
 }
 
+func (l location) Locations() []dyn.Location {
+	return l.rb.Config().GetLocations(l.path)
+}
+
 func (l location) Path() dyn.Path {
 	return dyn.MustPathFromString(l.path)
 }

bundle/config/validate/validate_sync_patterns.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/databricks/cli/bundle"
 	"github.com/databricks/cli/libs/diag"
+	"github.com/databricks/cli/libs/dyn"
 	"github.com/databricks/cli/libs/fileset"
 	"golang.org/x/sync/errgroup"
 )
@@ -64,10 +65,10 @@ func checkPatterns(patterns []string, path string, rb bundle.ReadOnlyBundle) (di
 				loc := location{path: fmt.Sprintf("%s[%d]", path, index), rb: rb}
 				mu.Lock()
 				diags = diags.Append(diag.Diagnostic{
-					Severity: diag.Warning,
-					Summary:  fmt.Sprintf("Pattern %s does not match any files", p),
-					Location: loc.Location(),
-					Path:     loc.Path(),
+					Severity:  diag.Warning,
+					Summary:   fmt.Sprintf("Pattern %s does not match any files", p),
+					Locations: []dyn.Location{loc.Location()},
+					Path:      loc.Path(),
 				})
 				mu.Unlock()
 			}

bundle/render/render_text_output.go

@@ -32,8 +32,8 @@ const errorTemplate = `{{ "Error" | red }}: {{ .Summary }}
 {{- if .Path.String }}
   {{ "at " }}{{ .Path.String | green }}
 {{- end }}
-{{- if .Location.File }}
-  {{ "in " }}{{ .Location.String | cyan }}
+{{- range $index, $element := .Locations }}
+  {{ if eq $index 0 }}in {{else}}   {{ end}}{{ $element.String | cyan }}
 {{- end }}
 {{- if .Detail }}
 
@@ -46,8 +46,8 @@ const warningTemplate = `{{ "Warning" | yellow }}: {{ .Summary }}
 {{- if .Path.String }}
   {{ "at " }}{{ .Path.String | green }}
 {{- end }}
-{{- if .Location.File }}
-  {{ "in " }}{{ .Location.String | cyan }}
+{{- range $index, $element := .Locations }}
+  {{ if eq $index 0 }}in {{else}}   {{ end}}{{ $element.String | cyan }}
 {{- end }}
 {{- if .Detail }}
 
@@ -141,12 +141,18 @@ func renderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics)
 			t = warningT
 		}
 
-		// Make file relative to bundle root
-		if d.Location.File != "" && b != nil {
-			out, err := filepath.Rel(b.RootPath, d.Location.File)
-			// if we can't relativize the path, just use path as-is
-			if err == nil {
-				d.Location.File = out
+		for i := range d.Locations {
+			if b == nil {
+				break
+			}
+
+			// Make location relative to bundle root
+			if d.Locations[i].File != "" {
+				out, err := filepath.Rel(b.RootPath, d.Locations[i].File)
+				// if we can't relativize the path, just use path as-is
+				if err == nil {
+					d.Locations[i].File = out
+				}
 			}
 		}
 

bundle/render/render_text_output_test.go

@@ -88,34 +88,22 @@ func TestRenderTextOutput(t *testing.T) {
 			bundle: loadingBundle,
 			diags: diag.Diagnostics{
 				diag.Diagnostic{
-					Severity: diag.Error,
-					Summary:  "error (1)",
-					Detail:   "detail (1)",
-					Location: dyn.Location{
-						File:   "foo.py",
-						Line:   1,
-						Column: 1,
-					},
+					Severity:  diag.Error,
+					Summary:   "error (1)",
+					Detail:    "detail (1)",
+					Locations: []dyn.Location{{File: "foo.py", Line: 1, Column: 1}},
 				},
 				diag.Diagnostic{
-					Severity: diag.Error,
-					Summary:  "error (2)",
-					Detail:   "detail (2)",
-					Location: dyn.Location{
-						File:   "foo.py",
-						Line:   2,
-						Column: 1,
-					},
+					Severity:  diag.Error,
+					Summary:   "error (2)",
+					Detail:    "detail (2)",
+					Locations: []dyn.Location{{File: "foo.py", Line: 2, Column: 1}},
 				},
 				diag.Diagnostic{
-					Severity: diag.Warning,
-					Summary:  "warning (3)",
-					Detail:   "detail (3)",
-					Location: dyn.Location{
-						File:   "foo.py",
-						Line:   3,
-						Column: 1,
-					},
+					Severity:  diag.Warning,
+					Summary:   "warning (3)",
+					Detail:    "detail (3)",
+					Locations: []dyn.Location{{File: "foo.py", Line: 3, Column: 1}},
 				},
 			},
 			opts: RenderOptions{RenderSummaryTable: true},
@@ -174,24 +162,16 @@ func TestRenderTextOutput(t *testing.T) {
 			bundle: nil,
 			diags: diag.Diagnostics{
 				diag.Diagnostic{
-					Severity: diag.Error,
-					Summary:  "error (1)",
-					Detail:   "detail (1)",
-					Location: dyn.Location{
-						File:   "foo.py",
-						Line:   1,
-						Column: 1,
-					},
+					Severity:  diag.Error,
+					Summary:   "error (1)",
+					Detail:    "detail (1)",
+					Locations: []dyn.Location{{File: "foo.py", Line: 1, Column: 1}},
 				},
 				diag.Diagnostic{
-					Severity: diag.Warning,
-					Summary:  "warning (2)",
-					Detail:   "detail (2)",
-					Location: dyn.Location{
-						File:   "foo.py",
-						Line:   3,
-						Column: 1,
-					},
+					Severity:  diag.Warning,
+					Summary:   "warning (2)",
+					Detail:    "detail (2)",
+					Locations: []dyn.Location{{File: "foo.py", Line: 3, Column: 1}},
 				},
 			},
 			opts: RenderOptions{RenderSummaryTable: false},
@@ -252,17 +232,42 @@ func TestRenderDiagnostics(t *testing.T) {
 					Severity: diag.Error,
 					Summary:  "failed to load xxx",
 					Detail:   "'name' is required",
-					Location: dyn.Location{
+					Locations: []dyn.Location{{
 						File:   "foo.yaml",
 						Line:   1,
-						Column: 2,
-					},
+						Column: 2}},
 				},
 			},
 			expected: "Error: failed to load xxx\n" +
 				"  in foo.yaml:1:2\n\n" +
 				"'name' is required\n\n",
 		},
+		{
+			name: "error with multiple source locations",
+			diags: diag.Diagnostics{
+				{
+					Severity: diag.Error,
+					Summary:  "failed to load xxx",
+					Detail:   "'name' is required",
+					Locations: []dyn.Location{
+						{
+							File:   "foo.yaml",
+							Line:   1,
+							Column: 2,
+						},
+						{
+							File:   "bar.yaml",
+							Line:   3,
+							Column: 4,
+						},
+					},
+				},
+			},
+			expected: "Error: failed to load xxx\n" +
+				"  in foo.yaml:1:2\n" +
+				"     bar.yaml:3:4\n\n" +
+				"'name' is required\n\n",
+		},
 		{
 			name: "error with path",
 			diags: diag.Diagnostics{

bundle/tests/sync_include_exclude_no_matches_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/databricks/cli/bundle"
 	"github.com/databricks/cli/bundle/config/validate"
 	"github.com/databricks/cli/libs/diag"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -21,11 +22,13 @@ func TestSyncIncludeExcludeNoMatchesTest(t *testing.T) {
 
 	require.Equal(t, diags[0].Severity, diag.Warning)
 	require.Equal(t, diags[0].Summary, "Pattern dist does not match any files")
-	require.Equal(t, diags[0].Location.File, filepath.Join("sync", "override", "databricks.yml"))
-	require.Equal(t, diags[0].Location.Line, 17)
-	require.Equal(t, diags[0].Location.Column, 11)
 	require.Equal(t, diags[0].Path.String(), "sync.exclude[0]")
 
+	assert.Len(t, diags[0].Locations, 1)
+	require.Equal(t, diags[0].Locations[0].File, filepath.Join("sync", "override", "databricks.yml"))
+	require.Equal(t, diags[0].Locations[0].Line, 17)
+	require.Equal(t, diags[0].Locations[0].Column, 11)
+
 	summaries := []string{
 		fmt.Sprintf("Pattern %s does not match any files", filepath.Join("src", "*")),
 		fmt.Sprintf("Pattern %s does not match any files", filepath.Join("tests", "*")),

libs/diag/diagnostic.go

@@ -17,9 +17,9 @@ type Diagnostic struct {
 	// This may be multiple lines and may be nil.
 	Detail string
 
-	// Location is a source code location associated with the diagnostic message.
-	// It may be zero if there is no associated location.
-	Location dyn.Location
+	// Locations are the source code locations associated with the diagnostic message.
+	// It may be empty if there are no associated locations.
+	Locations []dyn.Location
 
 	// Path is a path to the value in a configuration tree that the diagnostic is associated with.
 	// It may be nil if there is no associated path.