From bd48d1f28c0a3a9bfb8c96ce8df74767e57bf8ad Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 30 Oct 2026 15:13:14 +0100 Subject: [PATCH 1/5] Remove the value a null overrides in includes and targets A null in an included file removes the value it lands on from databricks.yml and earlier includes, also a whole resource or section. A null in a target removes the value from the configuration when the target is selected: a resource, a field, a map entry, a list, and a field of a task, job cluster, job parameter or pipeline cluster matched by its key. A null that has nothing to remove adds nothing, without a warning; an empty targets. or variables. still declares the target or variable. Nulls do not survive the typed round trip between mutators, so the nulls of the target sections are kept on the side until the target is merged. Co-authored-by: Isaac --- .../empty_resources/empty_def/output.txt | 112 ++------- .../validate/empty_resources/null/output.txt | 112 ++------- .../bundle/validate/empty_tasks/output.txt | 3 +- .../validate/nulls/include_removes/output.txt | 48 +--- .../nulls/include_target_overrides/output.txt | 30 +-- .../nulls/resources_container/output.txt | 10 +- .../validate/nulls/resources_container/script | 2 +- .../nulls/resources_container/test.toml | 1 - .../nulls/root_without_base/output.txt | 12 +- .../output.txt | 8 - .../schedule_default_materialization/script | 2 +- .../test.toml | 1 - .../nulls/target_keyed_slices/output.txt | 30 +-- .../target_removes_job_fields/output.txt | 37 +-- .../nulls/target_removes_resource/output.txt | 14 +- .../presets_name_prefix_dev/output.txt | 4 - .../bundle/validate/required/output.txt | 6 +- .../mutator/resourcemutator/run_as_test.go | 2 +- bundle/config/root.go | 224 +++++++++++++++++- libs/dyn/convert/normalize.go | 18 ++ libs/dyn/convert/normalize_test.go | 37 +++ libs/dyn/drop_nulls.go | 28 +++ libs/dyn/drop_nulls_test.go | 35 +++ libs/dyn/merge/elements_by_key.go | 10 + libs/dyn/merge/elements_by_key_test.go | 28 +++ libs/dyn/merge/merge.go | 72 +++++- libs/dyn/merge/merge_test.go | 93 ++++++++ 27 files changed, 593 insertions(+), 386 deletions(-) delete mode 100644 acceptance/bundle/validate/nulls/resources_container/test.toml delete mode 100644 acceptance/bundle/validate/nulls/schedule_default_materialization/test.toml create mode 100644 libs/dyn/drop_nulls.go create mode 100644 libs/dyn/drop_nulls_test.go diff --git a/acceptance/bundle/validate/empty_resources/empty_def/output.txt b/acceptance/bundle/validate/empty_resources/empty_def/output.txt index 5e4ac031008..a7b495ca9db 100644 --- a/acceptance/bundle/validate/empty_resources/empty_def/output.txt +++ b/acceptance/bundle/validate/empty_resources/empty_def/output.txt @@ -1,154 +1,70 @@ === resources.jobs.rname === -Error: job rname is not defined - at resources.jobs.rname - in databricks.yml:6:11 - { - "jobs": { - "rname": null - } + "jobs": {} } === resources.pipelines.rname === -Error: pipeline rname is not defined - at resources.pipelines.rname - in databricks.yml:6:11 - { - "pipelines": { - "rname": null - } + "pipelines": {} } === resources.models.rname === -Error: model rname is not defined - at resources.models.rname - in databricks.yml:6:11 - { - "models": { - "rname": null - } + "models": {} } === resources.experiments.rname === -Error: experiment rname is not defined - at resources.experiments.rname - in databricks.yml:6:11 - { - "experiments": { - "rname": null - } + "experiments": {} } === resources.registered_models.rname === -Error: registered_model rname is not defined - at resources.registered_models.rname - in databricks.yml:6:11 - { - "registered_models": { - "rname": null - } + "registered_models": {} } === resources.quality_monitors.rname === -Error: quality_monitor rname is not defined - at resources.quality_monitors.rname - in databricks.yml:6:11 - { - "quality_monitors": { - "rname": null - } + "quality_monitors": {} } === resources.schemas.rname === -Error: schema rname is not defined - at resources.schemas.rname - in databricks.yml:6:11 - { - "schemas": { - "rname": null - } + "schemas": {} } === resources.volumes.rname === -Error: volume rname is not defined - at resources.volumes.rname - in databricks.yml:6:11 - { - "volumes": { - "rname": null - } + "volumes": {} } === resources.clusters.rname === -Error: cluster rname is not defined - at resources.clusters.rname - in databricks.yml:6:11 - { - "clusters": { - "rname": null - } + "clusters": {} } === resources.dashboards.rname === -Error: dashboard rname is not defined - at resources.dashboards.rname - in databricks.yml:6:11 - { - "dashboards": { - "rname": null - } + "dashboards": {} } === resources.apps.rname === -Error: app rname is not defined - at resources.apps.rname - in databricks.yml:6:11 - { - "apps": { - "rname": null - } + "apps": {} } === resources.sql_warehouses.rname === -Error: sql_warehouse rname is not defined - at resources.sql_warehouses.rname - in databricks.yml:6:11 - { - "sql_warehouses": { - "rname": null - } + "sql_warehouses": {} } === resources.secret_scopes.rname === -Error: secret_scope rname is not defined - at resources.secret_scopes.rname - in databricks.yml:6:11 - { - "secret_scopes": { - "rname": null - } + "secret_scopes": {} } === resources.alerts.rname === -Error: alert rname is not defined - at resources.alerts.rname - in databricks.yml:6:11 - { - "alerts": { - "rname": null - } + "alerts": {} } diff --git a/acceptance/bundle/validate/empty_resources/null/output.txt b/acceptance/bundle/validate/empty_resources/null/output.txt index 1c64bbb7176..a7b495ca9db 100644 --- a/acceptance/bundle/validate/empty_resources/null/output.txt +++ b/acceptance/bundle/validate/empty_resources/null/output.txt @@ -1,154 +1,70 @@ === resources.jobs.rname === -Error: job rname is not defined - at resources.jobs.rname - in databricks.yml:6:12 - { - "jobs": { - "rname": null - } + "jobs": {} } === resources.pipelines.rname === -Error: pipeline rname is not defined - at resources.pipelines.rname - in databricks.yml:6:12 - { - "pipelines": { - "rname": null - } + "pipelines": {} } === resources.models.rname === -Error: model rname is not defined - at resources.models.rname - in databricks.yml:6:12 - { - "models": { - "rname": null - } + "models": {} } === resources.experiments.rname === -Error: experiment rname is not defined - at resources.experiments.rname - in databricks.yml:6:12 - { - "experiments": { - "rname": null - } + "experiments": {} } === resources.registered_models.rname === -Error: registered_model rname is not defined - at resources.registered_models.rname - in databricks.yml:6:12 - { - "registered_models": { - "rname": null - } + "registered_models": {} } === resources.quality_monitors.rname === -Error: quality_monitor rname is not defined - at resources.quality_monitors.rname - in databricks.yml:6:12 - { - "quality_monitors": { - "rname": null - } + "quality_monitors": {} } === resources.schemas.rname === -Error: schema rname is not defined - at resources.schemas.rname - in databricks.yml:6:12 - { - "schemas": { - "rname": null - } + "schemas": {} } === resources.volumes.rname === -Error: volume rname is not defined - at resources.volumes.rname - in databricks.yml:6:12 - { - "volumes": { - "rname": null - } + "volumes": {} } === resources.clusters.rname === -Error: cluster rname is not defined - at resources.clusters.rname - in databricks.yml:6:12 - { - "clusters": { - "rname": null - } + "clusters": {} } === resources.dashboards.rname === -Error: dashboard rname is not defined - at resources.dashboards.rname - in databricks.yml:6:12 - { - "dashboards": { - "rname": null - } + "dashboards": {} } === resources.apps.rname === -Error: app rname is not defined - at resources.apps.rname - in databricks.yml:6:12 - { - "apps": { - "rname": null - } + "apps": {} } === resources.sql_warehouses.rname === -Error: sql_warehouse rname is not defined - at resources.sql_warehouses.rname - in databricks.yml:6:12 - { - "sql_warehouses": { - "rname": null - } + "sql_warehouses": {} } === resources.secret_scopes.rname === -Error: secret_scope rname is not defined - at resources.secret_scopes.rname - in databricks.yml:6:12 - { - "secret_scopes": { - "rname": null - } + "secret_scopes": {} } === resources.alerts.rname === -Error: alert rname is not defined - at resources.alerts.rname - in databricks.yml:6:12 - { - "alerts": { - "rname": null - } + "alerts": {} } diff --git a/acceptance/bundle/validate/empty_tasks/output.txt b/acceptance/bundle/validate/empty_tasks/output.txt index 36e6a7e63a8..7a8fcd95b9e 100644 --- a/acceptance/bundle/validate/empty_tasks/output.txt +++ b/acceptance/bundle/validate/empty_tasks/output.txt @@ -12,8 +12,7 @@ "name": "job with empty tasks", "queue": { "enabled": true - }, - "tasks": null + } }, "valid": { "deployment": { diff --git a/acceptance/bundle/validate/nulls/include_removes/output.txt b/acceptance/bundle/validate/nulls/include_removes/output.txt index f55a8cad6a6..b17656ebfdd 100644 --- a/acceptance/bundle/validate/nulls/include_removes/output.txt +++ b/acceptance/bundle/validate/nulls/include_removes/output.txt @@ -1,58 +1,28 @@ >>> [CLI] bundle validate -o json -Warning: expected a string value, found null - at bundle.cluster_id - in override.yml:2:15 - -Warning: expected a string value, found null - at presets.name_prefix - in override.yml:5:16 - -Warning: expected a string value, found null - at presets.tags.removed - in override.yml:7:14 - -Warning: expected a string value, found null - at presets.tags.not_in_base - in override.yml:8:18 - -Error: multiple resources or scripts have been defined with the same key: bar - at resources.jobs.bar - in databricks.yml:34:7 - override.yml:23:10 +Error: no value assigned to required variable with_default. Variables are usually assigned in databricks.yml, and they can be overridden using "--var", the BUNDLE_VAR_with_default environment variable, or .databricks/bundle//variable-overrides.json Exit code: 1 { - "cluster_id": "base-cluster", + "cluster_id": null, "presets": { - "name_prefix": "base-", + "name_prefix": null, "tags": { - "kept": "1", - "removed": "1" + "kept": "1" } }, "sync": { - "include": [ - "databricks.yml" + "paths": [ + "." ] }, - "permissions": [ - { - "group_name": "users", - "level": "CAN_VIEW" - } - ], + "permissions": null, "variables": { - "with_default": { - "default": "base" - } - }, - "run_as": { - "user_name": "base@example.com" + "with_default": {} }, + "run_as": null, "jobs": [ - "bar", "foo" ] } diff --git a/acceptance/bundle/validate/nulls/include_target_overrides/output.txt b/acceptance/bundle/validate/nulls/include_target_overrides/output.txt index 77ea92b92a9..cff39585292 100644 --- a/acceptance/bundle/validate/nulls/include_target_overrides/output.txt +++ b/acceptance/bundle/validate/nulls/include_target_overrides/output.txt @@ -1,43 +1,21 @@ >>> [CLI] bundle validate -o json -t dev -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.description - in a.yml:7:24 - -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.tags.removed - in a.yml:10:22 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.tasks[0].max_retries - in a.yml:13:28 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.max_concurrent_runs - in b.yml:8:32 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.timeout_seconds - in b.yml:9:28 - { "foo": { "description": "description from b.yml", - "max_concurrent_runs": 10, - "timeout_seconds": 120, + "max_concurrent_runs": 1, + "timeout_seconds": null, "tags": { - "kept": "1", - "removed": "1" + "kept": "1" }, "tasks": [ { "task_key": "t1", - "max_retries": 3 + "max_retries": null } ] }, "jobs": [ - "bar", "foo" ] } diff --git a/acceptance/bundle/validate/nulls/resources_container/output.txt b/acceptance/bundle/validate/nulls/resources_container/output.txt index 1241151fda5..d44a21b582a 100644 --- a/acceptance/bundle/validate/nulls/resources_container/output.txt +++ b/acceptance/bundle/validate/nulls/resources_container/output.txt @@ -1,9 +1,9 @@ >>> [CLI] bundle validate -Error: expected a map at "resources", found nil - Name: test-bundle +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default -Found 1 error - -Exit code: 1 +Validation OK! diff --git a/acceptance/bundle/validate/nulls/resources_container/script b/acceptance/bundle/validate/nulls/resources_container/script index 253cc3d205d..93f223b138b 100644 --- a/acceptance/bundle/validate/nulls/resources_container/script +++ b/acceptance/bundle/validate/nulls/resources_container/script @@ -1,2 +1,2 @@ -# resources: null is rejected. +# A null resources section in databricks.yml has nothing to remove and is accepted. errcode trace $CLI bundle validate diff --git a/acceptance/bundle/validate/nulls/resources_container/test.toml b/acceptance/bundle/validate/nulls/resources_container/test.toml deleted file mode 100644 index 022e0f2c7e9..00000000000 --- a/acceptance/bundle/validate/nulls/resources_container/test.toml +++ /dev/null @@ -1 +0,0 @@ -Badness = 'An empty (null) resources section is an error without a location, while empty targets, variables or sync sections are accepted.' diff --git a/acceptance/bundle/validate/nulls/root_without_base/output.txt b/acceptance/bundle/validate/nulls/root_without_base/output.txt index 81520fc5908..90da8d094ee 100644 --- a/acceptance/bundle/validate/nulls/root_without_base/output.txt +++ b/acceptance/bundle/validate/nulls/root_without_base/output.txt @@ -1,17 +1,7 @@ >>> [CLI] bundle validate -o json -Warning: expected a string value, found null - at resources.jobs.foo.description - in databricks.yml:8:20 - -Warning: expected a string value, found null - at resources.jobs.foo.tags.not_in_base - in databricks.yml:11:22 - { - "email_notifications": { - "on_failure": null - }, + "email_notifications": {}, "max_concurrent_runs": 1, "name": "foo", "queue": { diff --git a/acceptance/bundle/validate/nulls/schedule_default_materialization/output.txt b/acceptance/bundle/validate/nulls/schedule_default_materialization/output.txt index 8cc844569f5..b58f46ea8bd 100644 --- a/acceptance/bundle/validate/nulls/schedule_default_materialization/output.txt +++ b/acceptance/bundle/validate/nulls/schedule_default_materialization/output.txt @@ -1,13 +1,5 @@ >>> [CLI] bundle validate -o json -Warning: required field "quartz_cron_expression" is not set - at resources.jobs.j.schedule - in databricks.yml:8:17 - -Warning: required field "timezone_id" is not set - at resources.jobs.j.schedule - in databricks.yml:8:17 - { "schedule": null } diff --git a/acceptance/bundle/validate/nulls/schedule_default_materialization/script b/acceptance/bundle/validate/nulls/schedule_default_materialization/script index 0c5e31164ff..e8611e6fa89 100644 --- a/acceptance/bundle/validate/nulls/schedule_default_materialization/script +++ b/acceptance/bundle/validate/nulls/schedule_default_materialization/script @@ -1,2 +1,2 @@ -# Defaults are not applied under an explicitly null block (schedule stays null). +# A null schedule in databricks.yml has nothing to remove: the job has no schedule. trace $CLI bundle validate -o json | jq '.resources.jobs.j | {schedule}' diff --git a/acceptance/bundle/validate/nulls/schedule_default_materialization/test.toml b/acceptance/bundle/validate/nulls/schedule_default_materialization/test.toml deleted file mode 100644 index 68e81befdb7..00000000000 --- a/acceptance/bundle/validate/nulls/schedule_default_materialization/test.toml +++ /dev/null @@ -1 +0,0 @@ -Badness = 'An explicitly null schedule is reported as missing its required fields.' diff --git a/acceptance/bundle/validate/nulls/target_keyed_slices/output.txt b/acceptance/bundle/validate/nulls/target_keyed_slices/output.txt index 7b9a2d8e2d6..efeac34a4b5 100644 --- a/acceptance/bundle/validate/nulls/target_keyed_slices/output.txt +++ b/acceptance/bundle/validate/nulls/target_keyed_slices/output.txt @@ -1,24 +1,9 @@ >>> [CLI] bundle validate -o json -t dev -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.job_clusters[0].new_cluster.num_workers - in databricks.yml:45:30 - -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.parameters[0].default - in databricks.yml:48:24 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.tasks[0].max_retries - in databricks.yml:51:28 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.tasks[1].max_retries - in databricks.yml:54:28 - -Warning: expected a int value, found null - at targets.dev.resources.pipelines.bar.clusters[0].num_workers - in databricks.yml:61:28 +Warning: required field "default" is not set + at resources.jobs.foo.parameters[0] + in databricks.yml:15:11 + databricks.yml:47:15 { "job_clusters": [ @@ -26,21 +11,19 @@ Warning: expected a int value, found null "job_cluster_key": "c1", "new_cluster": { "node_type_id": "[NODE_TYPE_ID]", - "num_workers": 2, + "num_workers": 0, "spark_version": "15.4.x-scala2.12" } } ], "parameters": [ { - "default": "base", "name": "p1" } ], "tasks": [ { "job_cluster_key": "c1", - "max_retries": 3, "notebook_task": { "notebook_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/dev/files/nb" }, @@ -66,8 +49,7 @@ Warning: expected a int value, found null "pipeline_clusters": [ { "label": "default", - "node_type_id": "[NODE_TYPE_ID]", - "num_workers": 2 + "node_type_id": "[NODE_TYPE_ID]" } ] } diff --git a/acceptance/bundle/validate/nulls/target_removes_job_fields/output.txt b/acceptance/bundle/validate/nulls/target_removes_job_fields/output.txt index 551c3980f68..c4e6432ab54 100644 --- a/acceptance/bundle/validate/nulls/target_removes_job_fields/output.txt +++ b/acceptance/bundle/validate/nulls/target_removes_job_fields/output.txt @@ -1,51 +1,22 @@ >>> [CLI] bundle validate -o json -t dev -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.description - in databricks.yml:30:24 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.max_concurrent_runs - in databricks.yml:31:32 - -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.schedule.pause_status - in databricks.yml:34:27 - -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.tags.removed - in databricks.yml:36:22 - -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.tags.not_in_base - in databricks.yml:37:26 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.timeout_seconds - in databricks.yml:40:28 - { - "description": "base description", "email_notifications": { - "on_failure": [ - "base@example.com" - ], "on_success": [ "base@example.com" ] }, - "max_concurrent_runs": 5, + "max_concurrent_runs": 1, "name": "foo", "queue": { - "enabled": false + "enabled": true }, "schedule": { - "pause_status": "PAUSED", + "pause_status": "UNPAUSED", "quartz_cron_expression": "0 0 * * * ?", "timezone_id": "UTC" }, "tags": { - "kept": "1", - "removed": "1" + "kept": "1" } } diff --git a/acceptance/bundle/validate/nulls/target_removes_resource/output.txt b/acceptance/bundle/validate/nulls/target_removes_resource/output.txt index e4c5e98b82b..2c0b94dfe2c 100644 --- a/acceptance/bundle/validate/nulls/target_removes_resource/output.txt +++ b/acceptance/bundle/validate/nulls/target_removes_resource/output.txt @@ -1,20 +1,10 @@ >>> [CLI] bundle validate -o json -t dev -Error: job not_in_base is not defined - at resources.jobs.not_in_base - in databricks.yml:19:22 - - -Exit code: 1 { "jobs": [ - "bar", - "foo", - "not_in_base" + "foo" ], - "pipelines": [ - "baz" - ] + "pipelines": [] } >>> [CLI] bundle validate -o json -t prod diff --git a/acceptance/bundle/validate/presets_name_prefix_dev/output.txt b/acceptance/bundle/validate/presets_name_prefix_dev/output.txt index c7d93c71444..570cc29a6ff 100644 --- a/acceptance/bundle/validate/presets_name_prefix_dev/output.txt +++ b/acceptance/bundle/validate/presets_name_prefix_dev/output.txt @@ -32,9 +32,5 @@ presets: # PRESETS >>> [CLI] bundle validate -o json -Warning: expected a string value, found null - at presets.name_prefix - in databricks.yml:25:15 - === The end diff --git a/acceptance/bundle/validate/required/output.txt b/acceptance/bundle/validate/required/output.txt index ae3e6dad2f8..0c55c9cdb78 100644 --- a/acceptance/bundle/validate/required/output.txt +++ b/acceptance/bundle/validate/required/output.txt @@ -4,10 +4,6 @@ Warning: required field "catalog_name" is not set at resources.volumes.my_volume in databricks.yml:35:7 -Warning: required field "job_id" is not set - at resources.jobs.my_job_1.tasks[1].run_job_task - in databricks.yml:29:24 - Warning: required field "name" is not set at resources.models.my_model_1 in databricks.yml:13:7 @@ -26,4 +22,4 @@ Workspace: User: [USERNAME] Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default -Found 5 warnings +Found 4 warnings diff --git a/bundle/config/mutator/resourcemutator/run_as_test.go b/bundle/config/mutator/resourcemutator/run_as_test.go index 200939f2dd0..26c4ad39d6f 100644 --- a/bundle/config/mutator/resourcemutator/run_as_test.go +++ b/bundle/config/mutator/resourcemutator/run_as_test.go @@ -396,7 +396,7 @@ func TestRunAsIdentities(t *testing.T) { runAs string wantError bool }{ - {`null`, true}, + {`null`, false}, {`{}`, true}, {`{user_name: ""}`, true}, {`{service_principal_name: ""}`, true}, diff --git a/bundle/config/root.go b/bundle/config/root.go index a64b742630e..5d1e1e36418 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "reflect" + "slices" "strings" "github.com/databricks/cli/bundle/config/resources" @@ -38,6 +39,14 @@ type Root struct { //nolint:recvcheck // value receivers for read-only accessors value dyn.Value depth int + // withNulls is the configuration including the nil values of maps, which remove the + // value they are merged onto (see [Root.Merge] and [Root.MergeTargetOverrides]). + // value cannot hold them: a nil scalar does not convert to the typed configuration, + // and the conversion around every mutator drops the other nils. For a loaded file this + // is the whole file; after merging, only the target sections, which keep their nils + // until the target is merged. + withNulls dyn.Value + // Contains user defined variables Variables map[string]*variable.Variable `json:"variables,omitempty"` @@ -132,10 +141,18 @@ func LoadFromBytes(path string, raw []byte) (*Root, diag.Diagnostics) { } // Normalize dynamic configuration tree according to configuration type. - v, diags := convert.Normalize(r, v) + v, diags := convert.Normalize(r, v, convert.KeepNulls) + r.withNulls = v + + // A null has nothing to remove in a file on its own, so it is dropped, except where + // it declares an empty target or variable. + nv, err := addDeclarations(dyn.DropNulls(v), dyn.NilValue, v) + if err != nil { + return nil, diags.Extend(diag.Errorf("failed to load %s: %v", path, err)) + } // Convert normalized configuration tree to typed configuration. - err = r.updateWithDynamicValue(v) + err = r.updateWithDynamicValue(nv) if err != nil { diags = diags.Extend(diag.Errorf("failed to load %s: %v", path, err)) return nil, diags @@ -163,9 +180,11 @@ func (r *Root) updateWithDynamicValue(nv dyn.Value) error { // Hack: restore state; it may be cleared by [ToTyped] if // the configuration equals nil (happens in tests). depth := r.depth + withNulls := r.withNulls defer func() { r.depth = depth + r.withNulls = withNulls }() // Convert normalized configuration tree to typed configuration. @@ -295,13 +314,98 @@ func (r *Root) InitializeVariables(vars []string) error { return nil } +// Merge merges other into r. Values in other take precedence, and a null in other +// removes the value from r. func (r *Root) Merge(other *Root) error { + nulls := targetSections(other.withNulls) + if r.withNulls.IsValid() { + var err error + nulls, err = merge.MergeKeepingNulls(r.withNulls, nulls) + if err != nil { + return err + } + } + r.withNulls = nulls + // Merge dynamic configuration values. return r.Mutate(func(root dyn.Value) (dyn.Value, error) { - return merge.Merge(root, other.value) + merged, err := merge.MergeRemovingNulls(root, other.withNulls) + if err != nil { + return dyn.InvalidValue, err + } + return addDeclarations(merged, root, other.withNulls) }) } +// declarationSections are the sections where a null entry declares an empty entry +// if there is no entry to remove, e.g. an empty "targets.dev:" declares target dev. +var declarationSections = []string{"targets", "environments", "variables"} + +// addDeclarations returns v with the null entries of the declaration sections of +// nulls that base does not have. +func addDeclarations(v, base, nulls dyn.Value) (dyn.Value, error) { + for _, section := range declarationSections { + entries, ok := nulls.Get(section).AsMap() + if !ok { + continue + } + for _, pair := range entries.Pairs() { + name := pair.Key.MustString() + if pair.Value.Kind() != dyn.KindNil || base.Get(section).Get(name).IsValid() { + continue + } + var err error + v, err = dyn.SetByPath(v, dyn.NewPath(dyn.Key(section), dyn.Key(name)), pair.Value) + if err != nil { + return dyn.InvalidValue, err + } + } + } + return v, nil +} + +// targetSections returns the sections of v that hold target overrides. +func targetSections(v dyn.Value) dyn.Value { + out := dyn.NewMapping() + for _, key := range []string{"targets", "environments"} { + if section := v.Get(key); section.IsValid() { + out.SetLoc(key, nil, section) + } + } + return dyn.V(out) +} + +// targetWithNulls returns the target as loaded, including its nulls. +func (r *Root) targetWithNulls(name string) dyn.Value { + targets := r.withNulls.Get("targets") + if !targets.IsValid() { + // The deprecated name, renamed to "targets" by the EnvironmentsToTargets mutator. + targets = r.withNulls.Get("environments") + } + return targets.Get(name) +} + +// setNulls returns dst with the nil map values of src set at the same paths. +func setNulls(dst, src dyn.Value) (dyn.Value, error) { + var nulls []dyn.Path + _, err := dyn.Walk(src, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + if v.Kind() == dyn.KindNil && len(p) > 0 && p[len(p)-1].Key() != "" { + nulls = append(nulls, slices.Clone(p)) + } + return v, nil + }) + if err != nil { + return dyn.InvalidValue, err + } + for _, p := range nulls { + dst, err = dyn.SetByPath(dst, p, dyn.NilValue) + if err != nil { + return dyn.InvalidValue, err + } + } + return dst, nil +} + func mergeField(rv, ov dyn.Value, name string) (dyn.Value, error) { path := dyn.NewPath(dyn.Key(name)) reference, _ := dyn.GetByPath(rv, path) @@ -311,7 +415,7 @@ func mergeField(rv, ov dyn.Value, name string) (dyn.Value, error) { var out dyn.Value var err error if reference.IsValid() && override.IsValid() { - out, err = merge.Merge(reference, override) + out, err = merge.MergeKeepingNulls(reference, override) if err != nil { return dyn.InvalidValue, err } @@ -339,6 +443,13 @@ func (r *Root) MergeTargetOverrides(name string) error { return err } + // A null in the target removes the value from the root. The nulls are set here, and + // removed with what they override at the end. + target, err = setNulls(target, r.targetWithNulls(name)) + if err != nil { + return fmt.Errorf("failed to apply the nulls of target=%s: %w", name, err) + } + // Merge fields that can be merged 1:1. for _, f := range []string{ "bundle", @@ -369,8 +480,10 @@ func (r *Root) MergeTargetOverrides(name string) error { // If the target explicitly sets a default value, drop any lookup from the // root variable definition so SetVariables can assign this default. - lookupPath := varPath.Append(dyn.Key("lookup")) - root, err = dyn.SetByPath(root, lookupPath, dyn.NilValue) + if vDefault.Kind() != dyn.KindNil { + lookupPath := varPath.Append(dyn.Key("lookup")) + root, err = dyn.SetByPath(root, lookupPath, dyn.NilValue) + } } vLookup := variable.Get("lookup") @@ -383,8 +496,10 @@ func (r *Root) MergeTargetOverrides(name string) error { // If the target explicitly sets a lookup, drop any default value from the // root variable definition so lookup resolution remains authoritative. - defaultPath := varPath.Append(dyn.Key("default")) - root, err = dyn.SetByPath(root, defaultPath, dyn.NilValue) + if vLookup.Kind() != dyn.KindNil { + defaultPath := varPath.Append(dyn.Key("default")) + root, err = dyn.SetByPath(root, defaultPath, dyn.NilValue) + } } return root, err @@ -434,7 +549,7 @@ func (r *Root) MergeTargetOverrides(name string) error { } // Merge the override into the reference. - out, err := merge.Merge(ref, v) + out, err := merge.MergeKeepingNulls(ref, v) if err != nil { return err } @@ -446,10 +561,101 @@ func (r *Root) MergeTargetOverrides(name string) error { } } + root, err = mergeKeyedSequencesWithNulls(root) + if err != nil { + return err + } + + // The nulls have replaced the values they remove. Remaining nulls had nothing to + // remove and are not added either. + root = dyn.DropNulls(root) + r.withNulls = dyn.InvalidValue + // Convert normalized configuration tree to typed configuration. return r.updateWithDynamicValue(root) } +// keyedSequences are the sequences whose elements the Merge* mutators in +// bundle/config/mutator/resourcemutator merge by key. Those run after the nulls are +// gone, so a sequence with nulls in its elements is merged by key with the target. +var keyedSequences = []struct { + resourceType string + field string + key string + keyFunc func(dyn.Value) string +}{ + {"jobs", "tasks", "task_key", stringKey}, + {"jobs", "job_clusters", "job_cluster_key", stringKey}, + {"jobs", "parameters", "name", stringKey}, + {"pipelines", "clusters", "label", pipelineClusterLabel}, +} + +func stringKey(v dyn.Value) string { + s, _ := v.AsString() + return s +} + +// pipelineClusterLabel matches the key of the MergePipelineClusters mutator: the label +// is case-insensitive and defaults to "default". +func pipelineClusterLabel(v dyn.Value) string { + s, ok := v.AsString() + if !ok { + return "default" + } + return strings.ToLower(s) +} + +// mergeKeyedSequencesWithNulls merges the elements with the same key of the keyed +// sequences that hold nulls, so that a null removes the value from the element it +// overrides. Resources and elements can still be ${...} references here, which are +// left as they are. +func mergeKeyedSequencesWithNulls(root dyn.Value) (dyn.Value, error) { + for _, ks := range keyedSequences { + resources, ok := root.Get("resources").Get(ks.resourceType).AsMap() + if !ok { + continue + } + for _, pair := range resources.Pairs() { + field := pair.Value.Get(ks.field) + seq, ok := field.AsSequence() + if !ok || !hasNulls(field) || slices.ContainsFunc(seq, isNotMap) { + continue + } + merged, err := merge.ElementsByKeyKeepingNulls(ks.key, ks.keyFunc)(dyn.EmptyPath, field) + if err != nil { + return dyn.InvalidValue, err + } + p := dyn.NewPath(dyn.Key("resources"), dyn.Key(ks.resourceType), dyn.Key(pair.Key.MustString()), dyn.Key(ks.field)) + root, err = dyn.SetByPath(root, p, merged) + if err != nil { + return dyn.InvalidValue, err + } + } + } + return root, nil +} + +func isNotMap(v dyn.Value) bool { + return v.Kind() != dyn.KindMap +} + +// hasNulls reports whether v holds a nil map value at any depth. +func hasNulls(v dyn.Value) bool { + switch v.Kind() { + case dyn.KindMap: + for _, pair := range v.MustMap().Pairs() { + if pair.Value.Kind() == dyn.KindNil || hasNulls(pair.Value) { + return true + } + } + case dyn.KindSequence: + return slices.ContainsFunc(v.MustSequence(), hasNulls) + default: + // Leaf node. + } + return false +} + var allowedVariableDefinitions = []([]string){ {"default", "type", "description"}, {"default", "type"}, diff --git a/libs/dyn/convert/normalize.go b/libs/dyn/convert/normalize.go index e41ed4a0a87..17eb2c0d553 100644 --- a/libs/dyn/convert/normalize.go +++ b/libs/dyn/convert/normalize.go @@ -26,11 +26,17 @@ const ( // ToTyped, which force-sends it, and the backend rejects it (e.g. // "'' is not a valid cluster policy ID"). DropEmptyStrings + + // KeepNulls keeps the nil values of struct fields and map entries as they are, + // instead of dropping them with a warning (scalars) or keeping them (other types). + // Nil sequence elements are handled as without this option. + KeepNulls ) type normalizeOptions struct { includeMissingFields bool dropEmptyStrings bool + keepNulls bool } func Normalize(dst any, src dyn.Value, opts ...NormalizeOption) (dyn.Value, diag.Diagnostics) { @@ -41,6 +47,8 @@ func Normalize(dst any, src dyn.Value, opts ...NormalizeOption) (dyn.Value, diag n.includeMissingFields = true case DropEmptyStrings: n.dropEmptyStrings = true + case KeepNulls: + n.keepNulls = true } } @@ -167,6 +175,11 @@ func (n normalizeOptions) normalizeStruct(typ reflect.Type, src dyn.Value, seen continue } + if n.keepNulls && pv.Kind() == dyn.KindNil { + out.SetLoc(pk.MustString(), pk.Locations(), pv) + continue + } + // Normalize the value according to the field type. nv, err := n.normalizeType(typ.FieldByIndex(index).Type, pv, seen, path.Append(dyn.Key(pk.MustString()))) if err != nil { @@ -263,6 +276,11 @@ func (n normalizeOptions) normalizeMap(typ reflect.Type, src dyn.Value, seen []r pk := pair.Key pv := pair.Value + if n.keepNulls && pv.Kind() == dyn.KindNil { + out.SetLoc(pk.MustString(), pk.Locations(), pv) + continue + } + // Normalize the value according to the map element type. nv, err := n.normalizeType(typ.Elem(), pv, seen, path.Append(dyn.Key(pk.MustString()))) if err != nil { diff --git a/libs/dyn/convert/normalize_test.go b/libs/dyn/convert/normalize_test.go index ac95aaafac6..9cc4863d317 100644 --- a/libs/dyn/convert/normalize_test.go +++ b/libs/dyn/convert/normalize_test.go @@ -1016,3 +1016,40 @@ func TestNormalizeStructDropEmptyStringsKeepsNonEmpty(t *testing.T) { assert.True(t, ok) assert.Equal(t, "value", opt.MustString()) } + +func TestNormalizeKeepNulls(t *testing.T) { + type Inner struct { + Foo string `json:"foo"` + } + type Tmp struct { + Str string `json:"str"` + Int int `json:"int"` + Inner *Inner `json:"inner"` + Map map[string]string `json:"map"` + List []string `json:"list"` + } + + var typ Tmp + vin := dyn.V(map[string]dyn.Value{ + "str": dyn.NilValue, + "int": dyn.NilValue, + "inner": dyn.V(map[string]dyn.Value{"foo": dyn.NilValue}), + "map": dyn.V(map[string]dyn.Value{"key": dyn.NilValue}), + "list": dyn.V([]dyn.Value{dyn.V("a"), dyn.NilValue}), + }) + + vout, diags := Normalize(typ, vin, KeepNulls) + assert.Equal(t, diag.Diagnostics{{ + Severity: diag.Warning, + Summary: `expected a string value, found null`, + Locations: []diag.Location{{}}, + Paths: []*structpath.PathNode{structpath.NewIndex(structpath.NewStringKey(nil, "list"), 1)}, + }}, diags) + assert.Equal(t, map[string]any{ + "str": nil, + "int": nil, + "inner": map[string]any{"foo": nil}, + "map": map[string]any{"key": nil}, + "list": []any{"a"}, + }, vout.AsAny()) +} diff --git a/libs/dyn/drop_nulls.go b/libs/dyn/drop_nulls.go new file mode 100644 index 00000000000..0dd58dca977 --- /dev/null +++ b/libs/dyn/drop_nulls.go @@ -0,0 +1,28 @@ +package dyn + +// DropNulls returns v without the nil values of its maps, at any depth. +// Nil elements of sequences are kept. +func DropNulls(v Value) Value { + switch v.Kind() { + case KindMap: + m := v.MustMap() + out := newMappingWithSize(m.Len()) + for _, pair := range m.Pairs() { + if pair.Value.Kind() == KindNil { + continue + } + out.SetLoc(pair.Key.MustString(), pair.Key.Locations(), DropNulls(pair.Value)) + } + v.v = out + case KindSequence: + s := v.MustSequence() + out := make([]Value, len(s)) + for i := range s { + out[i] = DropNulls(s[i]) + } + v.v = out + default: + // Leaf node. + } + return v +} diff --git a/libs/dyn/drop_nulls_test.go b/libs/dyn/drop_nulls_test.go new file mode 100644 index 00000000000..38d51797e37 --- /dev/null +++ b/libs/dyn/drop_nulls_test.go @@ -0,0 +1,35 @@ +package dyn_test + +import ( + "testing" + + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + assert "github.com/databricks/cli/libs/dyn/dynassert" +) + +func TestDropNulls(t *testing.T) { + loc := diag.Location{File: "file", Line: 1, Column: 2} + v := dyn.NewValue(map[string]dyn.Value{ + "null": dyn.NilValue, + "map": dyn.V(map[string]dyn.Value{ + "null": dyn.NilValue, + "value": dyn.V("a"), + }), + "list": dyn.V([]dyn.Value{ + dyn.NilValue, + dyn.V(map[string]dyn.Value{"null": dyn.NilValue}), + }), + }, []diag.Location{loc}) + + out := dyn.DropNulls(v) + assert.Equal(t, map[string]any{ + "map": map[string]any{"value": "a"}, + "list": []any{nil, map[string]any{}}, + }, out.AsAny()) + assert.Equal(t, []diag.Location{loc}, out.Locations()) +} + +func TestDropNullsNil(t *testing.T) { + assert.Equal(t, dyn.NilValue, dyn.DropNulls(dyn.NilValue)) +} diff --git a/libs/dyn/merge/elements_by_key.go b/libs/dyn/merge/elements_by_key.go index 6bf71d41ae5..b503ce87a15 100644 --- a/libs/dyn/merge/elements_by_key.go +++ b/libs/dyn/merge/elements_by_key.go @@ -71,6 +71,10 @@ func (e elementsByKey) Map(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return e.doMap(nil, v, Merge) } +func (e elementsByKey) MapKeepingNulls(_ dyn.Path, v dyn.Value) (dyn.Value, error) { + return e.doMap(nil, v, MergeKeepingNulls) +} + func (e elementsByKey) MapWithOverride(p dyn.Path, v dyn.Value) (dyn.Value, error) { return e.doMap(nil, v, func(a, b dyn.Value) (dyn.Value, error) { return Override(a, b, OverrideVisitor{ @@ -105,3 +109,9 @@ func ElementsBySortedKey(key string, keyFunc func(dyn.Value) string) dyn.MapFunc func ElementsByKeyWithOverride(key string, keyFunc func(dyn.Value) string) dyn.MapFunc { return elementsByKey{key, keyFunc, false}.MapWithOverride } + +// ElementsByKeyKeepingNulls is like [ElementsByKey], except that elements with the same +// key are merged with [MergeKeepingNulls]. +func ElementsByKeyKeepingNulls(key string, keyFunc func(dyn.Value) string) dyn.MapFunc { + return elementsByKey{key, keyFunc, false}.MapKeepingNulls +} diff --git a/libs/dyn/merge/elements_by_key_test.go b/libs/dyn/merge/elements_by_key_test.go index 2aa19306820..67593b7aa6e 100644 --- a/libs/dyn/merge/elements_by_key_test.go +++ b/libs/dyn/merge/elements_by_key_test.go @@ -107,3 +107,31 @@ func TestElementByKeyWithOverride(t *testing.T) { }, vout.Index(1).AsAny(), ) } + +func TestElementByKeyKeepingNulls(t *testing.T) { + vin := dyn.V([]dyn.Value{ + dyn.V(map[string]dyn.Value{ + "key": dyn.V("foo"), + "removed": dyn.V(42), + "kept": dyn.V(42), + }), + dyn.V(map[string]dyn.Value{ + "key": dyn.V("foo"), + "removed": dyn.NilValue, + }), + }) + + keyFunc := func(v dyn.Value) string { + return v.MustString() + } + + vout, err := dyn.MapByPath(vin, dyn.EmptyPath, ElementsByKeyKeepingNulls("key", keyFunc)) + require.NoError(t, err) + assert.Equal(t, []any{ + map[string]any{ + "key": "foo", + "removed": nil, + "kept": 42, + }, + }, vout.AsAny()) +} diff --git a/libs/dyn/merge/merge.go b/libs/dyn/merge/merge.go index 374450ef240..a08f581c5f6 100644 --- a/libs/dyn/merge/merge.go +++ b/libs/dyn/merge/merge.go @@ -2,6 +2,7 @@ package merge import ( "fmt" + "slices" "github.com/databricks/cli/libs/dyn" ) @@ -33,15 +34,45 @@ import ( // incoming value is retained and the location of the existing value `a` is accumulated. // This is because the incoming value overwrites the existing value. func Merge(a, b dyn.Value) (dyn.Value, error) { - return merge(a, b) + return merge(a, b, nullIsIgnored) } -func merge(a, b dyn.Value) (dyn.Value, error) { +// MergeKeepingNulls is like [Merge], except that a nil in b replaces the value in a. +// The nils in b are kept in the result. +func MergeKeepingNulls(a, b dyn.Value) (dyn.Value, error) { + return merge(a, b, nullReplaces) +} + +// MergeRemovingNulls is like [Merge], except that a nil map value in b removes the +// same key from a. The nil map values of b are not added to the result, also where a +// has no value to remove (see [dyn.DropNulls]). A nil b leaves a unchanged. +func MergeRemovingNulls(a, b dyn.Value) (dyn.Value, error) { + return merge(a, b, nullRemoves) +} + +// nullMode is what a nil in the second value does to the first one. +type nullMode int + +const ( + nullIsIgnored nullMode = iota + nullReplaces + nullRemoves +) + +func merge(a, b dyn.Value, mode nullMode) (dyn.Value, error) { ak := a.Kind() bk := b.Kind() + // If b is nil and replaces a, return b. + if bk == dyn.KindNil && mode == nullReplaces { + return b, nil + } + // If a is nil, return b. if ak == dyn.KindNil { + if mode == nullRemoves { + b = dyn.DropNulls(b) + } return b.AppendLocationsFromValue(a), nil } @@ -56,12 +87,12 @@ func merge(a, b dyn.Value) (dyn.Value, error) { if bk != dyn.KindMap { return dyn.InvalidValue, fmt.Errorf("cannot merge map with %s", bk) } - return mergeMap(a, b) + return mergeMap(a, b, mode) case dyn.KindSequence: if bk != dyn.KindSequence { return dyn.InvalidValue, fmt.Errorf("cannot merge sequence with %s", bk) } - return mergeSequence(a, b) + return mergeSequence(a, b, mode) default: if ak != bk { return dyn.InvalidValue, fmt.Errorf("cannot merge %s with %s", ak, bk) @@ -70,7 +101,7 @@ func merge(a, b dyn.Value) (dyn.Value, error) { } } -func mergeMap(a, b dyn.Value) (dyn.Value, error) { +func mergeMap(a, b dyn.Value, mode nullMode) (dyn.Value, error) { out := dyn.NewMapping() am := a.MustMap() bm := b.MustMap() @@ -79,30 +110,45 @@ func mergeMap(a, b dyn.Value) (dyn.Value, error) { out.Merge(am) // Merge the values from b into the output map. + var removed []string for _, pair := range bm.Pairs() { pk := pair.Key key := pk.MustString() pv := pair.Value + if mode == nullRemoves && pv.Kind() == dyn.KindNil { + removed = append(removed, key) + continue + } if ov, ok := out.Get(pk); ok { // If the key already exists, merge the values. - merged, err := merge(ov, pv) + merged, err := merge(ov, pv, mode) if err != nil { return dyn.InvalidValue, err } out.SetLoc(key, pair.Key.Locations(), merged) } else { // Otherwise, just set the value. + if mode == nullRemoves { + pv = dyn.DropNulls(pv) + } out.SetLoc(key, pair.Key.Locations(), pv) } } + if len(removed) > 0 { + out = withoutKeys(out, removed) + } + // Preserve the location of the first value. Accumulate the locations of the second value. return dyn.NewValue(out, a.Locations()).AppendLocationsFromValue(b), nil } -func mergeSequence(a, b dyn.Value) (dyn.Value, error) { +func mergeSequence(a, b dyn.Value, mode nullMode) (dyn.Value, error) { as := a.MustSequence() bs := b.MustSequence() + if mode == nullRemoves { + bs = dyn.DropNulls(b).MustSequence() + } // Merging sequences means concatenating them. out := make([]dyn.Value, len(as)+len(bs)) @@ -117,3 +163,15 @@ func mergePrimitive(a, b dyn.Value) (dyn.Value, error) { // Merging primitive values means using the incoming value. return b.AppendLocationsFromValue(a), nil } + +// withoutKeys returns m without the given keys. +func withoutKeys(m dyn.Mapping, keys []string) dyn.Mapping { + out := dyn.NewMapping() + for _, pair := range m.Pairs() { + key := pair.Key.MustString() + if !slices.Contains(keys, key) { + out.SetLoc(key, pair.Key.Locations(), pair.Value) + } + } + return out +} diff --git a/libs/dyn/merge/merge_test.go b/libs/dyn/merge/merge_test.go index 9545c752258..43c655c295a 100644 --- a/libs/dyn/merge/merge_test.go +++ b/libs/dyn/merge/merge_test.go @@ -6,6 +6,7 @@ import ( "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestMergeMaps(t *testing.T) { @@ -299,3 +300,95 @@ func TestMergePrimitivesError(t *testing.T) { assert.Equal(t, dyn.InvalidValue, out) } } + +func TestMergeNullModes(t *testing.T) { + a := dyn.V(map[string]dyn.Value{ + "removed": dyn.V("a"), + "kept": dyn.V("a"), + "nested": dyn.V(map[string]dyn.Value{ + "removed": dyn.V("a"), + "kept": dyn.V("a"), + }), + "list": dyn.V([]dyn.Value{dyn.V("a")}), + }) + b := dyn.V(map[string]dyn.Value{ + "removed": dyn.NilValue, + "nested": dyn.V(map[string]dyn.Value{ + "removed": dyn.NilValue, + }), + "not_in_a": dyn.NilValue, + "new": dyn.V(map[string]dyn.Value{ + "null": dyn.NilValue, + "value": dyn.V("b"), + }), + "list": dyn.V([]dyn.Value{ + dyn.V(map[string]dyn.Value{"null": dyn.NilValue}), + }), + }) + + for _, tc := range []struct { + name string + merge func(a, b dyn.Value) (dyn.Value, error) + want map[string]any + }{ + { + name: "ignored", + merge: Merge, + want: map[string]any{ + "removed": "a", + "kept": "a", + "nested": map[string]any{"removed": "a", "kept": "a"}, + "list": []any{"a", map[string]any{"null": nil}}, + "not_in_a": nil, + "new": map[string]any{"null": nil, "value": "b"}, + }, + }, + { + name: "keeping nulls", + merge: MergeKeepingNulls, + want: map[string]any{ + "removed": nil, + "kept": "a", + "nested": map[string]any{"removed": nil, "kept": "a"}, + "list": []any{"a", map[string]any{"null": nil}}, + "not_in_a": nil, + "new": map[string]any{"null": nil, "value": "b"}, + }, + }, + { + name: "removing nulls", + merge: MergeRemovingNulls, + want: map[string]any{ + "kept": "a", + "nested": map[string]any{"kept": "a"}, + "list": []any{"a", map[string]any{}}, + "new": map[string]any{"value": "b"}, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + out, err := tc.merge(a, b) + require.NoError(t, err) + assert.Equal(t, tc.want, out.AsAny()) + }) + } +} + +func TestMergeNullModesNilRoot(t *testing.T) { + b := dyn.V(map[string]dyn.Value{ + "null": dyn.NilValue, + "value": dyn.V("b"), + }) + + out, err := MergeKeepingNulls(dyn.NilValue, b) + require.NoError(t, err) + assert.Equal(t, map[string]any{"null": nil, "value": "b"}, out.AsAny()) + + out, err = MergeRemovingNulls(dyn.NilValue, b) + require.NoError(t, err) + assert.Equal(t, map[string]any{"value": "b"}, out.AsAny()) + + out, err = MergeKeepingNulls(b, dyn.NilValue) + require.NoError(t, err) + assert.Equal(t, dyn.KindNil, out.Kind()) +} From 1fe40bd69e0ef2962002d48b82644c4e08cf69d3 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 30 Oct 2026 15:14:48 +0100 Subject: [PATCH 2/5] acc: regenerate nulls/target_removes_top_level_fields after the --var change Co-authored-by: Isaac --- .../output.txt | 55 +++---------------- 1 file changed, 8 insertions(+), 47 deletions(-) diff --git a/acceptance/bundle/validate/nulls/target_removes_top_level_fields/output.txt b/acceptance/bundle/validate/nulls/target_removes_top_level_fields/output.txt index 0397ae3730c..7cb643f1ce1 100644 --- a/acceptance/bundle/validate/nulls/target_removes_top_level_fields/output.txt +++ b/acceptance/bundle/validate/nulls/target_removes_top_level_fields/output.txt @@ -1,77 +1,38 @@ >>> [CLI] bundle validate -o json -t dev --var with_default=from-cli --var with_lookup=from-cli -Warning: expected a string value, found null - at targets.dev.mode - in databricks.yml:45:11 - -Warning: expected a string value, found null - at targets.dev.cluster_id - in databricks.yml:46:17 - -Warning: expected a string value, found null - at targets.dev.workspace.root_path - in databricks.yml:50:18 - -Warning: expected a string value, found null - at targets.dev.presets.name_prefix - in databricks.yml:52:20 - -Warning: expected a string value, found null - at targets.dev.presets.tags.removed - in databricks.yml:54:18 - { "bundle": { - "mode": "development", - "cluster_id": "base-cluster", + "mode": null, + "cluster_id": null, "git": { - "branch": "base-branch" + "branch": null } }, - "root_path": "/Workspace/Users/[USERNAME]/base", + "root_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/dev", "presets": { - "name_prefix": "base-[USERNAME]-", + "name_prefix": null, "tags": { - "dev": "[USERNAME]", - "kept": "1", - "removed": "1" + "kept": "1" } }, "sync": { - "include": [ - "databricks.yml" - ], "paths": [ "." ] }, - "permissions": [ - { - "level": "CAN_MANAGE", - "user_name": "[USERNAME]" - } - ], + "permissions": null, "variables": { "with_default": { - "default": "base", "value": "from-cli" }, "with_lookup": { - "lookup": { - "cluster": "some-test-cluster" - }, "value": "from-cli" } }, - "run_as": { - "user_name": "base@example.com" - }, + "run_as": null, "artifacts": { "kept": { "build": "build-kept" - }, - "removed": { - "build": "build-removed" } } } From 1e6ba6280fb43f4ef748799e1e0842756fea816b Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 30 Oct 2026 15:15:57 +0100 Subject: [PATCH 3/5] Add changelog entry Co-authored-by: Isaac --- .nextchanges/bundles/7261.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 .nextchanges/bundles/7261.md diff --git a/.nextchanges/bundles/7261.md b/.nextchanges/bundles/7261.md new file mode 100644 index 00000000000..7c38152d286 --- /dev/null +++ b/.nextchanges/bundles/7261.md @@ -0,0 +1 @@ +* A null in an included file or in a target now removes the value it overrides, e.g. `targets.dev.resources.jobs.foo: null` leaves job `foo` out of target `dev`; a null that has nothing to remove is ignored without a warning ([#7261](https://github.com/databricks/cli/pull/7261)). ([#7261](https://github.com/databricks/cli/pull/7261)) From 8b8b8de00a0a75339de552011e2377b45b54e500 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 30 Oct 2026 15:16:28 +0100 Subject: [PATCH 4/5] Fix changelog entry link Co-authored-by: Isaac --- .nextchanges/bundles/7261.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.nextchanges/bundles/7261.md b/.nextchanges/bundles/7261.md index 7c38152d286..277bad05511 100644 --- a/.nextchanges/bundles/7261.md +++ b/.nextchanges/bundles/7261.md @@ -1 +1 @@ -* A null in an included file or in a target now removes the value it overrides, e.g. `targets.dev.resources.jobs.foo: null` leaves job `foo` out of target `dev`; a null that has nothing to remove is ignored without a warning ([#7261](https://github.com/databricks/cli/pull/7261)). ([#7261](https://github.com/databricks/cli/pull/7261)) +* A null in an included file or in a target now removes the value it overrides, e.g. `targets.dev.resources.jobs.foo: null` leaves job `foo` out of target `dev`; a null that has nothing to remove is ignored without a warning. ([#7261](https://github.com/databricks/cli/pull/7261)) From b7e451e1908aa4dd8713f4e39975775b75d0b2d2 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 30 Oct 2026 15:46:29 +0100 Subject: [PATCH 5/5] Keep empty named entries, apply target nulls without dropping others An empty resources.., artifacts. or scripts. in a file is kept as a declaration, like an empty target or variable, so that it is still reported as undefined or not configured. Target nulls are applied with MergeRemovingNulls instead of dropping all nulls at the end, which removed those declarations too. Keyed sequences with nulls are merged by key with the root before the fields are merged. Co-authored-by: Isaac --- .../bundle/deploy/mlops-stacks/output.txt | 34 +--- .../bundle/run_as/empty_override/output.txt | 8 +- .../bundle/run_as/empty_override/script | 3 +- .../empty_resources/empty_def/output.txt | 112 +++++++++++-- .../validate/empty_resources/null/output.txt | 112 +++++++++++-- bundle/config/root.go | 157 ++++++++++++------ libs/dyn/merge/merge_test.go | 4 + 7 files changed, 316 insertions(+), 114 deletions(-) diff --git a/acceptance/bundle/deploy/mlops-stacks/output.txt b/acceptance/bundle/deploy/mlops-stacks/output.txt index 1b62cc0093e..61a027c7b7d 100644 --- a/acceptance/bundle/deploy/mlops-stacks/output.txt +++ b/acceptance/bundle/deploy/mlops-stacks/output.txt @@ -25,10 +25,6 @@ This directory contains an ML project based on the default [Databricks MLOps Stacks](https://github.com/databricks/mlops-stacks), >>> [CLI] bundle summary --var catalog_name=main -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 @@ -56,10 +52,6 @@ Resources: URL: (not deployed) >>> [CLI] bundle validate --var catalog_name=main -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 @@ -70,13 +62,9 @@ Workspace: User: [USERNAME] Path: /Workspace/Users/[USERNAME]/.bundle/project_name_[UNIQUE_NAME]/dev -Found 2 warnings +Found 1 warning >>> [CLI] bundle deploy --var catalog_name=main -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 @@ -94,10 +82,6 @@ Files: 26 uploaded, 0 deleted Resources: 8 created, 0 changed, 0 deleted, 0 unchanged >>> [CLI] bundle summary --var catalog_name=main -o json -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 @@ -110,18 +94,10 @@ Warning: unknown field: description } === Assert the batch inference job actually exists -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 @@ -131,10 +107,6 @@ Warning: unknown field: description } >>> [CLI] bundle destroy --var catalog_name=main --auto-approve -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 @@ -150,10 +122,6 @@ All files and directories at the following location will be deleted: /Workspace/ Destroy: 4 deleted >>> [CLI] schemas delete main.project_name_[UNIQUE_NAME] -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 diff --git a/acceptance/bundle/run_as/empty_override/output.txt b/acceptance/bundle/run_as/empty_override/output.txt index 635d9eda9df..4c2f1879167 100644 --- a/acceptance/bundle/run_as/empty_override/output.txt +++ b/acceptance/bundle/run_as/empty_override/output.txt @@ -1,2 +1,6 @@ - "run_as": { - "run_as": { + +>>> [CLI] bundle validate -o json +{ + "run_as": null, + "job_run_as": null +} diff --git a/acceptance/bundle/run_as/empty_override/script b/acceptance/bundle/run_as/empty_override/script index de89daa9199..de9320c4290 100644 --- a/acceptance/bundle/run_as/empty_override/script +++ b/acceptance/bundle/run_as/empty_override/script @@ -1 +1,2 @@ -$CLI bundle validate -o json | grep run_as +# An empty run_as in a target override removes run_as. +trace $CLI bundle validate -o json | jq '{run_as, job_run_as: .resources.jobs.foo.run_as}' diff --git a/acceptance/bundle/validate/empty_resources/empty_def/output.txt b/acceptance/bundle/validate/empty_resources/empty_def/output.txt index a7b495ca9db..5e4ac031008 100644 --- a/acceptance/bundle/validate/empty_resources/empty_def/output.txt +++ b/acceptance/bundle/validate/empty_resources/empty_def/output.txt @@ -1,70 +1,154 @@ === resources.jobs.rname === +Error: job rname is not defined + at resources.jobs.rname + in databricks.yml:6:11 + { - "jobs": {} + "jobs": { + "rname": null + } } === resources.pipelines.rname === +Error: pipeline rname is not defined + at resources.pipelines.rname + in databricks.yml:6:11 + { - "pipelines": {} + "pipelines": { + "rname": null + } } === resources.models.rname === +Error: model rname is not defined + at resources.models.rname + in databricks.yml:6:11 + { - "models": {} + "models": { + "rname": null + } } === resources.experiments.rname === +Error: experiment rname is not defined + at resources.experiments.rname + in databricks.yml:6:11 + { - "experiments": {} + "experiments": { + "rname": null + } } === resources.registered_models.rname === +Error: registered_model rname is not defined + at resources.registered_models.rname + in databricks.yml:6:11 + { - "registered_models": {} + "registered_models": { + "rname": null + } } === resources.quality_monitors.rname === +Error: quality_monitor rname is not defined + at resources.quality_monitors.rname + in databricks.yml:6:11 + { - "quality_monitors": {} + "quality_monitors": { + "rname": null + } } === resources.schemas.rname === +Error: schema rname is not defined + at resources.schemas.rname + in databricks.yml:6:11 + { - "schemas": {} + "schemas": { + "rname": null + } } === resources.volumes.rname === +Error: volume rname is not defined + at resources.volumes.rname + in databricks.yml:6:11 + { - "volumes": {} + "volumes": { + "rname": null + } } === resources.clusters.rname === +Error: cluster rname is not defined + at resources.clusters.rname + in databricks.yml:6:11 + { - "clusters": {} + "clusters": { + "rname": null + } } === resources.dashboards.rname === +Error: dashboard rname is not defined + at resources.dashboards.rname + in databricks.yml:6:11 + { - "dashboards": {} + "dashboards": { + "rname": null + } } === resources.apps.rname === +Error: app rname is not defined + at resources.apps.rname + in databricks.yml:6:11 + { - "apps": {} + "apps": { + "rname": null + } } === resources.sql_warehouses.rname === +Error: sql_warehouse rname is not defined + at resources.sql_warehouses.rname + in databricks.yml:6:11 + { - "sql_warehouses": {} + "sql_warehouses": { + "rname": null + } } === resources.secret_scopes.rname === +Error: secret_scope rname is not defined + at resources.secret_scopes.rname + in databricks.yml:6:11 + { - "secret_scopes": {} + "secret_scopes": { + "rname": null + } } === resources.alerts.rname === +Error: alert rname is not defined + at resources.alerts.rname + in databricks.yml:6:11 + { - "alerts": {} + "alerts": { + "rname": null + } } diff --git a/acceptance/bundle/validate/empty_resources/null/output.txt b/acceptance/bundle/validate/empty_resources/null/output.txt index a7b495ca9db..1c64bbb7176 100644 --- a/acceptance/bundle/validate/empty_resources/null/output.txt +++ b/acceptance/bundle/validate/empty_resources/null/output.txt @@ -1,70 +1,154 @@ === resources.jobs.rname === +Error: job rname is not defined + at resources.jobs.rname + in databricks.yml:6:12 + { - "jobs": {} + "jobs": { + "rname": null + } } === resources.pipelines.rname === +Error: pipeline rname is not defined + at resources.pipelines.rname + in databricks.yml:6:12 + { - "pipelines": {} + "pipelines": { + "rname": null + } } === resources.models.rname === +Error: model rname is not defined + at resources.models.rname + in databricks.yml:6:12 + { - "models": {} + "models": { + "rname": null + } } === resources.experiments.rname === +Error: experiment rname is not defined + at resources.experiments.rname + in databricks.yml:6:12 + { - "experiments": {} + "experiments": { + "rname": null + } } === resources.registered_models.rname === +Error: registered_model rname is not defined + at resources.registered_models.rname + in databricks.yml:6:12 + { - "registered_models": {} + "registered_models": { + "rname": null + } } === resources.quality_monitors.rname === +Error: quality_monitor rname is not defined + at resources.quality_monitors.rname + in databricks.yml:6:12 + { - "quality_monitors": {} + "quality_monitors": { + "rname": null + } } === resources.schemas.rname === +Error: schema rname is not defined + at resources.schemas.rname + in databricks.yml:6:12 + { - "schemas": {} + "schemas": { + "rname": null + } } === resources.volumes.rname === +Error: volume rname is not defined + at resources.volumes.rname + in databricks.yml:6:12 + { - "volumes": {} + "volumes": { + "rname": null + } } === resources.clusters.rname === +Error: cluster rname is not defined + at resources.clusters.rname + in databricks.yml:6:12 + { - "clusters": {} + "clusters": { + "rname": null + } } === resources.dashboards.rname === +Error: dashboard rname is not defined + at resources.dashboards.rname + in databricks.yml:6:12 + { - "dashboards": {} + "dashboards": { + "rname": null + } } === resources.apps.rname === +Error: app rname is not defined + at resources.apps.rname + in databricks.yml:6:12 + { - "apps": {} + "apps": { + "rname": null + } } === resources.sql_warehouses.rname === +Error: sql_warehouse rname is not defined + at resources.sql_warehouses.rname + in databricks.yml:6:12 + { - "sql_warehouses": {} + "sql_warehouses": { + "rname": null + } } === resources.secret_scopes.rname === +Error: secret_scope rname is not defined + at resources.secret_scopes.rname + in databricks.yml:6:12 + { - "secret_scopes": {} + "secret_scopes": { + "rname": null + } } === resources.alerts.rname === +Error: alert rname is not defined + at resources.alerts.rname + in databricks.yml:6:12 + { - "alerts": {} + "alerts": { + "rname": null + } } diff --git a/bundle/config/root.go b/bundle/config/root.go index 5d1e1e36418..49f8c0379ea 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -145,7 +145,7 @@ func LoadFromBytes(path string, raw []byte) (*Root, diag.Diagnostics) { r.withNulls = v // A null has nothing to remove in a file on its own, so it is dropped, except where - // it declares an empty target or variable. + // it declares an empty entry, e.g. a target. nv, err := addDeclarations(dyn.DropNulls(v), dyn.NilValue, v) if err != nil { return nil, diags.Extend(diag.Errorf("failed to load %s: %v", path, err)) @@ -337,25 +337,39 @@ func (r *Root) Merge(other *Root) error { }) } -// declarationSections are the sections where a null entry declares an empty entry -// if there is no entry to remove, e.g. an empty "targets.dev:" declares target dev. -var declarationSections = []string{"targets", "environments", "variables"} +// declarationSections are the maps in which a null entry declares an empty entry if +// there is no entry to remove: an empty "targets.dev:" declares target dev, and an +// empty "resources.jobs.foo:" is reported as an undefined job. The resources section +// holds one such map per resource type. +var declarationSections = []string{"targets", "environments", "variables", "artifacts", "scripts"} -// addDeclarations returns v with the null entries of the declaration sections of -// nulls that base does not have. +// addDeclarations returns v with the null entries of the declaration sections in nulls +// that have no value to remove in base. func addDeclarations(v, base, nulls dyn.Value) (dyn.Value, error) { + var sections []dyn.Path for _, section := range declarationSections { - entries, ok := nulls.Get(section).AsMap() - if !ok { + sections = append(sections, dyn.NewPath(dyn.Key(section))) + } + if resources, ok := nulls.Get("resources").AsMap(); ok { + for _, typ := range resources.Keys() { + sections = append(sections, dyn.NewPath(dyn.Key("resources"), dyn.Key(typ.MustString()))) + } + } + + for _, section := range sections { + entries, err := dyn.GetByPath(nulls, section) + if err != nil || entries.Kind() != dyn.KindMap { continue } - for _, pair := range entries.Pairs() { - name := pair.Key.MustString() - if pair.Value.Kind() != dyn.KindNil || base.Get(section).Get(name).IsValid() { + for _, pair := range entries.MustMap().Pairs() { + if pair.Value.Kind() != dyn.KindNil { continue } - var err error - v, err = dyn.SetByPath(v, dyn.NewPath(dyn.Key(section), dyn.Key(name)), pair.Value) + p := section.Append(dyn.Key(pair.Key.MustString())) + if bv, err := dyn.GetByPath(base, p); err == nil && bv.Kind() != dyn.KindNil { + continue + } + v, err = dyn.SetByPath(v, p, pair.Value) if err != nil { return dyn.InvalidValue, err } @@ -406,23 +420,51 @@ func setNulls(dst, src dyn.Value) (dyn.Value, error) { return dst, nil } +// setOrRemove returns v with the value at p set to nv without its nulls, or removed if +// nv is null. +func setOrRemove(v dyn.Value, p dyn.Path, nv dyn.Value) (dyn.Value, error) { + if nv.Kind() == dyn.KindNil { + return removeByPath(v, p) + } + return dyn.SetByPath(v, p, dyn.DropNulls(nv)) +} + +// removeByPath returns v without the value at p, if there is one. +func removeByPath(v dyn.Value, p dyn.Path) (dyn.Value, error) { + return dyn.Walk(v, func(vp dyn.Path, v dyn.Value) (dyn.Value, error) { + switch { + case vp.Equal(p): + return v, dyn.ErrDrop + case p.HasPrefix(vp): + return v, nil + default: + return v, dyn.ErrSkip + } + }) +} + func mergeField(rv, ov dyn.Value, name string) (dyn.Value, error) { path := dyn.NewPath(dyn.Key(name)) reference, _ := dyn.GetByPath(rv, path) override, _ := dyn.GetByPath(ov, path) + // A null override removes the field. + if override.Kind() == dyn.KindNil { + return removeByPath(rv, path) + } + // Merge the override into the reference. var out dyn.Value var err error if reference.IsValid() && override.IsValid() { - out, err = merge.MergeKeepingNulls(reference, override) + out, err = merge.MergeRemovingNulls(reference, override) if err != nil { return dyn.InvalidValue, err } } else if reference.IsValid() { out = reference } else if override.IsValid() { - out = override + out = dyn.DropNulls(override) } else { return rv, nil } @@ -443,13 +485,18 @@ func (r *Root) MergeTargetOverrides(name string) error { return err } - // A null in the target removes the value from the root. The nulls are set here, and - // removed with what they override at the end. + // A null in the target removes the value from the root; the target as merged so + // far has lost its nulls, so they are set again. target, err = setNulls(target, r.targetWithNulls(name)) if err != nil { return fmt.Errorf("failed to apply the nulls of target=%s: %w", name, err) } + root, target, err = mergeKeyedSequencesWithNulls(root, target) + if err != nil { + return err + } + // Merge fields that can be merged 1:1. for _, f := range []string{ "bundle", @@ -473,7 +520,7 @@ func (r *Root) MergeTargetOverrides(name string) error { vDefault := variable.Get("default") if vDefault.Kind() != dyn.KindInvalid { defaultPath := varPath.Append(dyn.Key("default")) - root, err = dyn.SetByPath(root, defaultPath, vDefault) + root, err = setOrRemove(root, defaultPath, vDefault) if err != nil { return root, err } @@ -489,7 +536,7 @@ func (r *Root) MergeTargetOverrides(name string) error { vLookup := variable.Get("lookup") if vLookup.Kind() != dyn.KindInvalid { lookupPath := varPath.Append(dyn.Key("lookup")) - root, err = dyn.SetByPath(root, lookupPath, vLookup) + root, err = setOrRemove(root, lookupPath, vLookup) if err != nil { return root, err } @@ -511,7 +558,7 @@ func (r *Root) MergeTargetOverrides(name string) error { // Merge `run_as`. This field must be overwritten if set, not merged. if v := target.Get("run_as"); v.Kind() != dyn.KindInvalid { - root, err = dyn.Set(root, "run_as", v) + root, err = setOrRemove(root, dyn.NewPath(dyn.Key("run_as")), v) if err != nil { return err } @@ -527,7 +574,7 @@ func (r *Root) MergeTargetOverrides(name string) error { // Merge `mode`. This field must be overwritten if set, not merged. if v := target.Get("mode"); v.Kind() != dyn.KindInvalid { - root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("mode")), v) + root, err = setOrRemove(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("mode")), v) if err != nil { return err } @@ -535,7 +582,7 @@ func (r *Root) MergeTargetOverrides(name string) error { // Merge `cluster_id`. This field must be overwritten if set, not merged. if v := target.Get("cluster_id"); v.Kind() != dyn.KindInvalid { - root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("cluster_id")), v) + root, err = setOrRemove(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("cluster_id")), v) if err != nil { return err } @@ -548,27 +595,22 @@ func (r *Root) MergeTargetOverrides(name string) error { ref = dyn.V(map[string]dyn.Value{}) } - // Merge the override into the reference. - out, err := merge.MergeKeepingNulls(ref, v) - if err != nil { - return err + // Merge the override into the reference. A null override removes it. + out := v + if v.Kind() != dyn.KindNil { + out, err = merge.MergeRemovingNulls(ref, v) + if err != nil { + return err + } } // Set the merged value. - root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("git")), out) + root, err = setOrRemove(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("git")), out) if err != nil { return err } } - root, err = mergeKeyedSequencesWithNulls(root) - if err != nil { - return err - } - - // The nulls have replaced the values they remove. Remaining nulls had nothing to - // remove and are not added either. - root = dyn.DropNulls(root) r.withNulls = dyn.InvalidValue // Convert normalized configuration tree to typed configuration. @@ -605,34 +647,49 @@ func pipelineClusterLabel(v dyn.Value) string { return strings.ToLower(s) } -// mergeKeyedSequencesWithNulls merges the elements with the same key of the keyed -// sequences that hold nulls, so that a null removes the value from the element it -// overrides. Resources and elements can still be ${...} references here, which are -// left as they are. -func mergeKeyedSequencesWithNulls(root dyn.Value) (dyn.Value, error) { +// mergeKeyedSequencesWithNulls merges the keyed sequences of the target that hold nulls +// with the same sequence of the root, element by element with the same key, so that a +// null removes the value from the element it overrides. The merged sequence replaces the +// one in the target and is removed from the root. Resources and elements can still be +// ${...} references here, which are left as they are. +func mergeKeyedSequencesWithNulls(root, target dyn.Value) (dyn.Value, dyn.Value, error) { for _, ks := range keyedSequences { - resources, ok := root.Get("resources").Get(ks.resourceType).AsMap() + resources, ok := target.Get("resources").Get(ks.resourceType).AsMap() if !ok { continue } for _, pair := range resources.Pairs() { - field := pair.Value.Get(ks.field) - seq, ok := field.AsSequence() - if !ok || !hasNulls(field) || slices.ContainsFunc(seq, isNotMap) { + override := pair.Value.Get(ks.field) + seq, ok := override.AsSequence() + if !ok || !hasNulls(override) || slices.ContainsFunc(seq, isNotMap) { continue } - merged, err := merge.ElementsByKeyKeepingNulls(ks.key, ks.keyFunc)(dyn.EmptyPath, field) + + p := dyn.NewPath(dyn.Key("resources"), dyn.Key(ks.resourceType), dyn.Key(pair.Key.MustString()), dyn.Key(ks.field)) + reference, _ := dyn.GetByPath(root, p) + if reference.IsValid() { + base, ok := reference.AsSequence() + if !ok || slices.ContainsFunc(base, isNotMap) { + continue + } + seq = slices.Concat(base, seq) + } + + merged, err := merge.ElementsByKeyKeepingNulls(ks.key, ks.keyFunc)(p, dyn.NewValue(seq, override.Locations())) if err != nil { - return dyn.InvalidValue, err + return dyn.InvalidValue, dyn.InvalidValue, err } - p := dyn.NewPath(dyn.Key("resources"), dyn.Key(ks.resourceType), dyn.Key(pair.Key.MustString()), dyn.Key(ks.field)) - root, err = dyn.SetByPath(root, p, merged) + target, err = dyn.SetByPath(target, p, dyn.DropNulls(merged)) if err != nil { - return dyn.InvalidValue, err + return dyn.InvalidValue, dyn.InvalidValue, err + } + root, err = removeByPath(root, p) + if err != nil { + return dyn.InvalidValue, dyn.InvalidValue, err } } } - return root, nil + return root, target, nil } func isNotMap(v dyn.Value) bool { diff --git a/libs/dyn/merge/merge_test.go b/libs/dyn/merge/merge_test.go index 43c655c295a..92f89b0d31f 100644 --- a/libs/dyn/merge/merge_test.go +++ b/libs/dyn/merge/merge_test.go @@ -391,4 +391,8 @@ func TestMergeNullModesNilRoot(t *testing.T) { out, err = MergeKeepingNulls(b, dyn.NilValue) require.NoError(t, err) assert.Equal(t, dyn.KindNil, out.Kind()) + + out, err = MergeRemovingNulls(b, dyn.NilValue) + require.NoError(t, err) + assert.Equal(t, map[string]any{"null": nil, "value": "b"}, out.AsAny()) }