diff --git a/.nextchanges/bundles/merge-null-target-override.md b/.nextchanges/bundles/merge-null-target-override.md new file mode 100644 index 00000000000..fd545e97e3f --- /dev/null +++ b/.nextchanges/bundles/merge-null-target-override.md @@ -0,0 +1 @@ +* Fix a target override setting a field to `null` (e.g. `autoscale: null`) being ignored; the field now drops the value inherited from the base configuration instead of keeping it ([#5701](https://github.com/databricks/cli/issues/5701)). diff --git a/acceptance/bundle/override/clusters/databricks.yml b/acceptance/bundle/override/clusters/databricks.yml index 65afcca00c6..6bd8fbbb9d6 100644 --- a/acceptance/bundle/override/clusters/databricks.yml +++ b/acceptance/bundle/override/clusters/databricks.yml @@ -31,3 +31,10 @@ targets: spark_conf: "spark.executor.memory": "4g" "spark.executor.memory2": "4g" + + # Setting autoscale to null in a target drops the inherited autoscale block. + no_autoscale: + resources: + clusters: + foo: + autoscale: null diff --git a/acceptance/bundle/override/clusters/output.txt b/acceptance/bundle/override/clusters/output.txt index 1c26581d399..2366ebd52ba 100644 --- a/acceptance/bundle/override/clusters/output.txt +++ b/acceptance/bundle/override/clusters/output.txt @@ -31,3 +31,16 @@ }, "spark_version": "15.2.x-scala2.12" } + +>>> [CLI] bundle validate -o json -t no_autoscale +{ + "autoscale": null, + "autotermination_minutes": 60, + "cluster_name": "foo", + "node_type_id": "[NODE_TYPE_ID]", + "num_workers": 2, + "spark_conf": { + "spark.executor.memory": "2g" + }, + "spark_version": "13.3.x-scala2.12" +} diff --git a/acceptance/bundle/override/clusters/script b/acceptance/bundle/override/clusters/script index 4a73dd93e0b..e8386e81658 100644 --- a/acceptance/bundle/override/clusters/script +++ b/acceptance/bundle/override/clusters/script @@ -1,2 +1,3 @@ trace $CLI bundle validate -o json -t default | jq .resources.clusters.foo trace $CLI bundle validate -o json -t development | jq .resources.clusters.foo +trace $CLI bundle validate -o json -t no_autoscale | jq .resources.clusters.foo diff --git a/libs/dyn/merge/merge.go b/libs/dyn/merge/merge.go index 374450ef240..85fb9298043 100644 --- a/libs/dyn/merge/merge.go +++ b/libs/dyn/merge/merge.go @@ -10,7 +10,7 @@ import ( // // Semantics are as follows: // * Merging x with nil or nil with x always yields x. -// * Merging maps a and b means entries from map b take precedence. +// * Merging maps a and b means entries from map b take precedence. A nil entry in b clears the corresponding entry in a. // * Merging sequences a and b means concatenating them. // // Merging retains and accumulates the locations metadata associated with the values. @@ -84,6 +84,13 @@ func mergeMap(a, b dyn.Value) (dyn.Value, error) { key := pk.MustString() pv := pair.Value if ov, ok := out.Get(pk); ok { + // An explicit nil in the incoming map clears the existing value instead + // of being ignored, so a target can drop an inherited field (e.g. set + // `autoscale: null` to remove an inherited autoscale block). + if pv.Kind() == dyn.KindNil { + out.SetLoc(key, pair.Key.Locations(), pv.AppendLocationsFromValue(ov)) + continue + } // If the key already exists, merge the values. merged, err := merge(ov, pv) if err != nil { diff --git a/libs/dyn/merge/merge_test.go b/libs/dyn/merge/merge_test.go index 43e0e08c182..81cc136259d 100644 --- a/libs/dyn/merge/merge_test.go +++ b/libs/dyn/merge/merge_test.go @@ -117,6 +117,40 @@ func TestMergeMapsNil(t *testing.T) { } } +func TestMergeMapsNilEntryClearsExisting(t *testing.T) { + l1 := dyn.Location{File: "base", Line: 1, Column: 2} + base := dyn.NewValue(map[string]dyn.Value{ + "foo": dyn.NewValue("bar", []dyn.Location{l1}), + "autoscale": dyn.NewValue(map[string]dyn.Value{ + "min_workers": dyn.NewValue(1, []dyn.Location{l1}), + "max_workers": dyn.NewValue(2, []dyn.Location{l1}), + }, []dyn.Location{l1}), + }, []dyn.Location{l1}) + + l2 := dyn.Location{File: "target", Line: 3, Column: 4} + override := dyn.NewValue(map[string]dyn.Value{ + "autoscale": dyn.NewValue(nil, []dyn.Location{l2}), + }, []dyn.Location{l2}) + + out, err := Merge(base, override) + assert.NoError(t, err) + + // An explicit nil in the override clears the inherited value rather than + // leaving the base in place. + assert.Equal(t, map[string]any{ + "foo": "bar", + "autoscale": nil, + }, out.AsAny()) + assert.Equal(t, dyn.KindNil, out.Get("autoscale").Kind()) + + // The override's location wins and the base location is accumulated. + assert.Equal(t, l2, out.Get("autoscale").Location()) + assert.Equal(t, []dyn.Location{l2, l1}, out.Get("autoscale").Locations()) + + // Keys not present in the override are untouched. + assert.Equal(t, "bar", out.Get("foo").AsAny()) +} + func TestMergeMapsError(t *testing.T) { v := dyn.V(map[string]dyn.Value{ "foo": dyn.V("bar"),