From 3fb2b01a4d61b90895f886fca23ed6eb28c67273 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 20 Jul 2026 11:38:48 +0000 Subject: [PATCH 1/5] Handle missing parent/key in config-remote-sync patch (replace->add, remove no-op) --- NEXT_CHANGELOG.md | 1 + bundle/configsync/patch.go | 39 ++++++++++++++++- bundle/configsync/patch_test.go | 77 +++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 2 deletions(-) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index da7072e3864..36ea231a746 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -10,6 +10,7 @@ ### Bundles +* Fix `bundle config-remote-sync` failing to write back a remote change when the field's parent path or key is absent from the target YAML file ("parent path /resources does not exist", "key ... does not exist and can not be replaced"): a replace now falls back to adding the value (creating any missing parent), and removing an already-absent field is a no-op ([#XXXX](https://github.com/databricks/cli/pull/XXXX)). * direct: add basic version of job_runs resource (experimental) ([#5603](https://github.com/databricks/cli/pull/5603)). * Fix permissions added to a job or pipeline by a Python (PyDABs) mutator failing to deploy with "must have exactly one owner"; the deploying identity is now set as owner, matching resources whose permissions are declared in YAML ([#5821](https://github.com/databricks/cli/pull/5821)). * Remove duplicate enum values for jsonschema.json ([#5839](https://github.com/databricks/cli/pull/5839)). diff --git a/bundle/configsync/patch.go b/bundle/configsync/patch.go index e002d31335c..b5586848037 100644 --- a/bundle/configsync/patch.go +++ b/bundle/configsync/patch.go @@ -139,6 +139,37 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( } } + // A replace whose target is absent from every candidate path (the field or + // its parent chain is missing) cannot replace anything. The remote value + // still needs to land in config, which is an add, so retry as one against + // the canonical (first) candidate and feed the missing-parent retry below. + if !success && fieldChange.Change.Operation == OperationReplace && isPathNotFoundError(firstErr) { + jsonPointer, err := strPathToJSONPointer(fieldChange.FieldCandidates[0]) + if err != nil { + return nil, fmt.Errorf("failed to convert field path %q to JSON pointer: %w", fieldChange.FieldCandidates[0], err) + } + path, err := yamlpatch.ParsePath(jsonPointer) + if err != nil { + return nil, fmt.Errorf("failed to parse JSON Pointer %s: %w", jsonPointer, err) + } + patcher := gopkgv3yamlpatcher.New(gopkgv3yamlpatcher.IndentSpaces(2)) + modifiedContent, patchErr := patcher.Apply(content, yamlpatch.Patch{yamlpatch.Operation{ + Type: yamlpatch.OperationAdd, + Path: path, + Value: fieldChange.Change.Value, + }}) + switch { + case patchErr == nil: + content = modifiedContent + success = true + firstErr = nil + case isParentPathError(patchErr): + if missingPath, extractErr := extractMissingPath(patchErr); extractErr == nil { + parentNodesToCreate = append(parentNodesToCreate, parentNode{path, missingPath}) + } + } + } + // If all attempts failed with parent path errors, try creating nested structures if !success && len(parentNodesToCreate) > 0 { for _, errInfo := range parentNodesToCreate { @@ -165,8 +196,12 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( } if firstErr != nil { - if (fieldChange.Change.Operation == OperationRemove || fieldChange.Change.Operation == OperationReplace) && isPathNotFoundError(firstErr) { - return nil, fmt.Errorf("field not found in YAML configuration: %w", firstErr) + // A remove whose target is already absent from this file is a no-op: + // the field the remote dropped is not in config to begin with, so leave + // the file unchanged rather than failing the whole sync. + if fieldChange.Change.Operation == OperationRemove && isPathNotFoundError(firstErr) { + log.Debugf(ctx, "Nothing to remove for %s, field not present in YAML", fieldChange.FieldCandidates[0]) + return content, nil } return nil, fmt.Errorf("failed to apply change: %w", firstErr) } diff --git a/bundle/configsync/patch_test.go b/bundle/configsync/patch_test.go index b4faea82858..092011085f2 100644 --- a/bundle/configsync/patch_test.go +++ b/bundle/configsync/patch_test.go @@ -66,6 +66,83 @@ resources: assert.NotContains(t, modified, blankLineMarker) } +// A replace/remove whose target field, key, or parent chain is absent from the +// YAML file must not fail the sync: a replace writes the remote value in (as an +// add, creating any missing parent), and a remove is a no-op. +func TestApplyChangeMissingTarget(t *testing.T) { + ctx := logdiag.InitContext(t.Context()) + + tests := []struct { + name string + content string + op OperationType + value any + candidates []string + wantAdded string // substring expected in the result (empty for remove no-op) + }{ + { + name: "replace field whose /resources parent is absent", + content: `bundle: + name: x +targets: + dev: + resources: + jobs: + my_job: + max_concurrent_runs: 2 +`, + op: OperationReplace, + value: "targets.dev.resources.jobs.my_job.max_concurrent_runs", + candidates: []string{"resources.jobs.my_job.max_concurrent_runs"}, + wantAdded: "resources:", + }, + { + name: "replace key absent from an existing parent", + content: `resources: + jobs: + my_job: + tasks: + - task_key: a + notebook_task: + notebook_path: /a +`, + op: OperationReplace, + value: "ALL_DONE", + candidates: []string{"resources.jobs.my_job.tasks[0].run_if"}, + wantAdded: "run_if: ALL_DONE", + }, + { + name: "remove field already absent is a no-op", + content: `resources: + jobs: + my_job: + name: J +`, + op: OperationRemove, + value: nil, + candidates: []string{"resources.jobs.my_job.max_concurrent_runs"}, + wantAdded: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fc := FieldChange{ + FilePath: "databricks.yml", + Change: &ConfigChangeDesc{Operation: tt.op, Value: tt.value}, + FieldCandidates: tt.candidates, + } + got, err := applyChange(ctx, []byte(tt.content), fc) + require.NoError(t, err) + if tt.wantAdded == "" { + assert.Equal(t, tt.content, string(got)) + } else { + assert.Contains(t, string(got), tt.wantAdded) + } + }) + } +} + // for readability of test cases func nl(s string) string { return strings.TrimPrefix(s, "\n") From 2a7b9ec8e6f7b17e04546d0bac8db01e9dc5ead9 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 20 Jul 2026 12:05:43 +0000 Subject: [PATCH 2/5] config-remote-sync: replace-fallback add prefers candidate whose parent exists --- bundle/configsync/patch.go | 55 +++++++++++++++++++-------------- bundle/configsync/patch_test.go | 27 ++++++++++++++++ 2 files changed, 58 insertions(+), 24 deletions(-) diff --git a/bundle/configsync/patch.go b/bundle/configsync/patch.go index b5586848037..c424ee521bd 100644 --- a/bundle/configsync/patch.go +++ b/bundle/configsync/patch.go @@ -141,31 +141,38 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( // A replace whose target is absent from every candidate path (the field or // its parent chain is missing) cannot replace anything. The remote value - // still needs to land in config, which is an add, so retry as one against - // the canonical (first) candidate and feed the missing-parent retry below. + // still needs to land in config, which is an add, so retry each candidate as + // an add. Trying every candidate matters when the resource lives only under a + // target: the first candidate ("resources...") has no parent, but the + // target-prefixed one does, so adding to it keeps the value in the target + // block instead of leaking a spurious top-level resources entry. Candidates + // whose parent is also missing feed the nested-structure retry below. if !success && fieldChange.Change.Operation == OperationReplace && isPathNotFoundError(firstErr) { - jsonPointer, err := strPathToJSONPointer(fieldChange.FieldCandidates[0]) - if err != nil { - return nil, fmt.Errorf("failed to convert field path %q to JSON pointer: %w", fieldChange.FieldCandidates[0], err) - } - path, err := yamlpatch.ParsePath(jsonPointer) - if err != nil { - return nil, fmt.Errorf("failed to parse JSON Pointer %s: %w", jsonPointer, err) - } - patcher := gopkgv3yamlpatcher.New(gopkgv3yamlpatcher.IndentSpaces(2)) - modifiedContent, patchErr := patcher.Apply(content, yamlpatch.Patch{yamlpatch.Operation{ - Type: yamlpatch.OperationAdd, - Path: path, - Value: fieldChange.Change.Value, - }}) - switch { - case patchErr == nil: - content = modifiedContent - success = true - firstErr = nil - case isParentPathError(patchErr): - if missingPath, extractErr := extractMissingPath(patchErr); extractErr == nil { - parentNodesToCreate = append(parentNodesToCreate, parentNode{path, missingPath}) + for _, fieldPathCandidate := range fieldChange.FieldCandidates { + jsonPointer, err := strPathToJSONPointer(fieldPathCandidate) + if err != nil { + return nil, fmt.Errorf("failed to convert field path %q to JSON pointer: %w", fieldPathCandidate, err) + } + path, err := yamlpatch.ParsePath(jsonPointer) + if err != nil { + return nil, fmt.Errorf("failed to parse JSON Pointer %s: %w", jsonPointer, err) + } + patcher := gopkgv3yamlpatcher.New(gopkgv3yamlpatcher.IndentSpaces(2)) + modifiedContent, patchErr := patcher.Apply(content, yamlpatch.Patch{yamlpatch.Operation{ + Type: yamlpatch.OperationAdd, + Path: path, + Value: fieldChange.Change.Value, + }}) + if patchErr == nil { + content = modifiedContent + success = true + firstErr = nil + break + } + if isParentPathError(patchErr) { + if missingPath, extractErr := extractMissingPath(patchErr); extractErr == nil { + parentNodesToCreate = append(parentNodesToCreate, parentNode{path, missingPath}) + } } } } diff --git a/bundle/configsync/patch_test.go b/bundle/configsync/patch_test.go index 092011085f2..f0eec3fc7f9 100644 --- a/bundle/configsync/patch_test.go +++ b/bundle/configsync/patch_test.go @@ -79,6 +79,7 @@ func TestApplyChangeMissingTarget(t *testing.T) { value any candidates []string wantAdded string // substring expected in the result (empty for remove no-op) + wantAbsent string // substring that must NOT appear in the result }{ { name: "replace field whose /resources parent is absent", @@ -96,6 +97,29 @@ targets: candidates: []string{"resources.jobs.my_job.max_concurrent_runs"}, wantAdded: "resources:", }, + { + // A target-only resource: the "resources..." candidate has no parent, + // but the target-prefixed one does, so the add must land in the target + // block rather than creating a spurious top-level resources entry. + name: "replace prefers the candidate whose parent exists", + content: `bundle: + name: x +targets: + dev: + resources: + jobs: + my_job: + name: J +`, + op: OperationReplace, + value: "added", + candidates: []string{ + "resources.jobs.my_job.description", + "targets.dev.resources.jobs.my_job.description", + }, + wantAdded: "description: added", + wantAbsent: "\nresources:", + }, { name: "replace key absent from an existing parent", content: `resources: @@ -139,6 +163,9 @@ targets: } else { assert.Contains(t, string(got), tt.wantAdded) } + if tt.wantAbsent != "" { + assert.NotContains(t, string(got), tt.wantAbsent) + } }) } } From b3a976cdd16a67bea1389fb8807dd0bdd909b391 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 20 Jul 2026 12:48:03 +0000 Subject: [PATCH 3/5] Drop changelog entry (config-remote-sync is a hidden command) --- NEXT_CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 36ea231a746..da7072e3864 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -10,7 +10,6 @@ ### Bundles -* Fix `bundle config-remote-sync` failing to write back a remote change when the field's parent path or key is absent from the target YAML file ("parent path /resources does not exist", "key ... does not exist and can not be replaced"): a replace now falls back to adding the value (creating any missing parent), and removing an already-absent field is a no-op ([#XXXX](https://github.com/databricks/cli/pull/XXXX)). * direct: add basic version of job_runs resource (experimental) ([#5603](https://github.com/databricks/cli/pull/5603)). * Fix permissions added to a job or pipeline by a Python (PyDABs) mutator failing to deploy with "must have exactly one owner"; the deploying identity is now set as owner, matching resources whose permissions are declared in YAML ([#5821](https://github.com/databricks/cli/pull/5821)). * Remove duplicate enum values for jsonschema.json ([#5839](https://github.com/databricks/cli/pull/5839)). From 8d8c8adfa12f097074d98faad68b780c8108a526 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Tue, 21 Jul 2026 13:58:56 +0000 Subject: [PATCH 4/5] Fix config-remote-sync writing edits to wrong task; make yamlFileIndex block-aware (revert patch.go fallback) --- .../databricks.yml.tmpl | 34 ++++++ .../task_split_target_index/out.test.toml | 4 + .../task_split_target_index/output.txt | 36 ++++++ .../task_split_target_index/script | 37 +++++++ .../task_split_target_index/test.toml | 10 ++ bundle/configsync/patch.go | 46 +------- bundle/configsync/patch_test.go | 104 ------------------ bundle/configsync/resolve.go | 37 ++++++- bundle/configsync/resolve_test.go | 44 ++++++-- 9 files changed, 188 insertions(+), 164 deletions(-) create mode 100644 acceptance/bundle/config-remote-sync/task_split_target_index/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/task_split_target_index/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/task_split_target_index/output.txt create mode 100644 acceptance/bundle/config-remote-sync/task_split_target_index/script create mode 100644 acceptance/bundle/config-remote-sync/task_split_target_index/test.toml diff --git a/acceptance/bundle/config-remote-sync/task_split_target_index/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/task_split_target_index/databricks.yml.tmpl new file mode 100644 index 00000000000..9581be2a521 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_index/databricks.yml.tmpl @@ -0,0 +1,34 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +targets: + default: + mode: development + resources: + jobs: + my_job: + tasks: + - task_key: aaa_task + run_if: ALL_SUCCESS + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/aaa + +resources: + jobs: + my_job: + tasks: + - task_key: bbb_task + run_if: ALL_SUCCESS + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/bbb + new_cluster: + spark_version: $DEFAULT_SPARK_VERSION + node_type_id: $NODE_TYPE_ID + num_workers: 1 + - task_key: ccc_task + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/ccc + new_cluster: + spark_version: $DEFAULT_SPARK_VERSION + node_type_id: $NODE_TYPE_ID + num_workers: 1 diff --git a/acceptance/bundle/config-remote-sync/task_split_target_index/out.test.toml b/acceptance/bundle/config-remote-sync/task_split_target_index/out.test.toml new file mode 100644 index 00000000000..579b1e4a3c9 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_index/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = true +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/task_split_target_index/output.txt b/acceptance/bundle/config-remote-sync/task_split_target_index/output.txt new file mode 100644 index 00000000000..04b725289ce --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_index/output.txt @@ -0,0 +1,36 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Set run_if on bbb_task remotely (merged task order: aaa,bbb,ccc) + +=== Detect and save changes +Detected changes in 1 resource(s): + +Resource: resources.jobs.my_job + tasks[task_key='bbb_task'].run_if: replace + + + +=== Configuration changes + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -19,5 +19,5 @@ + tasks: + - task_key: bbb_task +- run_if: ALL_SUCCESS ++ run_if: ALL_DONE + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/bbb + +>>> [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 + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/task_split_target_index/script b/acceptance/bundle/config-remote-sync/task_split_target_index/script new file mode 100644 index 00000000000..8ab908ffde1 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_index/script @@ -0,0 +1,37 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve +} +trap cleanup EXIT + +$CLI bundle deploy +job_id="$(read_id.py my_job)" + +# my_job's tasks are split across two config blocks in the SAME file: the +# top-level resources block (bbb_task, ccc_task) and the targets.default +# override block (aaa_task). After MergeJobTasks sorts by task_key the merged +# order is aaa,bbb,ccc. yamlFileIndex must be block-aware: bbb_task's remote +# run_if edit resolves to index 0 of the top-level block (bbb_task), not to the +# whole-file position 1 (ccc_task). +title "Set run_if on bbb_task remotely (merged task order: aaa,bbb,ccc)" +echo +edit_resource.py jobs $job_id < 0 { for _, errInfo := range parentNodesToCreate { @@ -203,12 +165,8 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( } if firstErr != nil { - // A remove whose target is already absent from this file is a no-op: - // the field the remote dropped is not in config to begin with, so leave - // the file unchanged rather than failing the whole sync. - if fieldChange.Change.Operation == OperationRemove && isPathNotFoundError(firstErr) { - log.Debugf(ctx, "Nothing to remove for %s, field not present in YAML", fieldChange.FieldCandidates[0]) - return content, nil + if (fieldChange.Change.Operation == OperationRemove || fieldChange.Change.Operation == OperationReplace) && isPathNotFoundError(firstErr) { + return nil, fmt.Errorf("field not found in YAML configuration: %w", firstErr) } return nil, fmt.Errorf("failed to apply change: %w", firstErr) } diff --git a/bundle/configsync/patch_test.go b/bundle/configsync/patch_test.go index f0eec3fc7f9..b4faea82858 100644 --- a/bundle/configsync/patch_test.go +++ b/bundle/configsync/patch_test.go @@ -66,110 +66,6 @@ resources: assert.NotContains(t, modified, blankLineMarker) } -// A replace/remove whose target field, key, or parent chain is absent from the -// YAML file must not fail the sync: a replace writes the remote value in (as an -// add, creating any missing parent), and a remove is a no-op. -func TestApplyChangeMissingTarget(t *testing.T) { - ctx := logdiag.InitContext(t.Context()) - - tests := []struct { - name string - content string - op OperationType - value any - candidates []string - wantAdded string // substring expected in the result (empty for remove no-op) - wantAbsent string // substring that must NOT appear in the result - }{ - { - name: "replace field whose /resources parent is absent", - content: `bundle: - name: x -targets: - dev: - resources: - jobs: - my_job: - max_concurrent_runs: 2 -`, - op: OperationReplace, - value: "targets.dev.resources.jobs.my_job.max_concurrent_runs", - candidates: []string{"resources.jobs.my_job.max_concurrent_runs"}, - wantAdded: "resources:", - }, - { - // A target-only resource: the "resources..." candidate has no parent, - // but the target-prefixed one does, so the add must land in the target - // block rather than creating a spurious top-level resources entry. - name: "replace prefers the candidate whose parent exists", - content: `bundle: - name: x -targets: - dev: - resources: - jobs: - my_job: - name: J -`, - op: OperationReplace, - value: "added", - candidates: []string{ - "resources.jobs.my_job.description", - "targets.dev.resources.jobs.my_job.description", - }, - wantAdded: "description: added", - wantAbsent: "\nresources:", - }, - { - name: "replace key absent from an existing parent", - content: `resources: - jobs: - my_job: - tasks: - - task_key: a - notebook_task: - notebook_path: /a -`, - op: OperationReplace, - value: "ALL_DONE", - candidates: []string{"resources.jobs.my_job.tasks[0].run_if"}, - wantAdded: "run_if: ALL_DONE", - }, - { - name: "remove field already absent is a no-op", - content: `resources: - jobs: - my_job: - name: J -`, - op: OperationRemove, - value: nil, - candidates: []string{"resources.jobs.my_job.max_concurrent_runs"}, - wantAdded: "", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - fc := FieldChange{ - FilePath: "databricks.yml", - Change: &ConfigChangeDesc{Operation: tt.op, Value: tt.value}, - FieldCandidates: tt.candidates, - } - got, err := applyChange(ctx, []byte(tt.content), fc) - require.NoError(t, err) - if tt.wantAdded == "" { - assert.Equal(t, tt.content, string(got)) - } else { - assert.Contains(t, string(got), tt.wantAdded) - } - if tt.wantAbsent != "" { - assert.NotContains(t, string(got), tt.wantAbsent) - } - }) - } -} - // for readability of test cases func nl(s string) string { return strings.TrimPrefix(s, "\n") diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index 01018365e67..9c9922adfe8 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -87,7 +87,7 @@ func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) // Mutators may reorder sequence elements (e.g., tasks sorted by task_key). // Use location information to determine the original YAML file position. - yamlIndex := yamlFileIndex(seq, foundIndex) + yamlIndex := yamlFileIndex(seq, foundIndex, currentValue.Locations()) result = structpath.NewPatternIndex(result, yamlIndex) currentValue = seq[foundIndex] continue @@ -99,28 +99,53 @@ func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) // yamlFileIndex determines the original YAML file position of a sequence element. // Mutators may reorder sequence elements (e.g., tasks sorted by task_key), so the -// in-memory index may not match the position in the YAML file. This function uses -// location information to count how many elements from the same file appear before -// the target element, giving the correct index for YAML patching. -func yamlFileIndex(seq []dyn.Value, sortedIndex int) int { +// in-memory index may not match the position in the YAML file. +// +// A single field can hold sequences from more than one config block — e.g. a job's +// tasks defined partly under top-level `resources.jobs..tasks` and partly under +// `targets..resources.jobs..tasks`. MergeJobTasks concatenates and sorts them +// into one in-memory sequence, but each block is patched independently in the YAML. +// seqLocations holds one location per contributing block (the location of each block's +// sequence node); an element belongs to the block whose anchor is the closest one at or +// before it in the same file. Counting only same-block elements with a smaller line +// gives the index within that block, which is what the YAML patch targets. When the +// sequence comes from a single block this reduces to a plain same-file count. +func yamlFileIndex(seq []dyn.Value, sortedIndex int, seqLocations []dyn.Location) int { matchLocation := seq[sortedIndex].Location() if matchLocation.File == "" { return sortedIndex } + matchAnchor := blockAnchor(matchLocation, seqLocations) + yamlIndex := 0 for i, elem := range seq { if i == sortedIndex { continue } loc := elem.Location() - if loc.File == matchLocation.File && loc.Line < matchLocation.Line { + if loc.File == matchLocation.File && loc.Line < matchLocation.Line && blockAnchor(loc, seqLocations) == matchAnchor { yamlIndex++ } } return yamlIndex } +// blockAnchor returns the config block a sequence element belongs to, identified by the +// line of that block's sequence node. Blocks never interleave within a file, so the +// element's block is the anchor with the greatest line at or before the element in the +// same file. Returns 0 when no anchor matches (single unlocated block), which keeps all +// elements in one group. +func blockAnchor(loc dyn.Location, seqLocations []dyn.Location) int { + anchor := 0 + for _, l := range seqLocations { + if l.File == loc.File && l.Line <= loc.Line && l.Line > anchor { + anchor = l.Line + } + } + return anchor +} + func pathDepth(pathStr string) int { node, err := structpath.ParsePath(pathStr) if err != nil { diff --git a/bundle/configsync/resolve_test.go b/bundle/configsync/resolve_test.go index 9264ad7f5dc..59ad15c15bf 100644 --- a/bundle/configsync/resolve_test.go +++ b/bundle/configsync/resolve_test.go @@ -215,11 +215,13 @@ func TestYamlFileIndex(t *testing.T) { dyn.NewValue(nil, []dyn.Location{{File: "a.yml", Line: 30}}), // pipeline_task dyn.NewValue(nil, []dyn.Location{{File: "a.yml", Line: 20}}), // python_wheel_task } + // Single block: the sequence node is anchored at the block's line in the file. + seqLocations := []dyn.Location{{File: "a.yml", Line: 5}} - assert.Equal(t, 3, yamlFileIndex(seq, 0)) // extra: 3 elements before it in YAML - assert.Equal(t, 0, yamlFileIndex(seq, 1)) // notebook_task: first in YAML - assert.Equal(t, 2, yamlFileIndex(seq, 2)) // pipeline_task: 2 elements before it - assert.Equal(t, 1, yamlFileIndex(seq, 3)) // python_wheel_task: 1 element before it + assert.Equal(t, 3, yamlFileIndex(seq, 0, seqLocations)) // extra: 3 elements before it in YAML + assert.Equal(t, 0, yamlFileIndex(seq, 1, seqLocations)) // notebook_task: first in YAML + assert.Equal(t, 2, yamlFileIndex(seq, 2, seqLocations)) // pipeline_task: 2 elements before it + assert.Equal(t, 1, yamlFileIndex(seq, 3, seqLocations)) // python_wheel_task: 1 element before it } func TestYamlFileIndex_MultipleFiles(t *testing.T) { @@ -233,12 +235,14 @@ func TestYamlFileIndex_MultipleFiles(t *testing.T) { dyn.NewValue(nil, []dyn.Location{{File: "b.yml", Line: 5}}), // task_c dyn.NewValue(nil, []dyn.Location{{File: "b.yml", Line: 15}}), // task_d } + // One anchor per file, each above its file's first element. + seqLocations := []dyn.Location{{File: "a.yml", Line: 3}, {File: "b.yml", Line: 2}} // Indices are relative to each file - assert.Equal(t, 0, yamlFileIndex(seq, 0)) // task_a: first in file A - assert.Equal(t, 1, yamlFileIndex(seq, 1)) // task_b: second in file A - assert.Equal(t, 0, yamlFileIndex(seq, 2)) // task_c: first in file B - assert.Equal(t, 1, yamlFileIndex(seq, 3)) // task_d: second in file B + assert.Equal(t, 0, yamlFileIndex(seq, 0, seqLocations)) // task_a: first in file A + assert.Equal(t, 1, yamlFileIndex(seq, 1, seqLocations)) // task_b: second in file A + assert.Equal(t, 0, yamlFileIndex(seq, 2, seqLocations)) // task_c: first in file B + assert.Equal(t, 1, yamlFileIndex(seq, 3, seqLocations)) // task_d: second in file B } func TestYamlFileIndex_NoLocation(t *testing.T) { @@ -246,6 +250,26 @@ func TestYamlFileIndex_NoLocation(t *testing.T) { dyn.NewValue(nil, nil), dyn.NewValue(nil, nil), } - assert.Equal(t, 0, yamlFileIndex(seq, 0)) - assert.Equal(t, 1, yamlFileIndex(seq, 1)) + assert.Equal(t, 0, yamlFileIndex(seq, 0, nil)) + assert.Equal(t, 1, yamlFileIndex(seq, 1, nil)) +} + +func TestYamlFileIndex_SplitBlocksSameFile(t *testing.T) { + // A job's tasks split across two blocks in the SAME file: a targets override + // block (aaa, near the top) and the top-level resources block (bbb, ccc, lower + // down). MergeJobTasks concatenates and sorts by task_key -> [aaa, bbb, ccc]. + // The tasks sequence node carries one anchor per block: top-level tasks at line + // 20, target tasks at line 11 (order matches the real merge, which keeps the + // top-level location first). Each element's index must be relative to its own + // block, not the whole file. + seq := []dyn.Value{ + dyn.NewValue(nil, []dyn.Location{{File: "databricks.yml", Line: 11}}), // aaa (target block) + dyn.NewValue(nil, []dyn.Location{{File: "databricks.yml", Line: 20}}), // bbb (top-level block) + dyn.NewValue(nil, []dyn.Location{{File: "databricks.yml", Line: 28}}), // ccc (top-level block) + } + seqLocations := []dyn.Location{{File: "databricks.yml", Line: 20}, {File: "databricks.yml", Line: 11}} + + assert.Equal(t, 0, yamlFileIndex(seq, 0, seqLocations)) // aaa: first in target block + assert.Equal(t, 0, yamlFileIndex(seq, 1, seqLocations)) // bbb: first in top-level block (NOT 1) + assert.Equal(t, 1, yamlFileIndex(seq, 2, seqLocations)) // ccc: second in top-level block } From 1ee0e4794912f88bb40a6144afd5df094fd416db Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Wed, 22 Jul 2026 09:05:53 +0000 Subject: [PATCH 5/5] Fix config-remote-sync writing target-block task edit to wrong top-level task --- .../databricks.yml.tmpl | 28 ++++++++++ .../task_split_target_local/out.test.toml | 4 ++ .../task_split_target_local/output.txt | 36 ++++++++++++ .../task_split_target_local/script | 33 +++++++++++ .../task_split_target_local/test.toml | 10 ++++ bundle/configsync/resolve.go | 55 +++++++++++++++---- bundle/configsync/resolve_test.go | 42 +++++++++++--- 7 files changed, 188 insertions(+), 20 deletions(-) create mode 100644 acceptance/bundle/config-remote-sync/task_split_target_local/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/task_split_target_local/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/task_split_target_local/output.txt create mode 100644 acceptance/bundle/config-remote-sync/task_split_target_local/script create mode 100644 acceptance/bundle/config-remote-sync/task_split_target_local/test.toml diff --git a/acceptance/bundle/config-remote-sync/task_split_target_local/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/task_split_target_local/databricks.yml.tmpl new file mode 100644 index 00000000000..96db19e354a --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_local/databricks.yml.tmpl @@ -0,0 +1,28 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +# Mirror of task_split_target_index: here the EDITED task lives in the target +# block (aaa_tgt, target-local index 0) while a different task sits at the same +# index 0 of the top-level block (zzz_top, which also has run_if). The remote +# edit must land on aaa_tgt, not be misdirected to zzz_top via the unprefixed +# candidate. +resources: + jobs: + my_job: + tasks: + - task_key: zzz_top + run_if: ALL_SUCCESS + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/zzz + +targets: + default: + mode: development + resources: + jobs: + my_job: + tasks: + - task_key: aaa_tgt + run_if: ALL_SUCCESS + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/aaa diff --git a/acceptance/bundle/config-remote-sync/task_split_target_local/out.test.toml b/acceptance/bundle/config-remote-sync/task_split_target_local/out.test.toml new file mode 100644 index 00000000000..579b1e4a3c9 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_local/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = true +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/task_split_target_local/output.txt b/acceptance/bundle/config-remote-sync/task_split_target_local/output.txt new file mode 100644 index 00000000000..6e7a4acf698 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_local/output.txt @@ -0,0 +1,36 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Set run_if on aaa_tgt remotely (target-block task) + +=== Detect and save changes +Detected changes in 1 resource(s): + +Resource: resources.jobs.my_job + tasks[task_key='aaa_tgt'].run_if: replace + + + +=== Configuration changes + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -24,5 +24,5 @@ + tasks: + - task_key: aaa_tgt +- run_if: ALL_SUCCESS ++ run_if: ALL_DONE + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/aaa + +>>> [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 + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/task_split_target_local/script b/acceptance/bundle/config-remote-sync/task_split_target_local/script new file mode 100644 index 00000000000..3946293f135 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_local/script @@ -0,0 +1,33 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve +} +trap cleanup EXIT + +$CLI bundle deploy +job_id="$(read_id.py my_job)" + +# Merged+sorted task order is aaa_tgt, zzz_top. aaa_tgt is defined in the +# targets block (target-local index 0); zzz_top in the top-level block +# (top-level index 0) and also has run_if. Editing aaa_tgt's run_if must patch +# the targets block, not zzz_top in the top-level block. +title "Set run_if on aaa_tgt remotely (target-block task)" +echo +edit_resource.py jobs $job_id < "resources.jobs.foo.tasks[1].name" // Returns a PatternNode because for Add operations, [*] may be used as a placeholder for new elements. -func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) (*structpath.PatternNode, dyn.Location, error) { +// +// The returned bool reports whether the resolved element was contributed by a +// target override block. Its numeric index is then local to that block, so only +// the targets..-prefixed candidate is valid (see ResolveChanges). +func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) (*structpath.PatternNode, dyn.Location, bool, error) { node, err := structpath.ParsePath(pathStr) if err != nil { - return nil, dyn.Location{}, fmt.Errorf("failed to parse path %s: %w", pathStr, err) + return nil, dyn.Location{}, false, fmt.Errorf("failed to parse path %s: %w", pathStr, err) } nodes := node.AsSlice() var result *structpath.PatternNode + inOverrideBlock := false currentValue := b.Config.Value() for _, n := range nodes { @@ -57,10 +62,11 @@ func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) // Check for key-value selector: [key='value'] if key, value, ok := n.KeyValue(); ok { if !currentValue.IsValid() || currentValue.Kind() != dyn.KindSequence { - return nil, dyn.Location{}, fmt.Errorf("cannot apply [%s='%s'] selector to non-array value in path %s", key, value, pathStr) + return nil, dyn.Location{}, false, fmt.Errorf("cannot apply [%s='%s'] selector to non-array value in path %s", key, value, pathStr) } seq, _ := currentValue.AsSequence() + seqLocations := currentValue.Locations() foundIndex := -1 for i, elem := range seq { @@ -82,19 +88,37 @@ func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) currentValue = dyn.Value{} continue } - return nil, dyn.Location{}, fmt.Errorf("no array element found with %s='%s' in path %s", key, value, pathStr) + return nil, dyn.Location{}, false, fmt.Errorf("no array element found with %s='%s' in path %s", key, value, pathStr) } // Mutators may reorder sequence elements (e.g., tasks sorted by task_key). // Use location information to determine the original YAML file position. - yamlIndex := yamlFileIndex(seq, foundIndex, currentValue.Locations()) + yamlIndex := yamlFileIndex(seq, foundIndex, seqLocations) result = structpath.NewPatternIndex(result, yamlIndex) + // yamlIndex is local to the element's own block; latch so a nested + // single-block selector further down doesn't clear a target match. + inOverrideBlock = inOverrideBlock || elementInOverrideBlock(seq[foundIndex].Location(), seqLocations) currentValue = seq[foundIndex] continue } } - return result, currentValue.Location(), nil + return result, currentValue.Location(), inOverrideBlock, nil +} + +// elementInOverrideBlock reports whether a merged sequence element was contributed +// by a target override block rather than the top-level resources block. Target +// overrides are merged with the top-level block as the merge reference, and merge +// keeps the reference locations first, so seqLocations[0] is always the top-level +// anchor regardless of where each block appears in the file. An element whose +// nearest block anchor is not that top-level anchor came from a target block, so +// its block-local index is only valid under the targets.. prefix. +func elementInOverrideBlock(elemLoc dyn.Location, seqLocations []dyn.Location) bool { + if len(seqLocations) < 2 || elemLoc.File == "" { + return false + } + topAnchor := seqLocations[0] + return elemLoc.File != topAnchor.File || blockAnchor(elemLoc, seqLocations) != topAnchor.Line } // yamlFileIndex determines the original YAML file position of a sequence element. @@ -240,7 +264,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes configChange := resourceChanges[fieldPath] fullPath := resourceKey + "." + fieldPath - resolvedPath, resolvedLocation, err := resolveSelectors(fullPath, b, configChange.Operation) + resolvedPath, resolvedLocation, inOverrideBlock, err := resolveSelectors(fullPath, b, configChange.Operation) if err != nil { return nil, fmt.Errorf("failed to resolve selectors in path %s: %w", fullPath, err) } @@ -277,10 +301,19 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes } resolvedPathStr := resolvedPath.String() - candidates := []string{resolvedPathStr} - if targetName != "" { - targetPrefixedPath := "targets." + targetName + "." + resolvedPathStr - candidates = append(candidates, targetPrefixedPath) + var candidates []string + targetPrefixedPath := "targets." + targetName + "." + resolvedPathStr + switch { + case inOverrideBlock: + // The index is local to the target override block, so only the + // target-prefixed candidate points at the right element. Emitting the + // unprefixed candidate too would let applyChange write the same index + // into the top-level block and silently patch the wrong element. + candidates = []string{targetPrefixedPath} + case targetName != "": + candidates = []string{resolvedPathStr, targetPrefixedPath} + default: + candidates = []string{resolvedPathStr} } filePath := resolvedLocation.File diff --git a/bundle/configsync/resolve_test.go b/bundle/configsync/resolve_test.go index 59ad15c15bf..e39174abd00 100644 --- a/bundle/configsync/resolve_test.go +++ b/bundle/configsync/resolve_test.go @@ -32,7 +32,7 @@ func TestResolveSelectors_NoSelectors(t *testing.T) { mutator.DefaultMutators(ctx, b) - result, _, err := resolveSelectors("resources.jobs.test_job.name", b, OperationReplace) + result, _, _, err := resolveSelectors("resources.jobs.test_job.name", b, OperationReplace) require.NoError(t, err) assert.Equal(t, "resources.jobs.test_job.name", result.String()) } @@ -57,11 +57,11 @@ func TestResolveSelectors_NumericIndices(t *testing.T) { mutator.DefaultMutators(ctx, b) - result, _, err := resolveSelectors("resources.jobs.test_job.tasks[0].task_key", b, OperationReplace) + result, _, _, err := resolveSelectors("resources.jobs.test_job.tasks[0].task_key", b, OperationReplace) require.NoError(t, err) assert.Equal(t, "resources.jobs.test_job.tasks[0].task_key", result.String()) - result, _, err = resolveSelectors("resources.jobs.test_job.tasks[1].task_key", b, OperationReplace) + result, _, _, err = resolveSelectors("resources.jobs.test_job.tasks[1].task_key", b, OperationReplace) require.NoError(t, err) assert.Equal(t, "resources.jobs.test_job.tasks[1].task_key", result.String()) } @@ -90,11 +90,11 @@ func TestResolveSelectors_KeyValueSelector(t *testing.T) { mutator.DefaultMutators(ctx, b) - result, _, err := resolveSelectors("resources.jobs.test_job.tasks[task_key='main'].notebook_task.notebook_path", b, OperationReplace) + result, _, _, err := resolveSelectors("resources.jobs.test_job.tasks[task_key='main'].notebook_task.notebook_path", b, OperationReplace) require.NoError(t, err) assert.Equal(t, "resources.jobs.test_job.tasks[1].notebook_task.notebook_path", result.String()) - result, _, err = resolveSelectors("resources.jobs.test_job.tasks[task_key='setup'].notebook_task.notebook_path", b, OperationReplace) + result, _, _, err = resolveSelectors("resources.jobs.test_job.tasks[task_key='setup'].notebook_task.notebook_path", b, OperationReplace) require.NoError(t, err) assert.Equal(t, "resources.jobs.test_job.tasks[0].notebook_task.notebook_path", result.String()) } @@ -120,7 +120,7 @@ func TestResolveSelectors_SelectorNotFound(t *testing.T) { mutator.DefaultMutators(ctx, b) - _, _, err = resolveSelectors("resources.jobs.test_job.tasks[task_key='nonexistent'].notebook_task.notebook_path", b, OperationReplace) + _, _, _, err = resolveSelectors("resources.jobs.test_job.tasks[task_key='nonexistent'].notebook_task.notebook_path", b, OperationReplace) require.Error(t, err) assert.Contains(t, err.Error(), "no array element found with task_key='nonexistent'") } @@ -143,7 +143,7 @@ func TestResolveSelectors_SelectorOnNonArray(t *testing.T) { mutator.DefaultMutators(ctx, b) - _, _, err = resolveSelectors("resources.jobs.test_job[task_key='main'].name", b, OperationReplace) + _, _, _, err = resolveSelectors("resources.jobs.test_job[task_key='main'].name", b, OperationReplace) require.Error(t, err) assert.Contains(t, err.Error(), "cannot apply [task_key='main'] selector to non-array value") } @@ -174,7 +174,7 @@ func TestResolveSelectors_NestedSelectors(t *testing.T) { mutator.DefaultMutators(ctx, b) - result, _, err := resolveSelectors("resources.jobs.test_job.tasks[task_key='main'].libraries[0].pypi.package", b, OperationReplace) + result, _, _, err := resolveSelectors("resources.jobs.test_job.tasks[task_key='main'].libraries[0].pypi.package", b, OperationReplace) require.NoError(t, err) assert.Equal(t, "resources.jobs.test_job.tasks[1].libraries[0].pypi.package", result.String()) } @@ -200,7 +200,7 @@ func TestResolveSelectors_WildcardNotSupported(t *testing.T) { mutator.DefaultMutators(ctx, b) - _, _, err = resolveSelectors("resources.jobs.test_job.tasks.*.task_key", b, OperationReplace) + _, _, _, err = resolveSelectors("resources.jobs.test_job.tasks.*.task_key", b, OperationReplace) require.Error(t, err) assert.Contains(t, err.Error(), "wildcards not allowed in path") } @@ -273,3 +273,27 @@ func TestYamlFileIndex_SplitBlocksSameFile(t *testing.T) { assert.Equal(t, 0, yamlFileIndex(seq, 1, seqLocations)) // bbb: first in top-level block (NOT 1) assert.Equal(t, 1, yamlFileIndex(seq, 2, seqLocations)) // ccc: second in top-level block } + +func TestElementInOverrideBlock(t *testing.T) { + // seqLocations[0] is always the top-level anchor (merge keeps the reference + // block first); seqLocations[1] is the target override anchor. This holds + // regardless of which block appears first in the file. + topFirst := []dyn.Location{{File: "databricks.yml", Line: 6}, {File: "databricks.yml", Line: 20}} + assert.True(t, elementInOverrideBlock(dyn.Location{File: "databricks.yml", Line: 22}, topFirst)) // target block + assert.False(t, elementInOverrideBlock(dyn.Location{File: "databricks.yml", Line: 8}, topFirst)) // top-level block + + // Target block precedes resources in the file: top-level anchor still first. + targetFirst := []dyn.Location{{File: "databricks.yml", Line: 20}, {File: "databricks.yml", Line: 6}} + assert.True(t, elementInOverrideBlock(dyn.Location{File: "databricks.yml", Line: 8}, targetFirst)) // target block + assert.False(t, elementInOverrideBlock(dyn.Location{File: "databricks.yml", Line: 22}, targetFirst)) // top-level block + + // Single block (no override contribution) is never in an override block. + single := []dyn.Location{{File: "databricks.yml", Line: 6}} + assert.False(t, elementInOverrideBlock(dyn.Location{File: "databricks.yml", Line: 8}, single)) + assert.False(t, elementInOverrideBlock(dyn.Location{}, topFirst)) + + // Multi-file split: a top-level block in a.yml and a target block in b.yml. + multiFile := []dyn.Location{{File: "a.yml", Line: 6}, {File: "b.yml", Line: 4}} + assert.True(t, elementInOverrideBlock(dyn.Location{File: "b.yml", Line: 6}, multiFile)) // target block, other file + assert.False(t, elementInOverrideBlock(dyn.Location{File: "a.yml", Line: 8}, multiFile)) // top-level block +}