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..1afa52f5a73 --- /dev/null +++ b/acceptance/bundle/validate/references/job_cluster_whole_reference/databricks.yml @@ -0,0 +1,34 @@ +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} + 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/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..fa9141d6bd4 --- /dev/null +++ b/acceptance/bundle/validate/references/job_cluster_whole_reference/output.txt @@ -0,0 +1,25 @@ + +>>> [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 + } +} + +>>> [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..cc4671fc3a4 --- /dev/null +++ b/acceptance/bundle/validate/references/job_cluster_whole_reference/script @@ -0,0 +1,6 @@ +# 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' +# 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 aeb2a6f9843..f9a97fe6308 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,56 @@ 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 + } + + var nv dyn.Value + var err error + if len(pattern) == 0 { + _, err = dyn.GetByPath(v, m.key) + if dyn.IsNoSuchKeyError(err) { + return dyn.SetByPath(v, m.key, dyn.V(m.value)) + } + 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:]) + }) + } + 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 + } + // The error is relative to v, so prefix it with the path of v itself. + 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..9f7a4187ac5 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,118 @@ 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) { + 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, tc.expectedError) + }) + } +}