From b809fe38ba32a631d25e67b757b54e9fa433c568 Mon Sep 17 00:00:00 2001 From: "ilya.kuznetsov" Date: Tue, 3 Nov 2026 15:14:57 +0000 Subject: [PATCH 1/5] Fix config-remote-sync writes through scalar YAML parents --- .../config-remote-sync-scalar-parents.md | 1 + .../null_parent/databricks.yml.tmpl | 45 ++++++++++++++ .../null_parent/out.test.toml | 3 + .../config-remote-sync/null_parent/output.txt | 59 +++++++++++++++++++ .../config-remote-sync/null_parent/script | 26 ++++++++ .../config-remote-sync/null_parent/test.toml | 1 + .../databricks.yml.tmpl | 18 ++++++ .../variable_reference_parent/output.txt | 26 ++++---- .../variable_reference_parent/script | 16 +++-- bundle/configsync/patch.go | 50 ++++++++++++++++ bundle/configsync/patch_test.go | 28 +++++++++ bundle/configsync/resolve.go | 8 +-- bundle/configsync/variables.go | 21 ++++--- 13 files changed, 270 insertions(+), 32 deletions(-) create mode 100644 .nextchanges/bundles/config-remote-sync-scalar-parents.md create mode 100644 acceptance/bundle/config-remote-sync/null_parent/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/null_parent/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/null_parent/output.txt create mode 100644 acceptance/bundle/config-remote-sync/null_parent/script create mode 100644 acceptance/bundle/config-remote-sync/null_parent/test.toml diff --git a/.nextchanges/bundles/config-remote-sync-scalar-parents.md b/.nextchanges/bundles/config-remote-sync-scalar-parents.md new file mode 100644 index 00000000000..b88e9757de7 --- /dev/null +++ b/.nextchanges/bundles/config-remote-sync-scalar-parents.md @@ -0,0 +1 @@ +* Fix `bundle config-remote-sync` failing on null YAML containers and changes nested under variable references. diff --git a/acceptance/bundle/config-remote-sync/null_parent/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/null_parent/databricks.yml.tmpl new file mode 100644 index 00000000000..cf26f9405c2 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/null_parent/databricks.yml.tmpl @@ -0,0 +1,45 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +resources: + jobs: + my_job: + name: test-job-$UNIQUE_NAME + job_clusters: + - job_cluster_key: shared + new_cluster: + spark_version: $DEFAULT_SPARK_VERSION + node_type_id: $NODE_TYPE_ID + custom_tags: # Keep this comment when adding tags. + spark_env_vars: null + init_scripts: ~ + autoscale: null + - job_cluster_key: nested + new_cluster: null + tasks: + - task_key: main + job_cluster_key: shared + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/notebook + +targets: + default: + resources: + jobs: + my_job: + job_clusters: + - job_cluster_key: shared + new_cluster: + custom_tags: + existing: preserved + spark_env_vars: + EXISTING: preserved + autoscale: + min_workers: 1 + - job_cluster_key: nested + new_cluster: + spark_version: $DEFAULT_SPARK_VERSION + node_type_id: $NODE_TYPE_ID + num_workers: 1 + custom_tags: + existing: preserved diff --git a/acceptance/bundle/config-remote-sync/null_parent/out.test.toml b/acceptance/bundle/config-remote-sync/null_parent/out.test.toml new file mode 100644 index 00000000000..8c2cb31de53 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/null_parent/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +GOOS.windows = false +EnvMatrix.DMS = ["", "true"] diff --git a/acceptance/bundle/config-remote-sync/null_parent/output.txt b/acceptance/bundle/config-remote-sync/null_parent/output.txt new file mode 100644 index 00000000000..67f00367450 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/null_parent/output.txt @@ -0,0 +1,59 @@ + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Created jobs.my_job +Files: 2 uploaded, 0 deleted +Resources: 1 created, 0 changed, 0 deleted, 0 unchanged + +>>> [CLI] bundle config-remote-sync --save +Detected changes in 1 resource(s): + +Resource: resources.jobs.my_job + job_clusters[job_cluster_key='nested'].new_cluster.custom_tags['123']: add + job_clusters[job_cluster_key='nested'].new_cluster.custom_tags['team']: add + job_clusters[job_cluster_key='shared'].new_cluster.autoscale.max_workers: add + job_clusters[job_cluster_key='shared'].new_cluster.custom_tags['123']: add + job_clusters[job_cluster_key='shared'].new_cluster.custom_tags['team']: add + job_clusters[job_cluster_key='shared'].new_cluster.init_scripts: replace + job_clusters[job_cluster_key='shared'].new_cluster.spark_env_vars['ENV']: add + + + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -12,9 +12,18 @@ + node_type_id: [NODE_TYPE_ID] + custom_tags: # Keep this comment when adding tags. +- spark_env_vars: null +- init_scripts: ~ +- autoscale: null ++ "123": numeric key ++ team: analytics ++ spark_env_vars: ++ ENV: dev ++ init_scripts: ++ - workspace: ++ destination: /Workspace/init.sh ++ autoscale: ++ max_workers: 2 + - job_cluster_key: nested +- new_cluster: null ++ new_cluster: ++ custom_tags: ++ "123": numeric key ++ team: analytics + tasks: + - task_key: main + +>>> [CLI] bundle config-remote-sync --save +No changes detected. + + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.jobs.my_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default + +Destroy: 1 deleted diff --git a/acceptance/bundle/config-remote-sync/null_parent/script b/acceptance/bundle/config-remote-sync/null_parent/script new file mode 100644 index 00000000000..867a78cbfb1 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/null_parent/script @@ -0,0 +1,26 @@ +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve +} +trap cleanup EXIT + +trace $CLI bundle deploy +job_id=$(read_id.py my_job) + +edit_resource.py jobs "$job_id" <<'EOF' +for jc in r["job_clusters"]: + cluster = jc["new_cluster"] + cluster["custom_tags"].update({"team": "analytics", "123": "numeric key"}) + if jc["job_cluster_key"] == "shared": + cluster["spark_env_vars"]["ENV"] = "dev" + cluster["init_scripts"] = [{"workspace": {"destination": "/Workspace/init.sh"}}] + cluster["autoscale"]["max_workers"] = 2 +EOF + +cp databricks.yml databricks.yml.backup +trace $CLI bundle config-remote-sync --save +trace diff.py databricks.yml.backup databricks.yml +rm databricks.yml.backup + +trace $CLI bundle config-remote-sync --save diff --git a/acceptance/bundle/config-remote-sync/null_parent/test.toml b/acceptance/bundle/config-remote-sync/null_parent/test.toml new file mode 100644 index 00000000000..6455d92ed8d --- /dev/null +++ b/acceptance/bundle/config-remote-sync/null_parent/test.toml @@ -0,0 +1 @@ +Ignore = [".databricks", "databricks.yml", "databricks.yml.backup"] diff --git a/acceptance/bundle/config-remote-sync/variable_reference_parent/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/variable_reference_parent/databricks.yml.tmpl index 778f4dc71a8..dd53264ad95 100644 --- a/acceptance/bundle/config-remote-sync/variable_reference_parent/databricks.yml.tmpl +++ b/acceptance/bundle/config-remote-sync/variable_reference_parent/databricks.yml.tmpl @@ -7,6 +7,21 @@ variables: default: spark.speculation: "true" + init_scripts: + type: complex + default: + - workspace: + destination: /Workspace/init.sh + new_cluster: + type: complex + default: + spark_version: $DEFAULT_SPARK_VERSION + node_type_id: $NODE_TYPE_ID + num_workers: 1 + custom_tags: + changed: original + removed: original + targets: default: mode: development @@ -22,6 +37,9 @@ resources: node_type_id: $NODE_TYPE_ID num_workers: 1 spark_conf: ${var.spark_conf} + init_scripts: ${var.init_scripts} + - job_cluster_key: variable_cluster + new_cluster: ${var.new_cluster} tasks: - task_key: main job_cluster_key: shared diff --git a/acceptance/bundle/config-remote-sync/variable_reference_parent/output.txt b/acceptance/bundle/config-remote-sync/variable_reference_parent/output.txt index a4eac5ef990..e7817920a98 100644 --- a/acceptance/bundle/config-remote-sync/variable_reference_parent/output.txt +++ b/acceptance/bundle/config-remote-sync/variable_reference_parent/output.txt @@ -3,15 +3,17 @@ Created jobs.my_job Files: 2 uploaded, 0 deleted Resources: 1 created, 0 changed, 0 deleted, 0 unchanged -=== Inject a spark_conf key under a field defined as a complex variable -spark_conf is set to a variable reference, so a remotely-added key cannot be -written into that scalar node; the change is skipped while unrelated edits still sync. +=== Edit fields under complex variables === Detect and save all changes Detected changes in 1 resource(s): Resource: resources.jobs.my_job + job_clusters[job_cluster_key='shared'].new_cluster.init_scripts[0].workspace.destination: skip job_clusters[job_cluster_key='shared'].new_cluster.spark_conf['spark.executor.memory']: skip + job_clusters[job_cluster_key='variable_cluster'].new_cluster.custom_tags['changed']: skip + job_clusters[job_cluster_key='variable_cluster'].new_cluster.custom_tags['removed']: skip + job_clusters[job_cluster_key='variable_cluster'].new_cluster.spark_env_vars: skip max_concurrent_runs: replace @@ -21,7 +23,7 @@ Resource: resources.jobs.my_job >>> diff.py databricks.yml.backup databricks.yml --- databricks.yml.backup +++ databricks.yml -@@ -15,5 +15,5 @@ +@@ -30,5 +30,5 @@ jobs: my_job: - max_concurrent_runs: 1 @@ -34,20 +36,22 @@ Resource: resources.jobs.my_job >>> cat [OUTPUT_DIR]/out.requests.txt { "save": true, - "changes_total": 2, - "add_count": 1, - "replace_count": 1, + "changes_total": 6, + "add_count": 2, + "replace_count": 3, + "remove_count": 1, "resource_changes": [ { "resource_type": "jobs", - "changes_count": 2, - "add_count": 1, - "replace_count": 1 + "changes_count": 6, + "add_count": 2, + "replace_count": 3, + "remove_count": 1 } ], "files_changed_count": 1, "files_written_count": 1, - "skipped_changes_count": 1, + "skipped_changes_count": 5, "state_serial": 1, "state_lineage": "[UUID]", "state_source": "local", diff --git a/acceptance/bundle/config-remote-sync/variable_reference_parent/script b/acceptance/bundle/config-remote-sync/variable_reference_parent/script index 00347b093f6..5e49b27032b 100644 --- a/acceptance/bundle/config-remote-sync/variable_reference_parent/script +++ b/acceptance/bundle/config-remote-sync/variable_reference_parent/script @@ -12,15 +12,19 @@ trap cleanup EXIT $CLI bundle deploy job_id="$(read_id.py my_job)" -title "Inject a spark_conf key under a field defined as a complex variable" +title "Edit fields under complex variables" echo -echo "spark_conf is set to a variable reference, so a remotely-added key cannot be" -echo "written into that scalar node; the change is skipped while unrelated edits still sync." edit_resource.py jobs $job_id <<'EOF' r["max_concurrent_runs"] = 5 -for jc in r.get("job_clusters", []): - jc["new_cluster"]["spark_conf"]["spark.executor.memory"] = "4g" +cluster = r["job_clusters"][0]["new_cluster"] +cluster["spark_conf"]["spark.executor.memory"] = "4g" +cluster["init_scripts"][0]["workspace"]["destination"] = "/Workspace/updated-init.sh" + +cluster = r["job_clusters"][1]["new_cluster"] +cluster["spark_env_vars"] = {"ADDED": "value"} +cluster["custom_tags"]["changed"] = "updated" +del cluster["custom_tags"]["removed"] EOF title "Detect and save all changes" @@ -36,5 +40,5 @@ rm databricks.yml.backup title "Telemetry" echo # The engine name is dropped because both EnvMatrix variants share this output. -# skipped_changes_count reflects the spark_conf change skipped under a variable reference. +# skipped_changes_count includes changes under both immediate and distant variable ancestors. trace cat "$OUT_REQUESTS" | jq 'select(has("path") and .path == "/telemetry-ext") | .body.protoLogs[] | fromjson | .entry.databricks_cli_log.bundle_config_remote_sync_event | select(. != null) | del(.engine)' diff --git a/bundle/configsync/patch.go b/bundle/configsync/patch.go index 573b5d2c1ba..245e461ca71 100644 --- a/bundle/configsync/patch.go +++ b/bundle/configsync/patch.go @@ -13,6 +13,8 @@ import ( "strings" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/yamlloader" "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/structs/structpath" "github.com/palantir/pkg/yamlpatch/gopkgv3yamlpatcher" @@ -115,6 +117,16 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( Value: fieldChange.Change.Value, }}) + if patchErr != nil { + patch, err := nullParentPatch(content, fieldPathCandidate, path, fieldChange.Change.Value) + if err != nil { + return nil, err + } + if len(patch) > 0 { + modifiedContent, patchErr = patcher.Apply(content, patch) + } + } + // Collect parent path errors for later retry if patchErr != nil && isParentPathError(patchErr) { if missingPath, extractErr := extractMissingPath(patchErr); extractErr == nil { @@ -174,6 +186,44 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( return content, nil } +// nullParentPatch replaces a null ancestor with the containers needed for an add. +func nullParentPatch(content []byte, fieldPath string, path yamlpatch.Path, value any) (yamlpatch.Patch, error) { + config, err := yamlloader.LoadYAML("", bytes.NewReader(content)) + if err != nil { + return nil, err + } + node, err := structpath.ParsePattern(fieldPath) + if err != nil { + return nil, err + } + for ; node.Parent() != nil; node = node.Parent() { + key, isKey := node.StringKey() + index, isIndex := node.Index() + switch { + case isKey: + value = map[string]any{key: value} + case node.BracketStar(), isIndex && index == 0: + value = []any{value} + default: + // Only the first element can be added to an empty sequence. + return nil, nil + } + + parent := node.Parent() + v, ok := preResolvedValueAt(config, parent.String()) + if !ok { + continue + } + if v.Kind() != dyn.KindNil { + return nil, nil + } + // yamlpatch includes the document node as an empty first component. + parentPath := path[:parent.Len()+1] + return yamlpatch.Patch{{Type: yamlpatch.OperationReplace, Path: parentPath, Value: value}}, nil + } + return nil, nil +} + // isParentPathError checks if error indicates missing parent path. func isParentPathError(err error) bool { if err == nil { diff --git a/bundle/configsync/patch_test.go b/bundle/configsync/patch_test.go index ef0030f95f8..32e305adb02 100644 --- a/bundle/configsync/patch_test.go +++ b/bundle/configsync/patch_test.go @@ -437,3 +437,31 @@ func TestBuildNestedMaps(t *testing.T) { } assert.Equal(t, expected, result) } + +func TestApplyChangeScalarParents(t *testing.T) { + for _, tc := range []struct { + name string + content string + path string + want string + }{ + {"string", "parent: text\n", "parent.key", ""}, + {"boolean", "parent: false\n", "parent.key", ""}, + {"nonzero index", "parent: null\n", "parent[1].key", ""}, + {"append", "parent: null\n", "parent[*]", "parent: [value]"}, + {"nested sequence", "parent: ~\n", "parent[0].key", "parent: [{key: value}]"}, + } { + t.Run(tc.name, func(t *testing.T) { + result, err := applyChange(t.Context(), []byte(tc.content), FieldChange{ + WritePath: tc.path, + Change: &ConfigChangeDesc{Operation: OperationAdd, Value: "value"}, + }) + if tc.want == "" { + assert.ErrorContains(t, err, "scalar can not be a container") + return + } + require.NoError(t, err) + assert.YAMLEq(t, tc.want, string(result)) + }) + } +} diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index cdad5182336..3859714478d 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -193,13 +193,13 @@ func adjustArrayIndex(path *structpath.PatternNode, scope string, operations map // ResolveChanges resolves selectors and computes field path candidates for each change. // -// A change that cannot be attributed to a single source location, or whose parent +// A change that cannot be attributed to a single source location, or whose ancestor // in the config is written as a variable reference, is left unapplied rather than // written to a guessed or non-existent location. The count of those is returned so // the caller can record it: the command runs unattended, so a change that silently // disappears looks identical to one that was written. preResolved is the merged // config with ${...} references still literal, used to detect variable-reference -// parents. +// ancestors. func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes, preResolved dyn.Value) ([]FieldChange, int, error) { var result []FieldChange skipped := 0 @@ -278,8 +278,8 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes // to place the key or index under. Leave it unapplied, and record the skip on // the shared change so the output reports "skip" rather than a write that never // happens (matching the reclassification below). - if parentIsVariableReference(preResolved, mergedIndexPath.String()) { - log.Debugf(ctx, "config-remote-sync: skipping %s: parent is a variable reference", fullPath) + if ancestorIsVariableReference(preResolved, mergedIndexPath.String()) { + log.Debugf(ctx, "config-remote-sync: skipping %s: ancestor is a variable reference", fullPath) configChange.Operation = OperationSkip skipped++ continue diff --git a/bundle/configsync/variables.go b/bundle/configsync/variables.go index 2a0ac89cc92..211a6deb8d3 100644 --- a/bundle/configsync/variables.go +++ b/bundle/configsync/variables.go @@ -567,26 +567,25 @@ func preResolvedValueAt(preResolved dyn.Value, fieldPath string) (dyn.Value, boo return v, true } -// parentIsVariableReference reports whether the parent of fieldPath resolves to a +// ancestorIsVariableReference reports whether an ancestor of fieldPath resolves to a // scalar variable reference in the pre-resolved config (e.g. spark_conf: // ${var.spark_conf}). A nested key or index cannot be written into such a scalar, // so config-remote-sync skips the change. fieldPath is a merged-index path, the // same space as FieldChange.originalPath. -func parentIsVariableReference(preResolved dyn.Value, fieldPath string) bool { +func ancestorIsVariableReference(preResolved dyn.Value, fieldPath string) bool { node, err := structpath.ParsePattern(fieldPath) if err != nil { return false } - parent := node.Parent() - if parent == nil { - return false - } - v, ok := preResolvedValueAt(preResolved, parent.String()) - if !ok { - return false + for parent := node.Parent(); parent != nil; parent = parent.Parent() { + v, ok := preResolvedValueAt(preResolved, parent.String()) + if !ok { + continue + } + s, ok := v.AsString() + return ok && dynvar.ContainsVariableReference(s) } - s, ok := v.AsString() - return ok && dynvar.ContainsVariableReference(s) + return false } // sequenceSiblings returns the sibling elements of the parent sequence when From cffc0c24b5c2e7f4469ba08dc0dcdef1eeacc9fc Mon Sep 17 00:00:00 2001 From: "ilya.kuznetsov" Date: Tue, 3 Nov 2026 15:18:06 +0000 Subject: [PATCH 2/5] Link scalar-parent sync changelog to PR --- .nextchanges/bundles/config-remote-sync-scalar-parents.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.nextchanges/bundles/config-remote-sync-scalar-parents.md b/.nextchanges/bundles/config-remote-sync-scalar-parents.md index b88e9757de7..2d7a39ca24d 100644 --- a/.nextchanges/bundles/config-remote-sync-scalar-parents.md +++ b/.nextchanges/bundles/config-remote-sync-scalar-parents.md @@ -1 +1 @@ -* Fix `bundle config-remote-sync` failing on null YAML containers and changes nested under variable references. +* Fix `bundle config-remote-sync` failing on null YAML containers and changes nested under variable references. ([#7323](https://github.com/databricks/cli/pull/7323)) From bb70c3dab3d1b79628a35e383059e91c018d56bb Mon Sep 17 00:00:00 2001 From: "ilya.kuznetsov" Date: Tue, 3 Nov 2026 15:33:22 +0000 Subject: [PATCH 3/5] Inspect YAML nodes directly for null ancestor patches --- .../config-remote-sync-scalar-parents.md | 1 - bundle/configsync/patch.go | 81 ++++++++++++------- bundle/configsync/patch_test.go | 5 ++ 3 files changed, 59 insertions(+), 28 deletions(-) delete mode 100644 .nextchanges/bundles/config-remote-sync-scalar-parents.md diff --git a/.nextchanges/bundles/config-remote-sync-scalar-parents.md b/.nextchanges/bundles/config-remote-sync-scalar-parents.md deleted file mode 100644 index 2d7a39ca24d..00000000000 --- a/.nextchanges/bundles/config-remote-sync-scalar-parents.md +++ /dev/null @@ -1 +0,0 @@ -* Fix `bundle config-remote-sync` failing on null YAML containers and changes nested under variable references. ([#7323](https://github.com/databricks/cli/pull/7323)) diff --git a/bundle/configsync/patch.go b/bundle/configsync/patch.go index 245e461ca71..7a23918793a 100644 --- a/bundle/configsync/patch.go +++ b/bundle/configsync/patch.go @@ -13,8 +13,6 @@ import ( "strings" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/libs/dyn" - "github.com/databricks/cli/libs/dyn/yamlloader" "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/structs/structpath" "github.com/palantir/pkg/yamlpatch/gopkgv3yamlpatcher" @@ -118,7 +116,7 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( }}) if patchErr != nil { - patch, err := nullParentPatch(content, fieldPathCandidate, path, fieldChange.Change.Value) + patch, err := nullAncestorPatch(content, fieldPathCandidate, path, fieldChange.Change.Value) if err != nil { return nil, err } @@ -186,39 +184,68 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( return content, nil } -// nullParentPatch replaces a null ancestor with the containers needed for an add. -func nullParentPatch(content []byte, fieldPath string, path yamlpatch.Path, value any) (yamlpatch.Patch, error) { - config, err := yamlloader.LoadYAML("", bytes.NewReader(content)) - if err != nil { - return nil, err - } - node, err := structpath.ParsePattern(fieldPath) - if err != nil { +// nullAncestorPatch replaces a null ancestor with the containers needed for an add. +// It returns nil when no null ancestor can be replaced safely. +func nullAncestorPatch(content []byte, fieldPath string, path yamlpatch.Path, value any) (yamlpatch.Patch, error) { + var doc yaml.Node + if err := yaml.Unmarshal(content, &doc); err != nil { return nil, err } - for ; node.Parent() != nil; node = node.Parent() { - key, isKey := node.StringKey() - index, isIndex := node.Index() - switch { - case isKey: - value = map[string]any{key: value} - case node.BracketStar(), isIndex && index == 0: - value = []any{value} + current := &doc + for i, part := range path[:len(path)-1] { + switch current.Kind { + case yaml.DocumentNode: + if len(current.Content) != 1 { + return nil, nil + } + current = current.Content[0] + case yaml.MappingNode: + var child *yaml.Node + for j := 0; j+1 < len(current.Content); j += 2 { + if current.Content[j].Value == part { + child = current.Content[j+1] + break + } + } + if child == nil { + return nil, nil + } + current = child + case yaml.SequenceNode: + index, err := strconv.Atoi(part) + if err != nil || index < 0 || index >= len(current.Content) { + return nil, nil //nolint:nilerr // keep the original patch error for unsupported paths + } + current = current.Content[index] default: - // Only the first element can be added to an empty sequence. return nil, nil } - - parent := node.Parent() - v, ok := preResolvedValueAt(config, parent.String()) - if !ok { + // Replacing anchored content could alter aliases or leave dangling references. + if current.Anchor != "" { + return nil, nil + } + if current.Kind != yaml.ScalarNode || current.ShortTag() != "!!null" { continue } - if v.Kind() != dyn.KindNil { - return nil, nil + pattern, err := structpath.ParsePattern(fieldPath) + if err != nil { + return nil, err + } + for ; pattern.Len() > i; pattern = pattern.Parent() { + key, isKey := pattern.StringKey() + index, isIndex := pattern.Index() + switch { + case isKey: + value = map[string]any{key: value} + case pattern.BracketStar(), isIndex && index == 0: + value = []any{value} + default: + // Only the first element can be added to an empty sequence. + return nil, nil + } } // yamlpatch includes the document node as an empty first component. - parentPath := path[:parent.Len()+1] + parentPath := path[:i+1] return yamlpatch.Patch{{Type: yamlpatch.OperationReplace, Path: parentPath, Value: value}}, nil } return nil, nil diff --git a/bundle/configsync/patch_test.go b/bundle/configsync/patch_test.go index 32e305adb02..c9949c5f312 100644 --- a/bundle/configsync/patch_test.go +++ b/bundle/configsync/patch_test.go @@ -446,7 +446,12 @@ func TestApplyChangeScalarParents(t *testing.T) { want string }{ {"string", "parent: text\n", "parent.key", ""}, + {"quoted null", "parent: \"null\"\n", "parent.key", ""}, + {"empty string", "parent: \"\"\n", "parent.key", ""}, {"boolean", "parent: false\n", "parent.key", ""}, + {"anchored null", "parent: &shared null\nother: *shared\n", "parent.key", ""}, + {"aliased null", "other: &shared null\nparent: *shared\n", "parent.key", ""}, + {"anchored container", "parent: &shared {tags: null}\nother: *shared\n", "parent.tags.key", ""}, {"nonzero index", "parent: null\n", "parent[1].key", ""}, {"append", "parent: null\n", "parent[*]", "parent: [value]"}, {"nested sequence", "parent: ~\n", "parent[0].key", "parent: [{key: value}]"}, From 4820d19c22b269e7971a6815b885663acc841e64 Mon Sep 17 00:00:00 2001 From: "ilya.kuznetsov" Date: Tue, 3 Nov 2026 18:37:33 +0000 Subject: [PATCH 4/5] Preserve null-parent comments during config sync --- .../null_parent/databricks.yml.tmpl | 6 +- .../config-remote-sync/null_parent/output.txt | 12 +- .../config-remote-sync/null_parent/script | 1 + .../variable_reference_parent/output.txt | 11 +- .../variable_reference_parent/script | 1 + bundle/configsync/patch.go | 116 +++++++++++------- bundle/configsync/patch_test.go | 24 ++++ 7 files changed, 110 insertions(+), 61 deletions(-) diff --git a/acceptance/bundle/config-remote-sync/null_parent/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/null_parent/databricks.yml.tmpl index cf26f9405c2..0d083a722be 100644 --- a/acceptance/bundle/config-remote-sync/null_parent/databricks.yml.tmpl +++ b/acceptance/bundle/config-remote-sync/null_parent/databricks.yml.tmpl @@ -11,11 +11,11 @@ resources: spark_version: $DEFAULT_SPARK_VERSION node_type_id: $NODE_TYPE_ID custom_tags: # Keep this comment when adding tags. - spark_env_vars: null + spark_env_vars: null # Keep the environment comment. init_scripts: ~ - autoscale: null + autoscale: ~ # Keep the autoscale comment. - job_cluster_key: nested - new_cluster: null + new_cluster: null # Keep the cluster comment through both tag additions. tasks: - task_key: main job_cluster_key: shared diff --git a/acceptance/bundle/config-remote-sync/null_parent/output.txt b/acceptance/bundle/config-remote-sync/null_parent/output.txt index 67f00367450..a4c0786e1a1 100644 --- a/acceptance/bundle/config-remote-sync/null_parent/output.txt +++ b/acceptance/bundle/config-remote-sync/null_parent/output.txt @@ -25,21 +25,21 @@ Resource: resources.jobs.my_job @@ -12,9 +12,18 @@ node_type_id: [NODE_TYPE_ID] custom_tags: # Keep this comment when adding tags. -- spark_env_vars: null +- spark_env_vars: null # Keep the environment comment. - init_scripts: ~ -- autoscale: null +- autoscale: ~ # Keep the autoscale comment. + "123": numeric key + team: analytics -+ spark_env_vars: ++ spark_env_vars: # Keep the environment comment. + ENV: dev + init_scripts: + - workspace: + destination: /Workspace/init.sh -+ autoscale: ++ autoscale: # Keep the autoscale comment. + max_workers: 2 - job_cluster_key: nested -- new_cluster: null -+ new_cluster: +- new_cluster: null # Keep the cluster comment through both tag additions. ++ new_cluster: # Keep the cluster comment through both tag additions. + custom_tags: + "123": numeric key + team: analytics diff --git a/acceptance/bundle/config-remote-sync/null_parent/script b/acceptance/bundle/config-remote-sync/null_parent/script index 867a78cbfb1..b5fd36fb4c5 100644 --- a/acceptance/bundle/config-remote-sync/null_parent/script +++ b/acceptance/bundle/config-remote-sync/null_parent/script @@ -8,6 +8,7 @@ trap cleanup EXIT trace $CLI bundle deploy job_id=$(read_id.py my_job) +# Target overrides supply existing keys; new keys route to the null parents in the base config. edit_resource.py jobs "$job_id" <<'EOF' for jc in r["job_clusters"]: cluster = jc["new_cluster"] diff --git a/acceptance/bundle/config-remote-sync/variable_reference_parent/output.txt b/acceptance/bundle/config-remote-sync/variable_reference_parent/output.txt index e7817920a98..263bb5235b4 100644 --- a/acceptance/bundle/config-remote-sync/variable_reference_parent/output.txt +++ b/acceptance/bundle/config-remote-sync/variable_reference_parent/output.txt @@ -11,6 +11,7 @@ Detected changes in 1 resource(s): Resource: resources.jobs.my_job job_clusters[job_cluster_key='shared'].new_cluster.init_scripts[0].workspace.destination: skip job_clusters[job_cluster_key='shared'].new_cluster.spark_conf['spark.executor.memory']: skip + job_clusters[job_cluster_key='variable_cluster'].new_cluster.custom_tags['added']: skip job_clusters[job_cluster_key='variable_cluster'].new_cluster.custom_tags['changed']: skip job_clusters[job_cluster_key='variable_cluster'].new_cluster.custom_tags['removed']: skip job_clusters[job_cluster_key='variable_cluster'].new_cluster.spark_env_vars: skip @@ -36,22 +37,22 @@ Resource: resources.jobs.my_job >>> cat [OUTPUT_DIR]/out.requests.txt { "save": true, - "changes_total": 6, - "add_count": 2, + "changes_total": 7, + "add_count": 3, "replace_count": 3, "remove_count": 1, "resource_changes": [ { "resource_type": "jobs", - "changes_count": 6, - "add_count": 2, + "changes_count": 7, + "add_count": 3, "replace_count": 3, "remove_count": 1 } ], "files_changed_count": 1, "files_written_count": 1, - "skipped_changes_count": 5, + "skipped_changes_count": 6, "state_serial": 1, "state_lineage": "[UUID]", "state_source": "local", diff --git a/acceptance/bundle/config-remote-sync/variable_reference_parent/script b/acceptance/bundle/config-remote-sync/variable_reference_parent/script index 5e49b27032b..c49921acfe5 100644 --- a/acceptance/bundle/config-remote-sync/variable_reference_parent/script +++ b/acceptance/bundle/config-remote-sync/variable_reference_parent/script @@ -23,6 +23,7 @@ cluster["init_scripts"][0]["workspace"]["destination"] = "/Workspace/updated-ini cluster = r["job_clusters"][1]["new_cluster"] cluster["spark_env_vars"] = {"ADDED": "value"} +cluster["custom_tags"]["added"] = "new" cluster["custom_tags"]["changed"] = "updated" del cluster["custom_tags"]["removed"] EOF diff --git a/bundle/configsync/patch.go b/bundle/configsync/patch.go index 7a23918793a..fe22c961c59 100644 --- a/bundle/configsync/patch.go +++ b/bundle/configsync/patch.go @@ -116,12 +116,12 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( }}) if patchErr != nil { - patch, err := nullAncestorPatch(content, fieldPathCandidate, path, fieldChange.Change.Value) + patched, err := addThroughNullAncestor(content, fieldPathCandidate, fieldChange.Change.Value) if err != nil { return nil, err } - if len(patch) > 0 { - modifiedContent, patchErr = patcher.Apply(content, patch) + if patched != nil { + modifiedContent, patchErr = patched, nil } } @@ -184,69 +184,86 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( return content, nil } -// nullAncestorPatch replaces a null ancestor with the containers needed for an add. +// addThroughNullAncestor applies an add by materializing its null ancestor. // It returns nil when no null ancestor can be replaced safely. -func nullAncestorPatch(content []byte, fieldPath string, path yamlpatch.Path, value any) (yamlpatch.Patch, error) { +func addThroughNullAncestor(content []byte, fieldPath string, value any) ([]byte, error) { var doc yaml.Node if err := yaml.Unmarshal(content, &doc); err != nil { return nil, err } - current := &doc - for i, part := range path[:len(path)-1] { + if len(doc.Content) != 1 { + return nil, nil + } + pattern, err := structpath.ParsePattern(fieldPath) + if err != nil { + return nil, err + } + current := doc.Content[0] + var keyNode *yaml.Node + for depth, part := range pattern.AsSlice() { + // Replacing anchored content could alter aliases or leave dangling references. + if current.Anchor != "" { + return nil, nil + } + if current.Kind == yaml.ScalarNode && current.ShortTag() == "!!null" { + for node := pattern; node.Len() > depth; node = node.Parent() { + key, isKey := node.StringKey() + index, isIndex := node.Index() + switch { + case isKey: + value = map[string]any{key: value} + case node.BracketStar(), isIndex && index == 0: + value = []any{value} + default: + // Only the first element can be added to an empty sequence. + return nil, nil + } + } + var replacement yaml.Node + if err := replacement.Encode(value); err != nil { + return nil, err + } + replacement.HeadComment = current.HeadComment + replacement.FootComment = current.FootComment + // Block containers cannot carry the scalar's inline comment themselves. + if current.LineComment != "" { + if keyNode != nil { + keyNode.LineComment = strings.TrimSpace(keyNode.LineComment + "\n" + current.LineComment) + } else { + replacement.HeadComment = strings.TrimSpace(replacement.HeadComment + "\n" + current.LineComment) + } + } + *current = replacement + return encodeYAML(&doc) + } + switch current.Kind { - case yaml.DocumentNode: - if len(current.Content) != 1 { + case yaml.MappingNode: + key, ok := part.StringKey() + if !ok { return nil, nil } - current = current.Content[0] - case yaml.MappingNode: - var child *yaml.Node + keyNode = nil for j := 0; j+1 < len(current.Content); j += 2 { - if current.Content[j].Value == part { - child = current.Content[j+1] + if current.Content[j].Value == key { + keyNode = current.Content[j] + current = current.Content[j+1] break } } - if child == nil { + if keyNode == nil { return nil, nil } - current = child case yaml.SequenceNode: - index, err := strconv.Atoi(part) - if err != nil || index < 0 || index >= len(current.Content) { - return nil, nil //nolint:nilerr // keep the original patch error for unsupported paths + index, ok := part.Index() + if !ok || index < 0 || index >= len(current.Content) { + return nil, nil } + keyNode = nil current = current.Content[index] default: return nil, nil } - // Replacing anchored content could alter aliases or leave dangling references. - if current.Anchor != "" { - return nil, nil - } - if current.Kind != yaml.ScalarNode || current.ShortTag() != "!!null" { - continue - } - pattern, err := structpath.ParsePattern(fieldPath) - if err != nil { - return nil, err - } - for ; pattern.Len() > i; pattern = pattern.Parent() { - key, isKey := pattern.StringKey() - index, isIndex := pattern.Index() - switch { - case isKey: - value = map[string]any{key: value} - case pattern.BracketStar(), isIndex && index == 0: - value = []any{value} - default: - // Only the first element can be added to an empty sequence. - return nil, nil - } - } - // yamlpatch includes the document node as an empty first component. - parentPath := path[:i+1] - return yamlpatch.Patch{{Type: yamlpatch.OperationReplace, Path: parentPath, Value: value}}, nil } return nil, nil } @@ -366,10 +383,15 @@ func clearAddedFlowStyle(content []byte, fieldChanges []FieldChange) ([]byte, er clearFlowStyleAlongPath(&doc, candidate) } } + return encodeYAML(&doc) +} + +// encodeYAML uses the same indentation as the YAML patcher. +func encodeYAML(doc *yaml.Node) ([]byte, error) { var buf bytes.Buffer enc := yaml.NewEncoder(&buf) enc.SetIndent(2) - if err := enc.Encode(&doc); err != nil { + if err := enc.Encode(doc); err != nil { return nil, err } return buf.Bytes(), enc.Close() diff --git a/bundle/configsync/patch_test.go b/bundle/configsync/patch_test.go index c9949c5f312..40ee43d76a7 100644 --- a/bundle/configsync/patch_test.go +++ b/bundle/configsync/patch_test.go @@ -470,3 +470,27 @@ func TestApplyChangeScalarParents(t *testing.T) { }) } } + +func TestApplyChangeNullParentComments(t *testing.T) { + for _, tc := range []struct { + name string + content string + path string + want string + }{ + {"root", "# head\nnull # inline\n# foot\n", "key", "key: value"}, + {"sequence item", "parent:\n # head\n - null # inline\n # foot\n", "parent[0].key", "parent: [{key: value}]"}, + } { + t.Run(tc.name, func(t *testing.T) { + result, err := applyChange(t.Context(), []byte(tc.content), FieldChange{ + WritePath: tc.path, + Change: &ConfigChangeDesc{Operation: OperationAdd, Value: "value"}, + }) + require.NoError(t, err) + assert.YAMLEq(t, tc.want, string(result)) + for _, comment := range []string{"# head", "# inline", "# foot"} { + assert.Equal(t, 1, strings.Count(string(result), comment), "%s in %s", comment, result) + } + }) + } +} From ff7a19244d142ceea5618618412d4c8fc970c512 Mon Sep 17 00:00:00 2001 From: "ilya.kuznetsov" Date: Thu, 5 Nov 2026 10:40:29 +0000 Subject: [PATCH 5/5] Reuse YAML child lookup in config remote sync --- bundle/configsync/patch.go | 84 ++++++++++++++------------------------ 1 file changed, 31 insertions(+), 53 deletions(-) diff --git a/bundle/configsync/patch.go b/bundle/configsync/patch.go index fe22c961c59..a350686e1f4 100644 --- a/bundle/configsync/patch.go +++ b/bundle/configsync/patch.go @@ -237,31 +237,8 @@ func addThroughNullAncestor(content []byte, fieldPath string, value any) ([]byte return encodeYAML(&doc) } - switch current.Kind { - case yaml.MappingNode: - key, ok := part.StringKey() - if !ok { - return nil, nil - } - keyNode = nil - for j := 0; j+1 < len(current.Content); j += 2 { - if current.Content[j].Value == key { - keyNode = current.Content[j] - current = current.Content[j+1] - break - } - } - if keyNode == nil { - return nil, nil - } - case yaml.SequenceNode: - index, ok := part.Index() - if !ok || index < 0 || index >= len(current.Content) { - return nil, nil - } - keyNode = nil - current = current.Content[index] - default: + current, keyNode = yamlChild(current, (*structpath.PathNode)(part)) + if current == nil { return nil, nil } } @@ -397,6 +374,32 @@ func encodeYAML(doc *yaml.Node) ([]byte, error) { return buf.Bytes(), enc.Close() } +// yamlChild returns the child and its mapping key so callers can preserve comments. +// A nil child means the path component cannot be followed. +func yamlChild(current *yaml.Node, part *structpath.PathNode) (child, keyNode *yaml.Node) { + switch current.Kind { + case yaml.MappingNode: + key, ok := part.StringKey() + if !ok { + return nil, nil + } + // current.Content: [key1, val1, key2, val2, ...] + for i := 0; i+1 < len(current.Content); i += 2 { + if current.Content[i].Value == key { + return current.Content[i+1], current.Content[i] + } + } + case yaml.SequenceNode: + index, ok := part.Index() + if ok && index >= 0 && index < len(current.Content) { + return current.Content[index], nil + } + default: + return nil, nil + } + return nil, nil +} + // clearFlowStyleAlongPath navigates the YAML tree along the given structpath, // clearing FlowStyle on every node from root to leaf (inclusive). func clearFlowStyleAlongPath(doc *yaml.Node, pathStr string) { @@ -412,35 +415,10 @@ func clearFlowStyleAlongPath(doc *yaml.Node, pathStr string) { for _, n := range node.AsSlice() { current.Style &^= yaml.FlowStyle - - if key, ok := n.StringKey(); ok { - if current.Kind != yaml.MappingNode { - return - } - found := false - // current.Content: [key1, val1, key2, val2, ...] - for i := 0; i+1 < len(current.Content); i += 2 { - if current.Content[i].Value == key { - current = current.Content[i+1] - found = true - break - } - } - if !found { - return - } - continue + current, _ = yamlChild(current, n) + if current == nil { + return } - - if idx, ok := n.Index(); ok { - if current.Kind != yaml.SequenceNode || idx < 0 || idx >= len(current.Content) { - return - } - current = current.Content[idx] - continue - } - - return } clearFlowStyleNodes(current)