Merge 489aba8d76 into e4d039a1aa

## Changes

The issue reported in #1828 illustrates how using a YAML timestamp-like
value (a date in this case) causes an issue during conversion to and
from the typed configuration tree.

We use the `AsAny()` function on the `dyn.Value` when normalizing for
the `any` type. We only use the `any` type for variable values, because
they can assume every type. The `AsAny()` function returns a `time.Time`
for the time value during conversion **to** the typed configuration
tree. Upon conversion **from** the typed configuration tree back into
the dynamic configuration tree, we cannot distinguish a `time.Time`
struct from any other struct.

To address this, we use the underlying string value of the time value
when we normalize for the `any` type.

Fixes #1828.

## Tests

Existing unit tests pass

bundle/artifacts/expand_globs.go

@@ -30,10 +30,14 @@ func createGlobError(v dyn.Value, p dyn.Path, message string) diag.Diagnostic {
 	}
 
 	return diag.Diagnostic{
-		Severity:  diag.Error,
-		Summary:   fmt.Sprintf("%s: %s", source, message),
-		Locations: []dyn.Location{v.Location()},
-		Paths:     []dyn.Path{p},
+		Severity: diag.Error,
+		Summary:  fmt.Sprintf("%s: %s", source, message),
+		LocationPathPairs: []diag.LocationPathPair{
+			{
+				L: v.Location(),
+				P: p,
+			},
+		},
 	}
 }
 

bundle/config/loader/process_include.go

@@ -107,27 +107,24 @@ func validateSingleResourceDefined(configRoot dyn.Value, ext, typ string) diag.D
 		detail.WriteString(l)
 	}
 
-	locations := []dyn.Location{}
-	paths := []dyn.Path{}
+	// TODO: test this.
+	locationPathPairs := []diag.LocationPathPair{}
 	for _, rr := range resources {
-		locations = append(locations, rr.value.Locations()...)
-		paths = append(paths, rr.path)
+		for _, l := range rr.value.Locations() {
+			locationPathPairs = append(locationPathPairs, diag.LocationPathPair{L: l, P: rr.path})
+		}
 	}
-	// Sort the locations and paths to make the output deterministic.
-	sort.Slice(locations, func(i, j int) bool {
-		return locations[i].String() < locations[j].String()
-	})
-	sort.Slice(paths, func(i, j int) bool {
-		return paths[i].String() < paths[j].String()
+	// Sort the location-path pairs to make the output deterministic.
+	sort.Slice(locationPathPairs, func(i, j int) bool {
+		return locationPathPairs[i].L.String() < locationPathPairs[j].L.String()
 	})
 
 	return diag.Diagnostics{
 		{
-			Severity:  diag.Recommendation,
-			Summary:   fmt.Sprintf("define a single %s in a file with the %s extension.", strings.ReplaceAll(typ, "_", " "), ext),
-			Detail:    detail.String(),
-			Locations: locations,
-			Paths:     paths,
+			Severity:          diag.Recommendation,
+			Summary:           fmt.Sprintf("define a single %s in a file with the %s extension.", strings.ReplaceAll(typ, "_", " "), ext),
+			Detail:            detail.String(),
+			LocationPathPairs: locationPathPairs,
 		},
 	}
 }

bundle/config/mutator/apply_presets.go

@@ -220,10 +220,16 @@ func validatePauseStatus(b *bundle.Bundle) diag.Diagnostics {
 	if p == "" || p == config.Paused || p == config.Unpaused {
 		return nil
 	}
+	// TODO: test this.
 	return diag.Diagnostics{{
-		Summary:   "Invalid value for trigger_pause_status, should be PAUSED or UNPAUSED",
-		Severity:  diag.Error,
-		Locations: []dyn.Location{b.Config.GetLocation("presets.trigger_pause_status")},
+		Summary:  "Invalid value for trigger_pause_status, should be PAUSED or UNPAUSED",
+		Severity: diag.Error,
+		LocationPathPairs: []diag.LocationPathPair{
+			{
+				L: b.Config.GetLocation("presets.trigger_pause_status"),
+				P: dyn.MustPathFromString("presets.trigger_pause_status"),
+			},
+		},
 	}}
 }
 

bundle/config/mutator/compute_id_compat.go

@@ -52,11 +52,19 @@ func rewriteComputeIdToClusterId(v dyn.Value, p dyn.Path) (dyn.Value, diag.Diagn
 		return v, nil
 	}
 
+	// TODO: test this.
+	locationPathPaths := []diag.LocationPathPair{}
+	for _, l := range computeId.Locations() {
+		locationPathPaths = append(locationPathPaths, diag.LocationPathPair{
+			L: l,
+			P: computeIdPath,
+		})
+	}
+
 	diags = diags.Append(diag.Diagnostic{
-		Severity:  diag.Warning,
-		Summary:   "compute_id is deprecated, please use cluster_id instead",
-		Locations: computeId.Locations(),
-		Paths:     []dyn.Path{computeIdPath},
+		Severity:          diag.Warning,
+		Summary:           "compute_id is deprecated, please use cluster_id instead",
+		LocationPathPairs: locationPathPaths,
 	})
 
 	clusterIdPath := p.Append(dyn.Key("cluster_id"))

bundle/config/mutator/process_target_mode.go

@@ -76,9 +76,15 @@ func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics {
 	// historically allowed.)
 	if p.TriggerPauseStatus == config.Unpaused {
 		diags = diags.Append(diag.Diagnostic{
-			Severity:  diag.Error,
-			Summary:   "target with 'mode: development' cannot set trigger pause status to UNPAUSED by default",
-			Locations: []dyn.Location{b.Config.GetLocation("presets.trigger_pause_status")},
+			Severity: diag.Error,
+			Summary:  "target with 'mode: development' cannot set trigger pause status to UNPAUSED by default",
+			// TODO: test this.
+			LocationPathPairs: []diag.LocationPathPair{
+				{
+					L: b.Config.GetLocation("presets.trigger_pause_status"),
+					P: dyn.MustPathFromString("presets.trigger_pause_status"),
+				},
+			},
 		})
 	}
 
@@ -98,9 +104,15 @@ func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics {
 		// it's a pitfall for users if they don't include it and later find out that
 		// only a single user can do development deployments.
 		diags = diags.Append(diag.Diagnostic{
-			Severity:  diag.Error,
-			Summary:   "prefix should contain the current username or ${workspace.current_user.short_name} to ensure uniqueness when using 'mode: development'",
-			Locations: []dyn.Location{b.Config.GetLocation("presets.name_prefix")},
+			Severity: diag.Error,
+			Summary:  "prefix should contain the current username or ${workspace.current_user.short_name} to ensure uniqueness when using 'mode: development'",
+			// TODO: test this.
+			LocationPathPairs: []diag.LocationPathPair{
+				{
+					L: b.Config.GetLocation("presets.name_prefix"),
+					P: dyn.MustPathFromString("presets.name_prefix"),
+				},
+			},
 		})
 	}
 	return diags

bundle/config/mutator/python/python_diagnostics.go

@@ -54,23 +54,23 @@ func parsePythonDiagnostics(input io.Reader) (diag.Diagnostics, error) {
 		if err != nil {
 			return nil, fmt.Errorf("failed to parse path: %s", err)
 		}
-		var paths []dyn.Path
-		if path != nil {
-			paths = []dyn.Path{path}
-		}
 
-		var locations []dyn.Location
 		location := convertPythonLocation(parsedLine.Location)
-		if location != (dyn.Location{}) {
-			locations = append(locations, location)
+
+		// TODO: test this.
+		locationPathPairs := []diag.LocationPathPair{}
+		if path != nil || location != (dyn.Location{}) {
+			locationPathPairs = append(locationPathPairs, diag.LocationPathPair{
+				L: location,
+				P: path,
+			})
 		}
 
 		diag := diag.Diagnostic{
-			Severity:  severity,
-			Summary:   parsedLine.Summary,
-			Detail:    parsedLine.Detail,
-			Locations: locations,
-			Paths:     paths,
+			Severity:          severity,
+			Summary:           parsedLine.Summary,
+			Detail:            parsedLine.Detail,
+			LocationPathPairs: locationPathPairs,
 		}
 
 		diags = diags.Append(diag)

bundle/config/mutator/rewrite_workspace_prefix.go

@@ -47,12 +47,21 @@ func (m *rewriteWorkspacePrefix) Apply(ctx context.Context, b *bundle.Bundle) di
 			for path, replacePath := range paths {
 				if strings.Contains(vv, path) {
 					newPath := strings.Replace(vv, path, replacePath, 1)
+
+					locationPathPairs := []diag.LocationPathPair{}
+					for _, l := range v.Locations() {
+						locationPathPairs = append(locationPathPairs, diag.LocationPathPair{
+							L: l,
+							P: p,
+						})
+					}
+
+					// TODO: test this.
 					diags = append(diags, diag.Diagnostic{
-						Severity:  diag.Warning,
-						Summary:   fmt.Sprintf("substring %q found in %q. Please update this to %q.", path, vv, newPath),
-						Detail:    "For more information, please refer to: https://docs.databricks.com/en/release-notes/dev-tools/bundles.html#workspace-paths",
-						Locations: v.Locations(),
-						Paths:     []dyn.Path{p},
+						Severity:          diag.Warning,
+						Summary:           fmt.Sprintf("substring %q found in %q. Please update this to %q.", path, vv, newPath),
+						Detail:            "For more information, please refer to: https://docs.databricks.com/en/release-notes/dev-tools/bundles.html#workspace-paths",
+						LocationPathPairs: locationPathPairs,
 					})
 
 					// Remove the workspace prefix from the string.

bundle/config/mutator/run_as.go

@@ -30,11 +30,19 @@ func (m *setRunAs) Name() string {
 	return "SetRunAs"
 }
 
-func reportRunAsNotSupported(resourceType string, location dyn.Location, currentUser string, runAsUser string) diag.Diagnostics {
+
+// TODO: CONTINUE with transformation to pair and test this.
+func reportRunAsNotSupported(resourceType string, location dyn.Location, path dyn.Path, currentUser string, runAsUser string) diag.Diagnostics {
 	return diag.Diagnostics{{
 		Summary: fmt.Sprintf("%s do not support a setting a run_as user that is different from the owner.\n"+
 			"Current identity: %s. Run as identity: %s.\n"+
 			"See https://docs.databricks.com/dev-tools/bundles/run-as.html to learn more about the run_as property.", resourceType, currentUser, runAsUser),
+		LocationPathPairs: []diag.LocationPathPair{
+			{
+				L: location,
+
+			},
+		},
 		Locations: []dyn.Location{location},
 		Severity:  diag.Error,
 	}}

bundle/render/render_text_output.go

@@ -112,18 +112,19 @@ func renderSummaryTemplate(out io.Writer, b *bundle.Bundle, diags diag.Diagnosti
 }
 
 func renderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics) error {
+	// TODO: test this.
 	for _, d := range diags {
-		for i := range d.Locations {
+		for i := range d.LocationPathPairs {
 			if b == nil {
 				break
 			}
 
 			// Make location relative to bundle root
-			if d.Locations[i].File != "" {
-				out, err := filepath.Rel(b.BundleRootPath, d.Locations[i].File)
+			if d.LocationPathPairs[i].L.File != "" {
+				out, err := filepath.Rel(b.BundleRootPath, d.LocationPathPairs[i].L.File)
 				// if we can't relativize the path, just use path as-is
 				if err == nil {
-					d.Locations[i].File = out
+					d.LocationPathPairs[i].L.File = out
 				}
 			}
 		}

bundle/tests/issue_1828/databricks.yml

@@ -0,0 +1,33 @@
+bundle:
+  name: issue_1828
+
+variables:
+  # One entry for each of the underlying YAML (or [dyn.Kind]) types.
+  # The test confirms we can convert to and from the typed configuration without losing information.
+
+  map:
+    default:
+      foo: bar
+
+  sequence:
+    default:
+      - foo
+      - bar
+
+  string:
+    default: foo
+
+  bool:
+    default: true
+
+  int:
+    default: 42
+
+  float:
+    default: 3.14
+
+  time:
+    default: 2021-01-01
+
+  nil:
+    default:

bundle/tests/issue_1828_test.go

@@ -0,0 +1,48 @@
+package config_tests
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestIssue1828(t *testing.T) {
+	b := load(t, "./issue_1828")
+
+	if assert.Contains(t, b.Config.Variables, "map") {
+		assert.Equal(t, map[string]any{
+			"foo": "bar",
+		}, b.Config.Variables["map"].Default)
+	}
+
+	if assert.Contains(t, b.Config.Variables, "sequence") {
+		assert.Equal(t, []any{
+			"foo",
+			"bar",
+		}, b.Config.Variables["sequence"].Default)
+	}
+
+	if assert.Contains(t, b.Config.Variables, "string") {
+		assert.Equal(t, "foo", b.Config.Variables["string"].Default)
+	}
+
+	if assert.Contains(t, b.Config.Variables, "bool") {
+		assert.Equal(t, true, b.Config.Variables["bool"].Default)
+	}
+
+	if assert.Contains(t, b.Config.Variables, "int") {
+		assert.Equal(t, 42, b.Config.Variables["int"].Default)
+	}
+
+	if assert.Contains(t, b.Config.Variables, "float") {
+		assert.Equal(t, 3.14, b.Config.Variables["float"].Default)
+	}
+
+	if assert.Contains(t, b.Config.Variables, "time") {
+		assert.Equal(t, "2021-01-01", b.Config.Variables["time"].Default)
+	}
+
+	if assert.Contains(t, b.Config.Variables, "nil") {
+		assert.Equal(t, nil, b.Config.Variables["nil"].Default)
+	}
+}

libs/cmdio/render.go

@@ -394,10 +394,7 @@ func fancyJSON(v any) ([]byte, error) {
 
 const errorTemplate = `{{ "Error" | red }}: {{ .Summary }}
 {{- range $index, $element := .Paths }}
-  {{ if eq $index 0 }}at {{else}}   {{ end}}{{ $element.String | green }}
-{{- end }}
-{{- range $index, $element := .Locations }}
-  {{ if eq $index 0 }}in {{else}}   {{ end}}{{ $element.String | cyan }}
+  {{ if eq $index 0 }}at {{else}}   {{ end}}{{ $element.P.String | green }} in {{ $element.L.String | cyan }}
 {{- end }}
 {{- if .Detail }}
 
@@ -406,12 +403,11 @@ const errorTemplate = `{{ "Error" | red }}: {{ .Summary }}
 
 `
 
+// TODO: Only print "at" and "in" if both are not nil.
+// TODO: also add tests for this.
 const warningTemplate = `{{ "Warning" | yellow }}: {{ .Summary }}
 {{- range $index, $element := .Paths }}
-  {{ if eq $index 0 }}at {{else}}   {{ end}}{{ $element.String | green }}
-{{- end }}
-{{- range $index, $element := .Locations }}
-  {{ if eq $index 0 }}in {{else}}   {{ end}}{{ $element.String | cyan }}
+  {{ if eq $index 0 }}at {{else}}   {{ end}}{{ $element.P.String | green }} in {{ $element.L.String | cyan }}
 {{- end }}
 {{- if .Detail }}
 
@@ -421,11 +417,8 @@ const warningTemplate = `{{ "Warning" | yellow }}: {{ .Summary }}
 `
 
 const recommendationTemplate = `{{ "Recommendation" | blue }}: {{ .Summary }}
-{{- range $index, $element := .Paths }}
-  {{ if eq $index 0 }}at {{else}}   {{ end}}{{ $element.String | green }}
-{{- end }}
-{{- range $index, $element := .Locations }}
-  {{ if eq $index 0 }}in {{else}}   {{ end}}{{ $element.String | cyan }}
+{{- range $index, $element := .LocationPathPairs}}
+  {{ if eq $index 0 }}at {{else}}   {{ end}}{{ $element.P.String | green }} in {{ $element.L.String | cyan }}
 {{- end }}
 {{- if .Detail }}
 

libs/diag/diagnostic.go

@@ -7,6 +7,15 @@ import (
 	"github.com/databricks/cli/libs/dyn"
 )
 
+// Convenience struct for retaining a 1:1 mapping between locations and paths.
+type LocationPathPair struct {
+	// Locations are the source code locations associated with the diagnostic message.
+	L dyn.Location
+
+	// Paths are paths to the values in the configuration tree that the diagnostic is associated with.
+	P dyn.Path
+}
+
 type Diagnostic struct {
 	Severity Severity
 
@@ -18,11 +27,13 @@ type Diagnostic struct {
 	// This may be multiple lines and may be nil.
 	Detail string
 
-	// Locations are the source code locations associated with the diagnostic message.
+	// LocationPathPairs are the source code locations and paths associated with the diagnostic message.
+	// It may be empty if there are no associated locations and paths.
+	LocationPathPairs []LocationPathPair
+
 	// It may be empty if there are no associated locations.
 	Locations []dyn.Location
 
-	// Paths are paths to the values in the configuration tree that the diagnostic is associated with.
 	// It may be nil if there are no associated paths.
 	Paths []dyn.Path
 

libs/dyn/convert/normalize.go

@@ -398,6 +398,34 @@ func (n normalizeOptions) normalizeFloat(typ reflect.Type, src dyn.Value, path d
 	return dyn.NewValue(out, src.Locations()), diags
 }
 
-func (n normalizeOptions) normalizeInterface(typ reflect.Type, src dyn.Value, path dyn.Path) (dyn.Value, diag.Diagnostics) {
+func (n normalizeOptions) normalizeInterface(_ reflect.Type, src dyn.Value, path dyn.Path) (dyn.Value, diag.Diagnostics) {
+	// Deal with every [dyn.Kind] here to ensure completeness.
+	switch src.Kind() {
+	case dyn.KindMap:
+		// Fall through
+	case dyn.KindSequence:
+		// Fall through
+	case dyn.KindString:
+		// Fall through
+	case dyn.KindBool:
+		// Fall through
+	case dyn.KindInt:
+		// Fall through
+	case dyn.KindFloat:
+		// Fall through
+	case dyn.KindTime:
+		// Conversion of a time value to an interface{}.
+		// The [dyn.Value.AsAny] equivalent for this kind is the [time.Time] struct.
+		// If we convert to a typed representation and back again, we cannot distinguish
+		// a [time.Time] struct from any other struct.
+		//
+		// Therefore, we normalize the time value to a string.
+		return dyn.NewValue(src.MustTime().String(), src.Locations()), nil
+	case dyn.KindNil:
+		// Fall through
+	default:
+		return dyn.InvalidValue, diag.Errorf("unsupported kind: %s", src.Kind())
+	}
+
 	return src, nil
 }

libs/dyn/convert/normalize_test.go

@@ -858,23 +858,7 @@ func TestNormalizeAnchors(t *testing.T) {
 	}, vout.AsAny())
 }
 
-func TestNormalizeBoolToAny(t *testing.T) {
-	var typ any
-	vin := dyn.NewValue(false, []dyn.Location{{File: "file", Line: 1, Column: 1}})
-	vout, err := Normalize(&typ, vin)
-	assert.Len(t, err, 0)
-	assert.Equal(t, dyn.NewValue(false, []dyn.Location{{File: "file", Line: 1, Column: 1}}), vout)
-}
-
-func TestNormalizeIntToAny(t *testing.T) {
-	var typ any
-	vin := dyn.NewValue(10, []dyn.Location{{File: "file", Line: 1, Column: 1}})
-	vout, err := Normalize(&typ, vin)
-	assert.Len(t, err, 0)
-	assert.Equal(t, dyn.NewValue(10, []dyn.Location{{File: "file", Line: 1, Column: 1}}), vout)
-}
-
-func TestNormalizeSliceToAny(t *testing.T) {
+func TestNormalizeAnyFromSlice(t *testing.T) {
 	var typ any
 	v1 := dyn.NewValue(1, []dyn.Location{{File: "file", Line: 1, Column: 1}})
 	v2 := dyn.NewValue(2, []dyn.Location{{File: "file", Line: 1, Column: 1}})
@@ -883,3 +867,35 @@ func TestNormalizeSliceToAny(t *testing.T) {
 	assert.Len(t, err, 0)
 	assert.Equal(t, dyn.NewValue([]dyn.Value{v1, v2}, []dyn.Location{{File: "file", Line: 1, Column: 1}}), vout)
 }
+
+func TestNormalizeAnyFromString(t *testing.T) {
+	var typ any
+	vin := dyn.NewValue("string", []dyn.Location{{File: "file", Line: 1, Column: 1}})
+	vout, err := Normalize(&typ, vin)
+	assert.Len(t, err, 0)
+	assert.Equal(t, dyn.NewValue("string", []dyn.Location{{File: "file", Line: 1, Column: 1}}), vout)
+}
+
+func TestNormalizeAnyFromBool(t *testing.T) {
+	var typ any
+	vin := dyn.NewValue(false, []dyn.Location{{File: "file", Line: 1, Column: 1}})
+	vout, err := Normalize(&typ, vin)
+	assert.Len(t, err, 0)
+	assert.Equal(t, dyn.NewValue(false, []dyn.Location{{File: "file", Line: 1, Column: 1}}), vout)
+}
+
+func TestNormalizeAnyFromInt(t *testing.T) {
+	var typ any
+	vin := dyn.NewValue(10, []dyn.Location{{File: "file", Line: 1, Column: 1}})
+	vout, err := Normalize(&typ, vin)
+	assert.Len(t, err, 0)
+	assert.Equal(t, dyn.NewValue(10, []dyn.Location{{File: "file", Line: 1, Column: 1}}), vout)
+}
+
+func TestNormalizeAnyFromTime(t *testing.T) {
+	var typ any
+	vin := dyn.NewValue(dyn.MustTime("2024-08-29"), []dyn.Location{{File: "file", Line: 1, Column: 1}})
+	vout, err := Normalize(&typ, vin)
+	assert.Empty(t, err)
+	assert.Equal(t, dyn.NewValue("2024-08-29", vin.Locations()), vout)
+}