From 5fa9cd3093a9f5e96afd1360e982b39156aea2a6 Mon Sep 17 00:00:00 2001 From: "pieter.noordhuis" Date: Wed, 4 Nov 2026 09:14:45 +0000 Subject: [PATCH 1/3] Skip whole-block references above the key when setting defaults SetDefault only skipped a pure ${...} reference where the pattern ends. A reference higher up, such as new_cluster: ${resources.jobs.base.job_clusters[0].new_cluster}, still failed with "expected a map to index ...workload_type, found string", because fixed path components do not treat a non-map as a no-match. Walk the pattern one component at a time and skip a pure reference at any level; the referenced block gets the default itself. Co-authored-by: Isaac --- .../defaults-whole-reference-parent.md | 1 + .../databricks.yml | 19 ++++ .../job_cluster_whole_reference/out.test.toml | 2 + .../job_cluster_whole_reference/output.txt | 17 ++++ .../job_cluster_whole_reference/script | 4 + bundle/set_default.go | 65 ++++++++++--- bundle/set_default_test.go | 95 +++++++++++++++++++ 7 files changed, 189 insertions(+), 14 deletions(-) create mode 100644 .nextchanges/bundles/defaults-whole-reference-parent.md create mode 100644 acceptance/bundle/validate/references/job_cluster_whole_reference/databricks.yml create mode 100644 acceptance/bundle/validate/references/job_cluster_whole_reference/out.test.toml create mode 100644 acceptance/bundle/validate/references/job_cluster_whole_reference/output.txt create mode 100644 acceptance/bundle/validate/references/job_cluster_whole_reference/script diff --git a/.nextchanges/bundles/defaults-whole-reference-parent.md b/.nextchanges/bundles/defaults-whole-reference-parent.md new file mode 100644 index 00000000000..95b6f5c8afe --- /dev/null +++ b/.nextchanges/bundles/defaults-whole-reference-parent.md @@ -0,0 +1 @@ +* Fix `bundle validate` failing with "expected a map to index" when a job cluster `new_cluster` or a cluster `workload_type` is a reference to a whole block, such as `${resources.jobs.base.job_clusters[0].new_cluster}`. ([#7338](https://github.com/databricks/cli/pull/7338)) diff --git a/acceptance/bundle/validate/references/job_cluster_whole_reference/databricks.yml b/acceptance/bundle/validate/references/job_cluster_whole_reference/databricks.yml new file mode 100644 index 00000000000..6da90786a8c --- /dev/null +++ b/acceptance/bundle/validate/references/job_cluster_whole_reference/databricks.yml @@ -0,0 +1,19 @@ +bundle: + name: test-bundle + +resources: + jobs: + base: + job_clusters: + - job_cluster_key: main + new_cluster: + spark_version: 15.4.x-scala2.12 + node_type_id: i3.xlarge + num_workers: 1 + workload_type: + clients: + jobs: false + viaref: + job_clusters: + - job_cluster_key: main + new_cluster: ${resources.jobs.base.job_clusters[0].new_cluster} diff --git a/acceptance/bundle/validate/references/job_cluster_whole_reference/out.test.toml b/acceptance/bundle/validate/references/job_cluster_whole_reference/out.test.toml new file mode 100644 index 00000000000..a927a5fbc06 --- /dev/null +++ b/acceptance/bundle/validate/references/job_cluster_whole_reference/out.test.toml @@ -0,0 +1,2 @@ +Cloud = false +EnvMatrix.DMS = ["", "true"] diff --git a/acceptance/bundle/validate/references/job_cluster_whole_reference/output.txt b/acceptance/bundle/validate/references/job_cluster_whole_reference/output.txt new file mode 100644 index 00000000000..90c5c52decc --- /dev/null +++ b/acceptance/bundle/validate/references/job_cluster_whole_reference/output.txt @@ -0,0 +1,17 @@ + +>>> [CLI] bundle validate +Name: test-bundle +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default + +Validation OK! + +>>> [CLI] bundle plan -o json +{ + "clients": { + "jobs": false, + "notebooks": true + } +} diff --git a/acceptance/bundle/validate/references/job_cluster_whole_reference/script b/acceptance/bundle/validate/references/job_cluster_whole_reference/script new file mode 100644 index 00000000000..51389884374 --- /dev/null +++ b/acceptance/bundle/validate/references/job_cluster_whole_reference/script @@ -0,0 +1,4 @@ +# The new_cluster of viaref is a reference to the whole new_cluster of another job. +# The defaults (e.g. workload_type.clients.notebooks) are not written into the reference; the referenced new_cluster gets them. +trace $CLI bundle validate +trace $CLI bundle plan -o json | jq '.plan."resources.jobs.viaref".new_state.value.job_clusters[0].new_cluster.workload_type' diff --git a/bundle/set_default.go b/bundle/set_default.go index aeb2a6f9843..6f00fe23ec6 100644 --- a/bundle/set_default.go +++ b/bundle/set_default.go @@ -2,6 +2,7 @@ package bundle import ( "context" + "errors" "fmt" "github.com/databricks/cli/libs/diag" @@ -30,20 +31,7 @@ func (m *setDefault) Name() string { func (m *setDefault) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { - return dyn.MapByPattern(v, m.pattern, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { - // A default cannot be written into a pure reference to a whole block - // (e.g. "${resources.jobs.base.schedule}"), so leave it to the referenced value. - if s, ok := v.AsString(); ok && dynvar.IsPureVariableReference(s) { - return v, nil - } - _, err := dyn.GetByPath(v, m.key) - switch { - case dyn.IsNoSuchKeyError(err): - return dyn.SetByPath(v, m.key, dyn.V(m.value)) - default: - return v, err - } - }) + return m.apply(v, dyn.EmptyPath, m.pattern) }) if err != nil { return diag.FromErr(err) @@ -52,6 +40,55 @@ func (m *setDefault) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { return nil } +// setDefaultPathError prefixes an error with the full path of the value it was +// raised on, because apply visits sub-values and dyn errors only carry the path +// relative to the value they were raised on. +type setDefaultPathError struct { + path dyn.Path + err error +} + +func (e setDefaultPathError) Error() string { + return fmt.Sprintf("%s: %s", e.path, e.err) +} + +// Unwrap lets wildcard visits and callers still classify the underlying dyn error. +func (e setDefaultPathError) Unwrap() error { + return e.err +} + +// apply walks the pattern one component at a time so that a pure reference +// anywhere along the way is skipped, not only where the pattern ends. +func (m *setDefault) apply(v dyn.Value, prefix dyn.Path, pattern dyn.Pattern) (dyn.Value, error) { + // A default cannot be written into a pure reference to a whole block + // (e.g. "${resources.jobs.base.schedule}"), so leave it to the referenced value. + if s, ok := v.AsString(); ok && dynvar.IsPureVariableReference(s) { + return v, nil + } + + if len(pattern) == 0 { + _, err := dyn.GetByPath(v, m.key) + switch { + case dyn.IsNoSuchKeyError(err): + return dyn.SetByPath(v, m.key, dyn.V(m.value)) + default: + return v, err + } + } + + nv, err := dyn.MapByPattern(v, pattern[:1], func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + return m.apply(v, prefix.Append(p...), pattern[1:]) + }) + if err == nil || len(prefix) == 0 { + return nv, err + } + // Errors from deeper levels already carry their full path. + if _, ok := errors.AsType[setDefaultPathError](err); ok { + return nv, err + } + return nv, setDefaultPathError{path: prefix, err: err} +} + func SetDefault(ctx context.Context, b *Bundle, pattern string, value any) { pat, err := dyn.NewPatternFromString(pattern) if err != nil { diff --git a/bundle/set_default_test.go b/bundle/set_default_test.go index 044906f14dd..209976d52c4 100644 --- a/bundle/set_default_test.go +++ b/bundle/set_default_test.go @@ -5,6 +5,7 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/dynvar" "github.com/databricks/cli/libs/logdiag" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -37,3 +38,97 @@ resources: require.NoError(t, err) assert.Equal(t, "${resources.jobs.base.schedule}", viaref.MustString()) } + +func TestSetDefaultSkipsPureReferenceAboveKey(t *testing.T) { + tests := []struct { + name string + config string + pattern string + set string + ref string + }{ + { + name: "job_cluster_new_cluster", + config: ` +resources: + jobs: + base: + job_clusters: + - job_cluster_key: main + new_cluster: + workload_type: + clients: + jobs: false + viaref: + job_clusters: + - job_cluster_key: main + new_cluster: ${resources.jobs.base.job_clusters[0].new_cluster} +`, + pattern: "resources.jobs.*.job_clusters[*].new_cluster.workload_type.clients.notebooks", + set: "resources.jobs.base.job_clusters[0].new_cluster.workload_type.clients.notebooks", + ref: "resources.jobs.viaref.job_clusters[0].new_cluster", + }, + { + name: "cluster_workload_type", + config: ` +resources: + clusters: + base: + workload_type: + clients: + jobs: false + viaref: + workload_type: ${resources.clusters.base.workload_type} +`, + pattern: "resources.clusters.*.workload_type.clients.notebooks", + set: "resources.clusters.base.workload_type.clients.notebooks", + ref: "resources.clusters.viaref.workload_type", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + root, diags := config.LoadFromBytes("databricks.yml", []byte(tc.config)) + require.Empty(t, diags) + b := &Bundle{Config: *root} + + ctx := logdiag.InitContext(t.Context()) + logdiag.SetCollect(ctx, true) + SetDefault(ctx, b, tc.pattern, true) + require.Empty(t, logdiag.FlushCollected(ctx)) + + v := b.Config.Value() + set, err := dyn.GetByPath(v, dyn.MustPathFromString(tc.set)) + require.NoError(t, err) + assert.True(t, set.MustBool()) + + ref, err := dyn.GetByPath(v, dyn.MustPathFromString(tc.ref)) + require.NoError(t, err) + assert.True(t, dynvar.IsPureVariableReference(ref.MustString())) + }) + } +} + +func TestSetDefaultErrorHasFullPath(t *testing.T) { + // Built directly because loading the config would drop a wrong-kind workload_type. + v := dyn.V(map[string]dyn.Value{ + "resources": dyn.V(map[string]dyn.Value{ + "jobs": dyn.V(map[string]dyn.Value{ + "job": dyn.V(map[string]dyn.Value{ + "job_clusters": dyn.V([]dyn.Value{ + dyn.V(map[string]dyn.Value{ + "new_cluster": dyn.V(map[string]dyn.Value{ + "workload_type": dyn.V("not a map"), + }), + }), + }), + }), + }), + }), + }) + + pattern := dyn.MustPatternFromString("resources.jobs.*.job_clusters[*].new_cluster.workload_type.clients") + m := SetDefaultMutator(pattern, "notebooks", true).(*setDefault) + _, err := m.apply(v, dyn.EmptyPath, m.pattern) + assert.EqualError(t, err, `resources.jobs.job.job_clusters[0].new_cluster.workload_type: expected a map to index "clients", found string`) +} From afbd4435863c25b8e217498242a12598d15a250a Mon Sep 17 00:00:00 2001 From: "pieter.noordhuis" Date: Wed, 4 Nov 2026 16:03:41 +0000 Subject: [PATCH 2/3] Report the full path for default errors at the end of the pattern An error from the key lookup at the end of the pattern is relative to the matched value, but it was prefixed with the parent's path, so the last pattern component was missing from the message. Prefix it with the path of the matched value instead. Also cover the cluster workload_type whole-block reference in the acceptance test. Co-authored-by: Isaac --- .../databricks.yml | 15 ++++++ .../job_cluster_whole_reference/output.txt | 8 +++ .../job_cluster_whole_reference/script | 2 + bundle/set_default.go | 19 +++---- bundle/set_default_test.go | 53 +++++++++++++------ 5 files changed, 72 insertions(+), 25 deletions(-) diff --git a/acceptance/bundle/validate/references/job_cluster_whole_reference/databricks.yml b/acceptance/bundle/validate/references/job_cluster_whole_reference/databricks.yml index 6da90786a8c..1afa52f5a73 100644 --- a/acceptance/bundle/validate/references/job_cluster_whole_reference/databricks.yml +++ b/acceptance/bundle/validate/references/job_cluster_whole_reference/databricks.yml @@ -17,3 +17,18 @@ resources: job_clusters: - job_cluster_key: main new_cluster: ${resources.jobs.base.job_clusters[0].new_cluster} + clusters: + base_cluster: + cluster_name: base_cluster + spark_version: 15.4.x-scala2.12 + node_type_id: i3.xlarge + num_workers: 1 + workload_type: + clients: + jobs: false + viaref_cluster: + cluster_name: viaref_cluster + spark_version: 15.4.x-scala2.12 + node_type_id: i3.xlarge + num_workers: 1 + workload_type: ${resources.clusters.base_cluster.workload_type} diff --git a/acceptance/bundle/validate/references/job_cluster_whole_reference/output.txt b/acceptance/bundle/validate/references/job_cluster_whole_reference/output.txt index 90c5c52decc..fa9141d6bd4 100644 --- a/acceptance/bundle/validate/references/job_cluster_whole_reference/output.txt +++ b/acceptance/bundle/validate/references/job_cluster_whole_reference/output.txt @@ -15,3 +15,11 @@ Validation OK! "notebooks": true } } + +>>> [CLI] bundle plan -o json +{ + "clients": { + "jobs": false, + "notebooks": true + } +} diff --git a/acceptance/bundle/validate/references/job_cluster_whole_reference/script b/acceptance/bundle/validate/references/job_cluster_whole_reference/script index 51389884374..cc4671fc3a4 100644 --- a/acceptance/bundle/validate/references/job_cluster_whole_reference/script +++ b/acceptance/bundle/validate/references/job_cluster_whole_reference/script @@ -2,3 +2,5 @@ # The defaults (e.g. workload_type.clients.notebooks) are not written into the reference; the referenced new_cluster gets them. trace $CLI bundle validate trace $CLI bundle plan -o json | jq '.plan."resources.jobs.viaref".new_state.value.job_clusters[0].new_cluster.workload_type' +# The same holds for a cluster whose workload_type is a reference to the whole workload_type of another cluster. +trace $CLI bundle plan -o json | jq '.plan."resources.clusters.viaref_cluster".new_state.value.workload_type' diff --git a/bundle/set_default.go b/bundle/set_default.go index 6f00fe23ec6..f9a97fe6308 100644 --- a/bundle/set_default.go +++ b/bundle/set_default.go @@ -66,19 +66,19 @@ func (m *setDefault) apply(v dyn.Value, prefix dyn.Path, pattern dyn.Pattern) (d return v, nil } + var nv dyn.Value + var err error if len(pattern) == 0 { - _, err := dyn.GetByPath(v, m.key) - switch { - case dyn.IsNoSuchKeyError(err): + _, err = dyn.GetByPath(v, m.key) + if dyn.IsNoSuchKeyError(err) { return dyn.SetByPath(v, m.key, dyn.V(m.value)) - default: - return v, err } + nv = v + } else { + nv, err = dyn.MapByPattern(v, pattern[:1], func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + return m.apply(v, prefix.Append(p...), pattern[1:]) + }) } - - nv, err := dyn.MapByPattern(v, pattern[:1], func(p dyn.Path, v dyn.Value) (dyn.Value, error) { - return m.apply(v, prefix.Append(p...), pattern[1:]) - }) if err == nil || len(prefix) == 0 { return nv, err } @@ -86,6 +86,7 @@ func (m *setDefault) apply(v dyn.Value, prefix dyn.Path, pattern dyn.Pattern) (d if _, ok := errors.AsType[setDefaultPathError](err); ok { return nv, err } + // The error is relative to v, so prefix it with the path of v itself. return nv, setDefaultPathError{path: prefix, err: err} } diff --git a/bundle/set_default_test.go b/bundle/set_default_test.go index 209976d52c4..9f7a4187ac5 100644 --- a/bundle/set_default_test.go +++ b/bundle/set_default_test.go @@ -110,25 +110,46 @@ resources: } func TestSetDefaultErrorHasFullPath(t *testing.T) { - // Built directly because loading the config would drop a wrong-kind workload_type. - v := dyn.V(map[string]dyn.Value{ - "resources": dyn.V(map[string]dyn.Value{ - "jobs": dyn.V(map[string]dyn.Value{ - "job": dyn.V(map[string]dyn.Value{ - "job_clusters": dyn.V([]dyn.Value{ - dyn.V(map[string]dyn.Value{ - "new_cluster": dyn.V(map[string]dyn.Value{ - "workload_type": dyn.V("not a map"), + tests := []struct { + name string + workloadType dyn.Value + expectedError string + }{ + { + name: "mismatch_inside_pattern", + workloadType: dyn.V("not a map"), + expectedError: `resources.jobs.job.job_clusters[0].new_cluster.workload_type: expected a map to index "clients", found string`, + }, + { + name: "mismatch_at_pattern_end", + workloadType: dyn.V(map[string]dyn.Value{"clients": dyn.V("x")}), + expectedError: `resources.jobs.job.job_clusters[0].new_cluster.workload_type.clients: expected a map to index "notebooks", found string`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Built directly because loading the config would drop a wrong-kind workload_type. + v := dyn.V(map[string]dyn.Value{ + "resources": dyn.V(map[string]dyn.Value{ + "jobs": dyn.V(map[string]dyn.Value{ + "job": dyn.V(map[string]dyn.Value{ + "job_clusters": dyn.V([]dyn.Value{ + dyn.V(map[string]dyn.Value{ + "new_cluster": dyn.V(map[string]dyn.Value{ + "workload_type": tc.workloadType, + }), + }), }), }), }), }), - }), - }), - }) + }) - pattern := dyn.MustPatternFromString("resources.jobs.*.job_clusters[*].new_cluster.workload_type.clients") - m := SetDefaultMutator(pattern, "notebooks", true).(*setDefault) - _, err := m.apply(v, dyn.EmptyPath, m.pattern) - assert.EqualError(t, err, `resources.jobs.job.job_clusters[0].new_cluster.workload_type: expected a map to index "clients", found string`) + pattern := dyn.MustPatternFromString("resources.jobs.*.job_clusters[*].new_cluster.workload_type.clients") + m := SetDefaultMutator(pattern, "notebooks", true).(*setDefault) + _, err := m.apply(v, dyn.EmptyPath, m.pattern) + assert.EqualError(t, err, tc.expectedError) + }) + } } From af29e37cfe7662fc37c7a084a464938567cfc25e Mon Sep 17 00:00:00 2001 From: "pieter.noordhuis" Date: Thu, 5 Nov 2026 09:15:03 +0000 Subject: [PATCH 3/3] Drop the changelog entry Co-authored-by: Isaac --- .nextchanges/bundles/defaults-whole-reference-parent.md | 1 - 1 file changed, 1 deletion(-) delete mode 100644 .nextchanges/bundles/defaults-whole-reference-parent.md diff --git a/.nextchanges/bundles/defaults-whole-reference-parent.md b/.nextchanges/bundles/defaults-whole-reference-parent.md deleted file mode 100644 index 95b6f5c8afe..00000000000 --- a/.nextchanges/bundles/defaults-whole-reference-parent.md +++ /dev/null @@ -1 +0,0 @@ -* Fix `bundle validate` failing with "expected a map to index" when a job cluster `new_cluster` or a cluster `workload_type` is a reference to a whole block, such as `${resources.jobs.base.job_clusters[0].new_cluster}`. ([#7338](https://github.com/databricks/cli/pull/7338))