From cc713fad693fe49398cfcb47af41f6461b50c9b8 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Tue, 15 Jul 2025 11:00:42 +0200 Subject: [PATCH 1/5] Configure `__apply_policy_default_values_allow_list` for task and job clusters --- bundle/deploy/terraform/tfdyn/convert_job.go | 62 +++++++++++++ .../terraform/tfdyn/convert_job_test.go | 91 +++++++++++++++++++ libs/dyn/walk.go | 23 +++++ libs/dyn/walk_test.go | 16 ++++ 4 files changed, 192 insertions(+) diff --git a/bundle/deploy/terraform/tfdyn/convert_job.go b/bundle/deploy/terraform/tfdyn/convert_job.go index bb2f8cd0fed..57171850d63 100644 --- a/bundle/deploy/terraform/tfdyn/convert_job.go +++ b/bundle/deploy/terraform/tfdyn/convert_job.go @@ -3,7 +3,9 @@ package tfdyn import ( "context" "fmt" + "slices" "sort" + "strings" "github.com/databricks/cli/bundle/internal/tf/schema" "github.com/databricks/cli/libs/dyn" @@ -12,6 +14,50 @@ import ( "github.com/databricks/databricks-sdk-go/service/jobs" ) +func patchApplyPolicyDefaultValues(_ dyn.Path, v dyn.Value) (dyn.Value, error) { + if b, ok := v.Get("apply_policy_default_values").AsBool(); !ok || !b { + return v, nil + } + + // The field "apply_policy_default_values" is set. + // We need to collect the list of fields that are set explicitly + // and pass it to Terraform. This enables Terraform to clear + // server-side defaults from the update request, which in turn + // allows the backend to re-apply the policy defaults. + // + // For more details, see: https://github.com/databricks/terraform-provider-databricks/pull/4834 + // + paths := dyn.CollectLeafPaths(v) + + // If any of the map fields are set, always include them entirely instead of traversing the map. + for _, mapField := range []string{ + "custom_tags", + "spark_conf", + "spark_env_vars", + } { + if _, ok := v.Get(mapField).AsMap(); ok { + // Remove all paths that start with the map field. + paths = slices.DeleteFunc(paths, func(p string) bool { + return strings.HasPrefix(p, mapField+".") + }) + // Add the map field to the paths. + paths = append(paths, mapField) + } + } + + sort.Strings(paths) + valList := make([]dyn.Value, len(paths)) + for i, s := range paths { + valList[i] = dyn.V(s) + } + v, err := dyn.Set(v, "__apply_policy_default_values_allow_list", dyn.V(valList)) + if err != nil { + return dyn.InvalidValue, err + } + + return v, nil +} + func convertJobResource(ctx context.Context, vin dyn.Value) (dyn.Value, error) { // Normalize the input value to the underlying job schema. // This removes superfluous keys and adapts the input to the expected schema. @@ -101,6 +147,22 @@ func convertJobResource(ctx context.Context, vin dyn.Value) (dyn.Value, error) { log.Debugf(ctx, "job normalization diagnostic: %s", diag.Summary) } + // Apply __apply_policy_default_values_allow_list for tasks + vout, err = dyn.Map(vout, "task", dyn.Foreach(func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { + return dyn.Map(v, "new_cluster", patchApplyPolicyDefaultValues) + })) + if err != nil { + return dyn.InvalidValue, err + } + + // Apply __apply_policy_default_values_allow_list for job clusters + vout, err = dyn.Map(vout, "job_cluster", dyn.Foreach(func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { + return dyn.Map(v, "new_cluster", patchApplyPolicyDefaultValues) + })) + if err != nil { + return dyn.InvalidValue, err + } + return vout, err } diff --git a/bundle/deploy/terraform/tfdyn/convert_job_test.go b/bundle/deploy/terraform/tfdyn/convert_job_test.go index 8f4cfc2fa83..40ca3a4fcfb 100644 --- a/bundle/deploy/terraform/tfdyn/convert_job_test.go +++ b/bundle/deploy/terraform/tfdyn/convert_job_test.go @@ -149,3 +149,94 @@ func TestConvertJob(t *testing.T) { }, }, out.Permissions["job_my_job"]) } + +func TestConvertJobApplyPolicyDefaultValues(t *testing.T) { + src := resources.Job{ + JobSettings: jobs.JobSettings{ + Name: "my job", + JobClusters: []jobs.JobCluster{ + { + JobClusterKey: "key", + NewCluster: compute.ClusterSpec{ + ApplyPolicyDefaultValues: true, + PolicyId: "policy_id", + GcpAttributes: &compute.GcpAttributes{ + Availability: "SPOT", + LocalSsdCount: 2, + }, + }, + }, + { + JobClusterKey: "key2", + NewCluster: compute.ClusterSpec{ + ApplyPolicyDefaultValues: true, + PolicyId: "policy_id2", + CustomTags: map[string]string{ + "key": "value", + }, + SparkConf: map[string]string{ + "key": "value", + }, + SparkEnvVars: map[string]string{ + "key": "value", + }, + }, + }, + }, + }, + } + + vin, err := convert.FromTyped(src, dyn.NilValue) + require.NoError(t, err) + + ctx := context.Background() + out := schema.NewResources() + err = jobConverter{}.Convert(ctx, "my_job", vin, out) + require.NoError(t, err) + + assert.Equal(t, map[string]any{ + "name": "my job", + "job_cluster": []any{ + map[string]any{ + "job_cluster_key": "key", + "new_cluster": map[string]any{ + "__apply_policy_default_values_allow_list": []any{ + "apply_policy_default_values", + "gcp_attributes.availability", + "gcp_attributes.local_ssd_count", + "policy_id", + }, + "apply_policy_default_values": true, + "policy_id": "policy_id", + "gcp_attributes": map[string]any{ + "availability": "SPOT", + "local_ssd_count": int64(2), + }, + }, + }, + map[string]any{ + "job_cluster_key": "key2", + "new_cluster": map[string]any{ + "__apply_policy_default_values_allow_list": []any{ + "apply_policy_default_values", + "custom_tags", + "policy_id", + "spark_conf", + "spark_env_vars", + }, + "apply_policy_default_values": true, + "policy_id": "policy_id2", + "custom_tags": map[string]any{ + "key": "value", + }, + "spark_conf": map[string]any{ + "key": "value", + }, + "spark_env_vars": map[string]any{ + "key": "value", + }, + }, + }, + }, + }, out.Job["my_job"]) +} diff --git a/libs/dyn/walk.go b/libs/dyn/walk.go index 9d0a99356da..9fdc1878c9f 100644 --- a/libs/dyn/walk.go +++ b/libs/dyn/walk.go @@ -66,3 +66,26 @@ func walk(v Value, p Path, fn func(p Path, v Value) (Value, error)) (Value, erro return v, nil } + +// CollectLeafPaths traverses the value and returns all paths (as dot notation strings) to leaf nodes (non-map, non-sequence). +// The return value is not ordered. +func CollectLeafPaths(v Value) []string { + var paths []string + + Walk(v, func(p Path, v Value) (Value, error) { + if len(p) == 0 { + return v, nil + } + + switch v.Kind() { + case KindMap, KindSequence: + // Ignore internal nodes. + default: + paths = append(paths, p.String()) + } + + return v, nil + }) + + return paths +} diff --git a/libs/dyn/walk_test.go b/libs/dyn/walk_test.go index f7222b0a5b7..7ef8415080e 100644 --- a/libs/dyn/walk_test.go +++ b/libs/dyn/walk_test.go @@ -4,6 +4,7 @@ import ( "errors" "testing" + "github.com/databricks/cli/libs/dyn" . "github.com/databricks/cli/libs/dyn" assert "github.com/databricks/cli/libs/dyn/dynassert" "github.com/stretchr/testify/require" @@ -252,3 +253,18 @@ func TestWalkSequenceError(t *testing.T) { assert.Equal(t, MustPathFromString(".[1]"), tracker.calls[2].path) assert.Equal(t, V("bar"), tracker.calls[2].value) } + +func TestCollectLeafPaths(t *testing.T) { + v := V(map[string]dyn.Value{ + "a": dyn.V(1), + "b": dyn.V(map[string]dyn.Value{ + "c": dyn.V(2), + "d": dyn.V(map[string]dyn.Value{ + "e": dyn.V(3), + }), + }), + "f": dyn.V([]dyn.Value{dyn.V(4), dyn.V(5)}), + }) + paths := CollectLeafPaths(v) + assert.ElementsMatch(t, []string{"a", "b.c", "b.d.e", "f[0]", "f[1]"}, paths) +} From 585f692dc32219f59c08967173f67dfa0afb3911 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Tue, 15 Jul 2025 11:29:18 +0200 Subject: [PATCH 2/5] Fix lint --- libs/dyn/walk.go | 2 +- libs/dyn/walk_test.go | 15 +++++++-------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/libs/dyn/walk.go b/libs/dyn/walk.go index 9fdc1878c9f..3f705caf960 100644 --- a/libs/dyn/walk.go +++ b/libs/dyn/walk.go @@ -72,7 +72,7 @@ func walk(v Value, p Path, fn func(p Path, v Value) (Value, error)) (Value, erro func CollectLeafPaths(v Value) []string { var paths []string - Walk(v, func(p Path, v Value) (Value, error) { + Walk(v, func(p Path, v Value) (Value, error) { //nolint:errcheck if len(p) == 0 { return v, nil } diff --git a/libs/dyn/walk_test.go b/libs/dyn/walk_test.go index 7ef8415080e..2ccd8695cbe 100644 --- a/libs/dyn/walk_test.go +++ b/libs/dyn/walk_test.go @@ -4,7 +4,6 @@ import ( "errors" "testing" - "github.com/databricks/cli/libs/dyn" . "github.com/databricks/cli/libs/dyn" assert "github.com/databricks/cli/libs/dyn/dynassert" "github.com/stretchr/testify/require" @@ -255,15 +254,15 @@ func TestWalkSequenceError(t *testing.T) { } func TestCollectLeafPaths(t *testing.T) { - v := V(map[string]dyn.Value{ - "a": dyn.V(1), - "b": dyn.V(map[string]dyn.Value{ - "c": dyn.V(2), - "d": dyn.V(map[string]dyn.Value{ - "e": dyn.V(3), + v := V(map[string]Value{ + "a": V(1), + "b": V(map[string]Value{ + "c": V(2), + "d": V(map[string]Value{ + "e": V(3), }), }), - "f": dyn.V([]dyn.Value{dyn.V(4), dyn.V(5)}), + "f": V([]Value{V(4), V(5)}), }) paths := CollectLeafPaths(v) assert.ElementsMatch(t, []string{"a", "b.c", "b.d.e", "f[0]", "f[1]"}, paths) From 136790d22fc17ec6fe0cbf91505364a02abc40f5 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Tue, 15 Jul 2025 16:24:12 +0200 Subject: [PATCH 3/5] Only configure field if policy_id is also set --- bundle/deploy/terraform/tfdyn/convert_job.go | 6 ++++++ bundle/deploy/terraform/tfdyn/convert_job_test.go | 14 ++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/bundle/deploy/terraform/tfdyn/convert_job.go b/bundle/deploy/terraform/tfdyn/convert_job.go index 57171850d63..719a2bb0f61 100644 --- a/bundle/deploy/terraform/tfdyn/convert_job.go +++ b/bundle/deploy/terraform/tfdyn/convert_job.go @@ -15,10 +15,16 @@ import ( ) func patchApplyPolicyDefaultValues(_ dyn.Path, v dyn.Value) (dyn.Value, error) { + // If the field "apply_policy_default_values" is not set, do nothing. if b, ok := v.Get("apply_policy_default_values").AsBool(); !ok || !b { return v, nil } + // If the field "policy_id" is not set, do nothing. + if _, ok := v.Get("policy_id").AsString(); !ok { + return v, nil + } + // The field "apply_policy_default_values" is set. // We need to collect the list of fields that are set explicitly // and pass it to Terraform. This enables Terraform to clear diff --git a/bundle/deploy/terraform/tfdyn/convert_job_test.go b/bundle/deploy/terraform/tfdyn/convert_job_test.go index 40ca3a4fcfb..d1e1bae931a 100644 --- a/bundle/deploy/terraform/tfdyn/convert_job_test.go +++ b/bundle/deploy/terraform/tfdyn/convert_job_test.go @@ -182,6 +182,13 @@ func TestConvertJobApplyPolicyDefaultValues(t *testing.T) { }, }, }, + { + JobClusterKey: "key3", + NewCluster: compute.ClusterSpec{ + ApplyPolicyDefaultValues: true, + SparkVersion: "16.4.x-scala2.12", + }, + }, }, }, } @@ -237,6 +244,13 @@ func TestConvertJobApplyPolicyDefaultValues(t *testing.T) { }, }, }, + map[string]any{ + "job_cluster_key": "key3", + "new_cluster": map[string]any{ + "apply_policy_default_values": true, + "spark_version": "16.4.x-scala2.12", + }, + }, }, }, out.Job["my_job"]) } From 01e5574290655fdfcc2d02770bbb20d796affa23 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Tue, 15 Jul 2025 16:29:18 +0200 Subject: [PATCH 4/5] Update NEXT_CHANGELOG --- NEXT_CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index f789869b3e7..ce00d1437e3 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -10,4 +10,6 @@ ### Bundles + * Jobs that use cluster policy default values for their cluster configuration now correctly update those defaults on every deployment ([#3255](https://github.com/databricks/cli/pull/3255)). + ### API Changes From 2ea1643fecf99e70a6949cd8b94f36fe5bb44333 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 16 Jul 2025 12:33:27 +0200 Subject: [PATCH 5/5] Include sequences in the list of fields to pass through --- bundle/deploy/terraform/tfdyn/convert_job.go | 16 ++++++---- .../terraform/tfdyn/convert_job_test.go | 32 +++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/bundle/deploy/terraform/tfdyn/convert_job.go b/bundle/deploy/terraform/tfdyn/convert_job.go index 719a2bb0f61..e38ea36d6b2 100644 --- a/bundle/deploy/terraform/tfdyn/convert_job.go +++ b/bundle/deploy/terraform/tfdyn/convert_job.go @@ -35,19 +35,21 @@ func patchApplyPolicyDefaultValues(_ dyn.Path, v dyn.Value) (dyn.Value, error) { // paths := dyn.CollectLeafPaths(v) - // If any of the map fields are set, always include them entirely instead of traversing the map. - for _, mapField := range []string{ + // If any of the map or sequence fields are set, always include them entirely instead of traversing the them. + for _, field := range []string{ "custom_tags", + "init_scripts", "spark_conf", "spark_env_vars", + "ssh_public_keys", } { - if _, ok := v.Get(mapField).AsMap(); ok { - // Remove all paths that start with the map field. + if vv := v.Get(field); vv.IsValid() { + // Remove all paths that start with the field. paths = slices.DeleteFunc(paths, func(p string) bool { - return strings.HasPrefix(p, mapField+".") + return strings.HasPrefix(p, field+".") || strings.HasPrefix(p, field+"[") }) - // Add the map field to the paths. - paths = append(paths, mapField) + // Add the field to the paths. + paths = append(paths, field) } } diff --git a/bundle/deploy/terraform/tfdyn/convert_job_test.go b/bundle/deploy/terraform/tfdyn/convert_job_test.go index d1e1bae931a..a7c506d592b 100644 --- a/bundle/deploy/terraform/tfdyn/convert_job_test.go +++ b/bundle/deploy/terraform/tfdyn/convert_job_test.go @@ -174,12 +174,27 @@ func TestConvertJobApplyPolicyDefaultValues(t *testing.T) { CustomTags: map[string]string{ "key": "value", }, + InitScripts: []compute.InitScriptInfo{ + { + Workspace: &compute.WorkspaceStorageInfo{ + Destination: "/Workspace/path/to/init_script1", + }, + }, + { + Workspace: &compute.WorkspaceStorageInfo{ + Destination: "/Workspace/path/to/init_script2", + }, + }, + }, SparkConf: map[string]string{ "key": "value", }, SparkEnvVars: map[string]string{ "key": "value", }, + SshPublicKeys: []string{ + "ssh-rsa 1234", + }, }, }, { @@ -227,21 +242,38 @@ func TestConvertJobApplyPolicyDefaultValues(t *testing.T) { "__apply_policy_default_values_allow_list": []any{ "apply_policy_default_values", "custom_tags", + "init_scripts", "policy_id", "spark_conf", "spark_env_vars", + "ssh_public_keys", }, "apply_policy_default_values": true, "policy_id": "policy_id2", "custom_tags": map[string]any{ "key": "value", }, + "init_scripts": []any{ + map[string]any{ + "workspace": map[string]any{ + "destination": "/Workspace/path/to/init_script1", + }, + }, + map[string]any{ + "workspace": map[string]any{ + "destination": "/Workspace/path/to/init_script2", + }, + }, + }, "spark_conf": map[string]any{ "key": "value", }, "spark_env_vars": map[string]any{ "key": "value", }, + "ssh_public_keys": []any{ + "ssh-rsa 1234", + }, }, }, map[string]any{