From b09caf1d25c2f0c7a5f40c87731b20b55ddaabcb Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 08:19:37 +0000 Subject: [PATCH 01/28] bundle: route config sync write-back to the block that defines each change Co-authored-by: Isaac --- .../config-remote-sync/job_fields/output.txt | 10 +- .../job_multiple_tasks/output.txt | 13 +- .../multiple_files/output.txt | 32 +- .../split/isolation/databricks.yml.tmpl | 34 ++ .../split/isolation/out.test.toml | 4 + .../split/isolation/output.txt | 59 ++ .../config-remote-sync/split/isolation/script | 39 ++ .../split/keyed_edit/databricks.yml.tmpl | 30 + .../split/keyed_edit/out.test.toml | 4 + .../split/keyed_edit/output.txt | 53 ++ .../split/keyed_edit/script | 53 ++ .../split/keyed_remove/databricks.yml.tmpl | 45 ++ .../split/keyed_remove/out.test.toml | 4 + .../split/keyed_remove/output.txt | 87 +++ .../split/keyed_remove/script | 54 ++ .../split/keyed_rename/databricks.yml.tmpl | 43 ++ .../split/keyed_rename/out.test.toml | 4 + .../split/keyed_rename/output.txt | 115 ++++ .../split/keyed_rename/script | 86 +++ .../split/keyed_twoblock/databricks.yml.tmpl | 39 ++ .../split/keyed_twoblock/out.test.toml | 4 + .../split/keyed_twoblock/output.txt | 64 +++ .../split/keyed_twoblock/script | 57 ++ .../split/multifile/databricks.yml.tmpl | 22 + .../split/multifile/out.test.toml | 4 + .../split/multifile/output.txt | 72 +++ .../multifile/overrides/10-first.yml.tmpl | 12 + .../multifile/overrides/20-second.yml.tmpl | 11 + .../config-remote-sync/split/multifile/script | 70 +++ .../split/positional/databricks.yml.tmpl | 30 + .../split/positional/out.test.toml | 4 + .../split/positional/output.txt | 74 +++ .../split/positional/script | 60 ++ .../bundle/config-remote-sync/split/test.toml | 11 + .../task_rename_revert/output.txt | 30 +- bundle/configsync/blockindex.go | 511 ++++++++++++++++++ bundle/configsync/blockindex_test.go | 204 +++++++ bundle/configsync/rename.go | 212 ++++++++ bundle/configsync/resolve.go | 328 +++++++---- bundle/configsync/resolve_test.go | 76 +-- 40 files changed, 2440 insertions(+), 224 deletions(-) create mode 100644 acceptance/bundle/config-remote-sync/split/isolation/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/isolation/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/isolation/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/isolation/script create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_edit/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_edit/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_edit/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_edit/script create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_remove/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_remove/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_remove/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_remove/script create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_rename/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_rename/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_rename/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_rename/script create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_twoblock/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_twoblock/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_twoblock/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_twoblock/script create mode 100644 acceptance/bundle/config-remote-sync/split/multifile/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/multifile/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/multifile/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/multifile/overrides/10-first.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/multifile/overrides/20-second.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/multifile/script create mode 100644 acceptance/bundle/config-remote-sync/split/positional/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/positional/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/positional/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/positional/script create mode 100644 acceptance/bundle/config-remote-sync/split/test.toml create mode 100644 bundle/configsync/blockindex.go create mode 100644 bundle/configsync/blockindex_test.go create mode 100644 bundle/configsync/rename.go diff --git a/acceptance/bundle/config-remote-sync/job_fields/output.txt b/acceptance/bundle/config-remote-sync/job_fields/output.txt index 7e0b7fe07d4..9e4f05db576 100644 --- a/acceptance/bundle/config-remote-sync/job_fields/output.txt +++ b/acceptance/bundle/config-remote-sync/job_fields/output.txt @@ -58,18 +58,14 @@ Resource: resources.jobs.my_job + - samples.nyctaxi.trips environments: - environment_key: default -@@ -25,14 +31,14 @@ +@@ -25,5 +31,5 @@ - ./*.whl job_clusters: - - job_cluster_key: test_cluster + - job_cluster_key: test_cluster_renamed new_cluster: -- spark_version: [[DEFAULT_SPARK_VERSION]] - node_type_id: [NODE_TYPE_ID] - num_workers: 1 -+ spark_version: [[DEFAULT_SPARK_VERSION]] - tasks: - - task_key: main + spark_version: [[DEFAULT_SPARK_VERSION]] +@@ -34,5 +40,5 @@ notebook_task: notebook_path: /Users/{{workspace_user_name}}/notebook - job_cluster_key: test_cluster diff --git a/acceptance/bundle/config-remote-sync/job_multiple_tasks/output.txt b/acceptance/bundle/config-remote-sync/job_multiple_tasks/output.txt index 6c45a66bed1..39ea29bf4a5 100644 --- a/acceptance/bundle/config-remote-sync/job_multiple_tasks/output.txt +++ b/acceptance/bundle/config-remote-sync/job_multiple_tasks/output.txt @@ -97,21 +97,14 @@ Resource: resources.jobs.rename_task_job >>> diff.py databricks.yml.backup2 databricks.yml --- databricks.yml.backup2 +++ databricks.yml -@@ -52,14 +52,14 @@ +@@ -52,5 +52,5 @@ rename_task_job: tasks: - - task_key: b_task -+ - new_cluster: -+ node_type_id: [NODE_TYPE_ID] -+ num_workers: 1 -+ spark_version: 13.3.x-snapshot-scala2.12 ++ - task_key: b_task_renamed notebook_task: notebook_path: /Users/{{workspace_user_name}}/b_task -- new_cluster: -- spark_version: 13.3.x-snapshot-scala2.12 -- node_type_id: [NODE_TYPE_ID] -- num_workers: 1 -+ task_key: b_task_renamed +@@ -61,5 +61,5 @@ - task_key: d_task depends_on: - - task_key: b_task diff --git a/acceptance/bundle/config-remote-sync/multiple_files/output.txt b/acceptance/bundle/config-remote-sync/multiple_files/output.txt index aa943ca185a..2799e540f41 100644 --- a/acceptance/bundle/config-remote-sync/multiple_files/output.txt +++ b/acceptance/bundle/config-remote-sync/multiple_files/output.txt @@ -32,27 +32,13 @@ Resource: resources.jobs.job_two >>> diff.py resources/job1.yml.backup resources/job1.yml --- resources/job1.yml.backup +++ resources/job1.yml -@@ -4,13 +4,13 @@ +@@ -4,5 +4,5 @@ max_concurrent_runs: 1 tasks: - - task_key: c_task -+ - depends_on: -+ - task_key: b_task -+ new_cluster: -+ node_type_id: [NODE_TYPE_ID] -+ num_workers: 1 -+ spark_version: 13.3.x-snapshot-scala2.12 ++ - task_key: c_task_renamed notebook_task: notebook_path: /Users/{{workspace_user_name}}/c_task -- new_cluster: -- spark_version: 13.3.x-snapshot-scala2.12 -- node_type_id: [NODE_TYPE_ID] -- num_workers: 1 -- depends_on: -- - task_key: b_task -+ task_key: c_task_renamed - - task_key: a_task - notebook_task: @@ -21,3 +21,10 @@ num_workers: 1 depends_on: @@ -71,26 +57,16 @@ Resource: resources.jobs.job_two >>> diff.py resources/job2.yml.backup resources/job2.yml --- resources/job2.yml.backup +++ resources/job2.yml -@@ -2,13 +2,13 @@ +@@ -2,7 +2,7 @@ jobs: job_two: - max_concurrent_runs: 2 + max_concurrent_runs: 10 tasks: - - task_key: run_pipeline -+ - new_cluster: -+ node_type_id: [NODE_TYPE_ID] -+ num_workers: 1 -+ spark_version: 13.3.x-snapshot-scala2.12 ++ - task_key: run_pipeline_renamed notebook_task: notebook_path: /Users/{{workspace_user_name}}/1 -- new_cluster: -- spark_version: 13.3.x-snapshot-scala2.12 -- node_type_id: [NODE_TYPE_ID] -- num_workers: 1 -+ task_key: run_pipeline_renamed - - task_key: etl_pipeline - notebook_task: @@ -18,5 +18,9 @@ node_type_id: [NODE_TYPE_ID] num_workers: 2 diff --git a/acceptance/bundle/config-remote-sync/split/isolation/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/split/isolation/databricks.yml.tmpl new file mode 100644 index 00000000000..c5bc72f02f8 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/isolation/databricks.yml.tmpl @@ -0,0 +1,34 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +# job_a's task list is split across both blocks and one task, "shared", is defined +# in both. job_b is an ordinary single-block job. +# +# The point of the fixture: an unrelated resource's change must still be applied +# in the same run as a structural change to a split element. The sync is +# unattended, so one harder change must never stop the rest. +resources: + jobs: + job_a: + tasks: + - task_key: shared + max_retries: 1 + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/shared + + job_b: + max_concurrent_runs: 1 + tasks: + - task_key: simple + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/simple + +targets: + dev: + mode: development + resources: + jobs: + job_a: + tasks: + - task_key: shared + timeout_seconds: 45 diff --git a/acceptance/bundle/config-remote-sync/split/isolation/out.test.toml b/acceptance/bundle/config-remote-sync/split/isolation/out.test.toml new file mode 100644 index 00000000000..4b5914daa2c --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/isolation/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = false +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/split/isolation/output.txt b/acceptance/bundle/config-remote-sync/split/isolation/output.txt new file mode 100644 index 00000000000..20ecba57699 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/isolation/output.txt @@ -0,0 +1,59 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Rename the two-block task on job_a, and edit job_b in the same run +=== Sync +Detected changes in 2 resource(s): + +Resource: resources.jobs.job_a + tasks[task_key='shared']: remove + tasks[task_key='shared_renamed']: add + +Resource: resources.jobs.job_b + max_concurrent_runs: replace + + + +=== job_b is updated, and job_a's rename is written in both blocks + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -12,5 +12,5 @@ + job_a: + tasks: +- - task_key: shared ++ - task_key: shared_renamed + max_retries: 1 + notebook_task: +@@ -18,5 +18,5 @@ + + job_b: +- max_concurrent_runs: 1 ++ max_concurrent_runs: 6 + tasks: + - task_key: simple +@@ -31,4 +31,4 @@ + job_a: + tasks: +- - task_key: shared ++ - task_key: shared_renamed + timeout_seconds: 45 + +>>> grep -c max_concurrent_runs: 6 databricks.yml +1 + +>>> grep -c task_key: shared_renamed databricks.yml +2 + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.job_a + delete resources.jobs.job_b + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/isolation/script b/acceptance/bundle/config-remote-sync/split/isolation/script new file mode 100644 index 00000000000..b1320eed712 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/isolation/script @@ -0,0 +1,39 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve -t dev +} +trap cleanup EXIT + +$CLI bundle deploy -t dev +job_a_id="$(read_id.py job_a)" +job_b_id="$(read_id.py job_b)" + + +# A rename of the two-block task on job_a, and a plain scalar edit on job_b, in the +# SAME run. job_b's edit is independent of anything job_a does, so it must be +# applied whether or not job_a's rename can be placed. +title "Rename the two-block task on job_a, and edit job_b in the same run" +edit_resource.py jobs $job_a_id <>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -29,2 +29,3 @@ + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/alpha ++ timeout_seconds: 111 + +=== Edit mu, a top-level task that is not first after the merge sort +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.split_job + tasks[task_key='mu'].timeout_seconds: add + + + +=== Only mu in the top-level block gains timeout_seconds: 222 + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -18,4 +18,5 @@ + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/mu ++ timeout_seconds: 222 + + targets: + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.split_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/keyed_edit/script b/acceptance/bundle/config-remote-sync/split/keyed_edit/script new file mode 100644 index 00000000000..dcfc1028455 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/keyed_edit/script @@ -0,0 +1,53 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve -t dev +} +trap cleanup EXIT + +$CLI bundle deploy -t dev +job_id="$(read_id.py split_job)" + + +# alpha is the only task in the target block, so it must be written at index 0 of +# that block. Its merged index is also 0, but the top-level block's index 0 is +# zeta -- a naive merged-index write lands on zeta instead. +title "Edit alpha, defined only in the target block" +edit_resource.py jobs $job_id <>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -13,7 +13,4 @@ + remove_job: + tasks: +- - task_key: gamma +- notebook_task: +- notebook_path: /Users/{{workspace_user_name}}/gamma + - task_key: beta + notebook_task: +@@ -36,8 +33,5 @@ + jobs: + remove_job: +- tasks: +- - task_key: alpha +- notebook_task: +- notebook_path: /Users/{{workspace_user_name}}/alpha ++ tasks: [] + twoblock_remove_job: + tasks: + +=== Remove the task defined in BOTH blocks +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.twoblock_remove_job + tasks[task_key='both']: remove + + + +=== 'both' is gone from both blocks; 'keep' survives + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -19,8 +19,4 @@ + twoblock_remove_job: + tasks: +- - task_key: both +- max_retries: 2 +- notebook_task: +- notebook_path: /Users/{{workspace_user_name}}/both + - task_key: keep + notebook_task: +@@ -35,5 +31,3 @@ + tasks: [] + twoblock_remove_job: +- tasks: +- - task_key: both +- timeout_seconds: 30 ++ tasks: [] + +>>> grep -c task_key: both databricks.yml +0 + +Exit code: 1 + +>>> grep -c task_key: keep databricks.yml +1 + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.remove_job + delete resources.jobs.twoblock_remove_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/keyed_remove/script b/acceptance/bundle/config-remote-sync/split/keyed_remove/script new file mode 100644 index 00000000000..73998fa670b --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/keyed_remove/script @@ -0,0 +1,54 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve -t dev +} +trap cleanup EXIT + +$CLI bundle deploy -t dev +remove_job_id="$(read_id.py remove_job)" +twoblock_job_id="$(read_id.py twoblock_remove_job)" + + +# Remove one task from each block in a single run. Exactly gamma and alpha must +# disappear; beta must survive even though the removals shift indices in both +# blocks. +title "Remove gamma from the top-level block and alpha from the target block" +edit_resource.py jobs $remove_job_id <>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -14,5 +14,5 @@ + rename_job: + tasks: +- - task_key: shared ++ - task_key: a_shared + max_retries: 1 + notebook_task: +@@ -37,7 +37,7 @@ + rename_job: + tasks: +- - task_key: shared ++ - task_key: a_shared + timeout_seconds: 45 +- - task_key: solo ++ - task_key: z_solo + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/solo + +>>> grep -c task_key: a_shared databricks.yml +2 + +>>> grep -c task_key: z_solo databricks.yml +1 + +=== Remove a task and rename a later one in the same run, single block +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.shift_job + tasks[task_key='aaa']: remove + tasks[task_key='mmm']: remove + tasks[task_key='mmm2']: add + + + +=== aaa is gone, mmm became mmm2, zzz keeps its name and its max_retries + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -23,7 +23,5 @@ + shift_job: + tasks: +- - task_key: aaa +- max_retries: 1 +- - task_key: mmm ++ - task_key: mmm2 + max_retries: 2 + - task_key: zzz + +>>> grep -c task_key: zzz databricks.yml +1 + +=== Rename the two-block task AND edit one of its fields in the same run +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.rename_job + tasks[task_key='a_shared']: remove + tasks[task_key='b_shared']: add + tasks[task_key='z_solo']: replace + + + +=== Left unapplied: the split is intact and timeout_seconds stays target-scoped + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -37,5 +37,5 @@ + - task_key: a_shared + timeout_seconds: 45 +- - task_key: z_solo +- notebook_task: ++ - notebook_task: + notebook_path: /Users/{{workspace_user_name}}/solo ++ task_key: z_solo + +>>> grep -c task_key: a_shared databricks.yml +2 + +>>> grep -c timeout_seconds: 45 databricks.yml +1 + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.rename_job + delete resources.jobs.shift_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/keyed_rename/script b/acceptance/bundle/config-remote-sync/split/keyed_rename/script new file mode 100644 index 00000000000..88b8106ed55 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/keyed_rename/script @@ -0,0 +1,86 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve -t dev +} +trap cleanup EXIT + +$CLI bundle deploy -t dev +job_id="$(read_id.py rename_job)" +shift_job_id="$(read_id.py shift_job)" + + +# Rename the two-block task and the target-only task in one run. Each key rewrite +# has to land on its own element: "a_shared" in both blocks, "z_solo" in the target +# block only. Their sort order is the reverse of the block order, so pairing by +# order instead of by identity swaps the two. +title "Rename a task defined in both blocks and a target-only task" +edit_resource.py jobs $job_id <>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -38,2 +38,3 @@ + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/omega ++ timeout_seconds: 303 + +=== Edit a field per block, a field defined in both, and add a new field +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.twoblock_job + tasks[task_key='shared'].max_retries: replace + tasks[task_key='shared'].min_retry_interval_millis: add + tasks[task_key='shared'].timeout_seconds: replace + + + +=== Each edit lands on the definition that was deployed + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -21,4 +21,5 @@ + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/shared ++ min_retry_interval_millis: 5000 + - task_key: nu + notebook_task: +@@ -33,6 +34,6 @@ + tasks: + - task_key: shared +- max_retries: 2 +- timeout_seconds: 60 ++ max_retries: 9 ++ timeout_seconds: 900 + - task_key: omega + notebook_task: + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.twoblock_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/keyed_twoblock/script b/acceptance/bundle/config-remote-sync/split/keyed_twoblock/script new file mode 100644 index 00000000000..2b7d8cf82ce --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/keyed_twoblock/script @@ -0,0 +1,57 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve -t dev +} +trap cleanup EXIT + +$CLI bundle deploy -t dev +job_id="$(read_id.py twoblock_job)" + + +# omega is defined only in the target block, but its sibling "shared" contributes +# to both blocks. Editing omega must land on omega and must leave the two-block +# element completely untouched. +title "Edit omega, whose sibling is defined in two blocks" +edit_resource.py jobs $job_id <>> diff.py databricks.yml.backup databricks.yml + +=== overrides/10-first.yml is untouched + +>>> diff.py overrides/10-first.yml.backup overrides/10-first.yml + +=== overrides/20-second.yml gains timeout_seconds: 777 + +>>> diff.py overrides/20-second.yml.backup overrides/20-second.yml +--- overrides/20-second.yml.backup ++++ overrides/20-second.yml +@@ -8,4 +8,5 @@ + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/second ++ timeout_seconds: 777 + - task_key: in_both_files + timeout_seconds: 30 + +=== Edit one field per file on the task declared in both override files +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.multifile_job + tasks[task_key='in_both_files'].max_retries: replace + tasks[task_key='in_both_files'].timeout_seconds: replace + + + +=== max_retries updates the first file, timeout_seconds the second + +>>> diff.py overrides/10-first.yml.backup overrides/10-first.yml +--- overrides/10-first.yml.backup ++++ overrides/10-first.yml +@@ -10,3 +10,3 @@ + notebook_path: /Users/{{workspace_user_name}}/first + - task_key: in_both_files +- max_retries: 1 ++ max_retries: 9 + +>>> diff.py overrides/20-second.yml.backup overrides/20-second.yml +--- overrides/20-second.yml.backup ++++ overrides/20-second.yml +@@ -10,3 +10,3 @@ + timeout_seconds: 777 + - task_key: in_both_files +- timeout_seconds: 30 ++ timeout_seconds: 900 + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.multifile_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/multifile/overrides/10-first.yml.tmpl b/acceptance/bundle/config-remote-sync/split/multifile/overrides/10-first.yml.tmpl new file mode 100644 index 00000000000..dd4438f6a98 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/multifile/overrides/10-first.yml.tmpl @@ -0,0 +1,12 @@ +targets: + dev: + mode: development + resources: + jobs: + multifile_job: + tasks: + - task_key: from_first_file + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/first + - task_key: in_both_files + max_retries: 1 diff --git a/acceptance/bundle/config-remote-sync/split/multifile/overrides/20-second.yml.tmpl b/acceptance/bundle/config-remote-sync/split/multifile/overrides/20-second.yml.tmpl new file mode 100644 index 00000000000..6dffbbfca0e --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/multifile/overrides/20-second.yml.tmpl @@ -0,0 +1,11 @@ +targets: + dev: + resources: + jobs: + multifile_job: + tasks: + - task_key: from_second_file + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/second + - task_key: in_both_files + timeout_seconds: 30 diff --git a/acceptance/bundle/config-remote-sync/split/multifile/script b/acceptance/bundle/config-remote-sync/split/multifile/script new file mode 100644 index 00000000000..f8dc6c4cee1 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/multifile/script @@ -0,0 +1,70 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml +envsubst < overrides/10-first.yml.tmpl > overrides/10-first.yml +envsubst < overrides/20-second.yml.tmpl > overrides/20-second.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve -t dev +} +trap cleanup EXIT + +$CLI bundle deploy -t dev +job_id="$(read_id.py multifile_job)" + + +# from_second_file is defined in overrides/20-second.yml. The edit must be written +# to that file only -- not to the top-level databricks.yml and not to the sibling +# override file, and without creating a duplicate subtree anywhere. +title "Edit a task defined in the second included file" +edit_resource.py jobs $job_id <>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -15,5 +15,5 @@ + clusters: + - label: default +- num_workers: 2 ++ num_workers: 5 + libraries: + - notebook: +@@ -28,3 +28,3 @@ + clusters: + - label: maintenance +- num_workers: 1 ++ num_workers: 3 + +=== Remove the target-block cluster (length change) +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.pipelines.split_pipeline + clusters: replace + clusters[1].num_workers: remove + + + +=== The top-level cluster is untouched; the target entry is left as a remnant + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -28,3 +28,2 @@ + clusters: + - label: maintenance +- num_workers: 3 + +>>> grep -c label: default databricks.yml +1 + +>>> grep -c label: maintenance databricks.yml +1 + +>>> grep -c num_workers databricks.yml +1 + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.pipelines.split_pipeline + +This action will result in the deletion of the following Lakeflow Spark Declarative Pipelines along with the +Streaming Tables (STs) and Materialized Views (MVs) managed by them: + delete resources.pipelines.split_pipeline + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/positional/script b/acceptance/bundle/config-remote-sync/split/positional/script new file mode 100644 index 00000000000..fa7b1d54db1 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/positional/script @@ -0,0 +1,60 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve -t dev +} +trap cleanup EXIT + +$CLI bundle deploy -t dev +pipeline_id="$(read_id.py split_pipeline)" + + +# Length-preserving field edits on both elements of a positional list that is +# split across two blocks. Each edit must land in its own block. +title "Edit num_workers on the top-level cluster and on the target cluster" +edit_resource.py pipelines $pipeline_id <>> diff.py databricks.yml.backup databricks.yml --- databricks.yml.backup +++ databricks.yml -@@ -6,11 +6,11 @@ +@@ -6,5 +6,5 @@ sample_job: tasks: - - task_key: new_task -- notebook_task: -- notebook_path: /Users/{{workspace_user_name}}/new_task -- new_cluster: -- spark_version: 13.3.x-snapshot-scala2.12 -+ - new_cluster: - node_type_id: [NODE_TYPE_ID] - num_workers: 1 -+ spark_version: 13.3.x-snapshot-scala2.12 -+ notebook_task: -+ notebook_path: '/Users/{{workspace_user_name}}/new_task' -+ task_key: new_task_2 - - targets: ++ - task_key: new_task_2 + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/new_task === Rename task back to new_task remotely @@ -49,13 +39,13 @@ Resource: resources.jobs.sample_job >>> diff.py databricks.yml.backup databricks.yml --- databricks.yml.backup +++ databricks.yml -@@ -12,5 +12,5 @@ +@@ -6,5 +6,5 @@ + sample_job: + tasks: +- - task_key: new_task_2 ++ - task_key: new_task notebook_task: - notebook_path: '/Users/{{workspace_user_name}}/new_task' -- task_key: new_task_2 -+ task_key: new_task - - targets: + notebook_path: /Users/{{workspace_user_name}}/new_task >>> [CLI] bundle destroy --auto-approve The following resources will be deleted: diff --git a/bundle/configsync/blockindex.go b/bundle/configsync/blockindex.go new file mode 100644 index 00000000000..9a76c59540c --- /dev/null +++ b/bundle/configsync/blockindex.go @@ -0,0 +1,511 @@ +package configsync + +import ( + "cmp" + "context" + "errors" + "fmt" + "maps" + "os" + "slices" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/log" + "github.com/databricks/cli/libs/structs/structpath" +) + +// A sequence field of a resource may be defined in more than one physical YAML +// region: the top-level resources... block and the +// targets..resources... override block, either of +// which may live in its own included file. Loading merges those regions into one +// sequence, and keyed sequences are also sorted by key, so an index into the +// merged sequence does not address any single region. Write-back has to map a +// merged position back to a region and to the position inside that region. +// There are exactly two kinds of region, so a block is a file plus which of the +// two it is. Both kinds may occur in the same file, so the file alone does not +// identify a block, and one target's override may span several files, so the kind +// alone does not either. +type sourceBlock struct { + // override is false for the top-level block and true for the selected target's + // override block. + override bool + file string +} + +// scopeKey identifies this block for index bookkeeping, so operations on one block +// cannot shift positions recorded for another. +func (b sourceBlock) scopeKey() string { + if b.override { + return "override\x00" + b.file + "\x00" + } + return "toplevel\x00" + b.file + "\x00" +} + +// blockResolver answers which physical block a merged value came from. +// +// It works by location: a merged value keeps the locations of the entries it was +// loaded from, and merging accumulates them (see libs/dyn/merge), so a value +// assembled from two blocks reports a location in each. Selecting a target folds +// its overrides into the resources tree and drops the targets subtree, so the +// blocks are recovered by parsing the contributing files again, where the two +// regions are still separate. +type blockResolver struct { + // blocks holds one parsed file per contributing file, keyed by block, so a + // path relative to a block can be looked up inside it. + blocks map[sourceBlock]dyn.Value + target string + byLocation map[dyn.Location]sourceBlock +} + +// newBlockResolver builds the location -> block mapping for the bundle's +// resources. It returns nil when the source cannot be re-read, in which case +// callers keep their existing single-block behaviour. +// +// Only the files the merged configuration actually references are read, and they +// are parsed directly rather than reloaded through the mutator pipeline: the +// pipeline resolves includes, reports through logdiag and executes the bundle's +// preinit script, none of which belongs to reading back a source location. +func newBlockResolver(ctx context.Context, b *bundle.Bundle) *blockResolver { + root := b.Config.Value() + if !root.IsValid() { + return nil + } + + r := &blockResolver{ + blocks: make(map[sourceBlock]dyn.Value), + target: b.Config.Bundle.Target, + byLocation: make(map[dyn.Location]sourceBlock), + } + + sourceFiles := slices.Sorted(maps.Keys(referencedFiles(root))) + for _, file := range sourceFiles { + contents, err := os.ReadFile(file) + if err != nil { + log.Debugf(ctx, "config-remote-sync: cannot read %s, treating its sequences as unsplit: %v", file, err) + continue + } + parsed, diags := config.LoadFromBytes(file, contents) + if diags.HasError() { + log.Debugf(ctx, "config-remote-sync: cannot parse %s, treating its sequences as unsplit: %v", file, diags.Error()) + continue + } + r.registerBlock(parsed.Value(), sourceBlock{file: file}) + if r.target != "" { + r.registerBlock(parsed.Value(), sourceBlock{override: true, file: file}) + } + } + + if len(r.byLocation) == 0 { + return nil + } + return r +} + +// referencedFiles returns the files the merged configuration was loaded from. +func referencedFiles(root dyn.Value) map[string]struct{} { + files := map[string]struct{}{} + _ = dyn.WalkReadOnly(root, func(_ dyn.Path, v dyn.Value) error { + for _, location := range v.Locations() { + if location.File != "" { + files[location.File] = struct{}{} + } + } + return nil + }) + return files +} + +// registerBlock records that block exists and maps every location under its +// resources subtree back to it, so a merged value can later be traced to the +// region it was written in. Does nothing when the file has no such region, which +// is why r.blocks doubles as the set of blocks that exist. +func (r *blockResolver) registerBlock(parsed dyn.Value, block sourceBlock) { + subtree, err := dyn.GetByPath(parsed, r.regionPath(block, dyn.NewPath(dyn.Key("resources")))) + if err != nil { + return + } + + // Keep the parsed file: resolving a path inside this block needs the tree it + // came from, and the entry also marks the block as present for sortedBlocks. + r.blocks[block] = parsed + _ = dyn.WalkReadOnly(subtree, func(_ dyn.Path, v dyn.Value) error { + for _, location := range v.Locations() { + // A file only carries locations of its own, so a location seen here + // belongs to this block. First writer wins: an outer node accumulates + // its children's locations, but the innermost node that owns a location + // is the one walked last. + if location.File != block.file { + continue + } + if _, ok := r.byLocation[location]; !ok { + r.byLocation[location] = block + } + } + return nil + }) +} + +// regionPath prefixes a resources-relative path with the region it belongs to. +func (r *blockResolver) regionPath(block sourceBlock, path dyn.Path) dyn.Path { + if !block.override { + return path + } + return append(dyn.NewPath(dyn.Key("targets"), dyn.Key(r.target)), path...) +} + +// candidatePath renders a resolved path the way the patch layer addresses it, +// which for an override block includes the targets. prefix. +func (r *blockResolver) candidatePath(block sourceBlock, path string) string { + if !block.override { + return path + } + return "targets." + r.target + "." + path +} + +// sortedBlocks lists the known blocks with the top-level ones first and a total +// order within each scope, so a choice between blocks never depends on map or +// location iteration order. +func (r *blockResolver) sortedBlocks() []sourceBlock { + blocks := slices.Collect(maps.Keys(r.blocks)) + slices.SortFunc(blocks, compareBlocks) + return blocks +} + +// compareBlocks orders top-level blocks before target blocks, then by file. +func compareBlocks(a, b sourceBlock) int { + if a.override != b.override { + if !a.override { + return -1 + } + return 1 + } + return cmp.Compare(a.file, b.file) +} + +// blocksOf returns the distinct blocks that contributed to value, sorted with the +// top-level block first. More than one result means the value is assembled from +// several regions and has no single source location. +func (r *blockResolver) blocksOf(value dyn.Value) []sourceBlock { + var blocks []sourceBlock + for _, location := range value.Locations() { + block, ok := r.byLocation[location] + if !ok { + continue + } + if !slices.Contains(blocks, block) { + blocks = append(blocks, block) + } + } + // Order is total and independent of how locations happened to accumulate, so + // callers that prefer the top-level block get a deterministic answer. + slices.SortFunc(blocks, compareBlocks) + return blocks +} + +// winningBlock returns the block a change to value has to be written to when +// several blocks define it. +// +// The target override is preferred. For a scalar that is also the block that won +// the merge, because mergePrimitive keeps the incoming value and records its +// location first, so writing the other copy would leave the effective value +// unchanged. For a map or sequence, merging records the base's location first +// (mergeMap, mergeSequence) even though the target contributed keys, so the first +// location is not a statement about precedence; the target is still the narrower +// scope and the one a remote edit made under that target belongs in. +func (r *blockResolver) winningBlock(value dyn.Value) (sourceBlock, bool) { + blocks := r.blocksOf(value) + if len(blocks) == 0 { + return sourceBlock{}, false + } + for _, block := range blocks { + if block.override { + return block, true + } + } + return blocks[0], true +} + +// indexWithinBlock returns the position of element in the sequence that block +// writes at sequencePath, where sequencePath is relative to the block. +func (r *blockResolver) indexWithinBlock(block sourceBlock, sequencePath dyn.Path, element dyn.Value) (int, bool) { + parsed, ok := r.blocks[block] + if !ok { + return 0, false + } + sequence, err := dyn.GetByPath(parsed, r.regionPath(block, sequencePath)) + if err != nil { + return 0, false + } + entries, ok := sequence.AsSequence() + if !ok { + return 0, false + } + + locations := make(map[dyn.Location]struct{}, len(element.Locations())) + for _, location := range element.Locations() { + locations[location] = struct{}{} + } + + // A region's sequence is the concatenation of the entries contributed by each + // included file, but the write targets one file, so the index has to be + // counted among that file's entries only. + local := 0 + for _, entry := range entries { + entryFile := entry.Location().File + if entryFile != block.file { + continue + } + for _, location := range entry.Locations() { + if _, ok := locations[location]; ok { + return local, true + } + } + local++ + } + return 0, false +} + +// errAmbiguousBlock reports a change that cannot be attributed to one physical +// block. Leaving such a change unapplied is preferable to writing it to a guessed +// location, since the sync runs unattended and a later run can retry. +var errAmbiguousBlock = errors.New("change cannot be attributed to a single source block") + +// singleDestination maps a change onto the one block that owns it, rewriting +// merged sequence indices into block-local ones. +// +// The element the change addresses decides the block: an element defined in +// exactly one block is written there. When the addressed element is assembled from +// several blocks, the leaf field decides instead, because merging records the +// location of each field separately. If neither identifies a single block the +// change is ambiguous and is reported as such. +func (r *blockResolver) singleDestination(change resolvedChange) (routeDestination, error) { + block, err := r.blockFor(change) + if err != nil { + return routeDestination{}, err + } + + path, err := r.pathWithinBlock(block, change) + if err != nil { + return routeDestination{}, err + } + return routeDestination{block: block, path: path}, nil +} + +// routeDestination is one physical place a change has to be written. +type routeDestination struct { + block sourceBlock + path *structpath.PatternNode +} + +// routeDestinations returns every physical place a change has to be written. +// +// A change to a field has one destination, because a field has one definition. +// A change that addresses a whole sequence element has one destination per block +// that defines the element: an element assembled from two blocks has a part in +// each, so removing it means deleting both parts and renaming it means rewriting +// the key in both. Writing per block keeps the split intact instead of collapsing +// the element into one scope. +func (r *blockResolver) routeDestinations(change resolvedChange) ([]routeDestination, error) { + if !addressesWholeElement(change) { + destination, err := r.singleDestination(change) + if err != nil { + return nil, err + } + return []routeDestination{destination}, nil + } + + element := change.steps[len(change.steps)-1].element + blocks := r.blocksOf(element) + if len(blocks) == 0 { + return nil, fmt.Errorf("%w: no source location for the addressed element", errAmbiguousBlock) + } + + destinations := make([]routeDestination, 0, len(blocks)) + for _, block := range blocks { + path, err := r.pathWithinBlock(block, change) + if err != nil { + return nil, err + } + destinations = append(destinations, routeDestination{block: block, path: path}) + } + return destinations, nil +} + +// addressesWholeElement reports whether the change targets an existing sequence +// element itself rather than something inside it. Only such a change can need more +// than one destination, since only it can span the blocks the element is built +// from. A new element is excluded: it exists in no block yet, so it has no parts to +// span and is placed by blockForNewElement instead. +func addressesWholeElement(change resolvedChange) bool { + if len(change.steps) == 0 { + return false + } + for _, step := range change.steps { + if step.newElement { + return false + } + } + last := change.steps[len(change.steps)-1] + return len(change.path.AsSlice()) == last.component+1 +} + +// blockFor picks the block a change belongs to. +func (r *blockResolver) blockFor(change resolvedChange) (sourceBlock, error) { + // A new element has no source of its own; it is placed relative to the + // sequence that receives it. + for _, step := range change.steps { + if step.newElement { + return r.blockForNewElement(change) + } + } + + // No sequence on the path: route by the leaf's own location. + if len(change.steps) == 0 { + if block, ok := r.winningBlock(change.leaf); ok { + return block, nil + } + return sourceBlock{}, fmt.Errorf("%w: no source location for the change", errAmbiguousBlock) + } + + element := change.steps[len(change.steps)-1].element + blocks := r.blocksOf(element) + if len(blocks) == 0 { + return sourceBlock{}, fmt.Errorf("%w: no source location for the addressed element", errAmbiguousBlock) + } + if len(blocks) == 1 { + return blocks[0], nil + } + + // The element is assembled from several blocks. A field of it is not: it is + // written in one block, or written in both and one of them wins, so a + // field-level change routes by the field's own location. + if change.leaf.IsValid() { + if block, ok := r.winningBlock(change.leaf); ok { + return block, nil + } + return sourceBlock{}, fmt.Errorf("%w: element is defined in %d blocks and the field has no source location", errAmbiguousBlock, len(blocks)) + } + + // An invalid leaf on a path that goes past the element means the change adds a + // field the element does not have yet. There is no location to route by, but it + // still needs a defined destination, so it goes to the block declaring the + // resource rather than into a target-specific scope. + if !addressesWholeElement(change) { + return declaringBlock(blocks), nil + } + + // The change addresses the element itself. Removing or recreating an element + // built from several blocks cannot be expressed against just one of them. + return sourceBlock{}, fmt.Errorf("%w: element is defined in %d blocks", errAmbiguousBlock, len(blocks)) +} + +// declaringBlock picks the block a resource is declared in, which is the top-level +// one whenever there is one. blocksOf and sortedBlocks both order top-level first. +func declaringBlock(blocks []sourceBlock) sourceBlock { + for _, block := range blocks { + if !block.override { + return block + } + } + return blocks[0] +} + +// blockForNewElement chooses where an element that exists in no source block +// should be written: the block that defines the sequence, or when several do, the +// block that declares the resource. Adding to the resource's own block keeps a new +// element out of a target-specific scope the user did not ask for. +func (r *blockResolver) blockForNewElement(change resolvedChange) (sourceBlock, error) { + step := change.steps[len(change.steps)-1] + + // A nested sequence is reached through the enclosing elements, which are + // already placed: route by the innermost one that exists in the source, since + // a new element belongs in the same block as its parent. + for i := len(change.steps) - 2; i >= 0; i-- { + enclosing := change.steps[i] + if enclosing.newElement { + continue + } + if blocks := r.blocksOf(enclosing.element); len(blocks) == 1 { + return blocks[0], nil + } + } + + blocks := r.blocksDefiningSequence(step.sequencePath) + if len(blocks) == 0 { + return sourceBlock{}, fmt.Errorf("%w: no block defines the sequence receiving the new element", errAmbiguousBlock) + } + return declaringBlock(blocks), nil +} + +// blocksDefiningSequence returns the blocks that write the sequence at +// sequencePath. An existing value is traced through its locations instead; this is +// for a value that does not exist yet, where only the receiving sequence is known. +func (r *blockResolver) blocksDefiningSequence(sequencePath dyn.Path) []sourceBlock { + var blocks []sourceBlock + for _, block := range r.sortedBlocks() { + sequence, err := dyn.GetByPath(r.blocks[block], r.regionPath(block, sequencePath)) + if err != nil { + continue + } + for _, location := range sequence.Locations() { + if location.File == block.file && !slices.Contains(blocks, block) { + blocks = append(blocks, block) + } + } + } + return blocks +} + +// pathWithinBlock rewrites a change's path so every sequence index addresses the +// element's position inside block rather than its position in the merged list. +func (r *blockResolver) pathWithinBlock(block sourceBlock, change resolvedChange) (*structpath.PatternNode, error) { + nodes := change.path.AsSlice() + indices := make([]int, len(change.steps)) + for i, step := range change.steps { + if step.newElement { + // Keeps the [*] placeholder; the patcher appends to the block's + // sequence. + indices[i] = -1 + continue + } + // A nested sequence is reached through the outer sequences on the path, + // whose indices refer to the merged view. Rewrite them to the block's own + // indices, otherwise the lookup addresses the wrong parent element. + sequencePath := slices.Clone(step.sequencePath) + for j := range i { + outer := change.steps[j] + if indices[j] < 0 || len(outer.sequencePath) >= len(sequencePath) { + continue + } + sequencePath[len(outer.sequencePath)] = dyn.Index(indices[j]) + } + + index, ok := r.indexWithinBlock(block, sequencePath, step.element) + if !ok { + return nil, fmt.Errorf("%w: element has no position in %s", errAmbiguousBlock, block.file) + } + indices[i] = index + } + + var result *structpath.PatternNode + next := 0 + for component, node := range nodes { + if next < len(change.steps) && change.steps[next].component == component { + if indices[next] < 0 { + result = structpath.NewPatternBracketStar(result) + } else { + result = structpath.NewPatternIndex(result, indices[next]) + } + next++ + continue + } + key, ok := node.StringKey() + if !ok { + return nil, fmt.Errorf("%w: unsupported path component in %s", errAmbiguousBlock, change.path.String()) + } + result = structpath.NewPatternStringKey(result, key) + } + return result, nil +} diff --git a/bundle/configsync/blockindex_test.go b/bundle/configsync/blockindex_test.go new file mode 100644 index 00000000000..7a2df2e420f --- /dev/null +++ b/bundle/configsync/blockindex_test.go @@ -0,0 +1,204 @@ +package configsync + +import ( + "os" + "path/filepath" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/mutator" + "github.com/databricks/cli/bundle/config/mutator/resourcemutator" + "github.com/databricks/cli/libs/logdiag" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// loadSplitBundle loads a bundle the way the sync command does, so a target's +// overrides are merged into the resources tree and keyed sequences are merged by +// key. Merging keyed sequences happens in the initialize phase, after the target +// is selected, so it has to be applied explicitly here. +func loadSplitBundle(t *testing.T, target, content string) (*bundle.Bundle, *blockResolver) { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "databricks.yml"), []byte(content), 0o600)) + + ctx := logdiag.InitContext(t.Context()) + b, err := bundle.Load(ctx, dir) + require.NoError(t, err) + mutator.DefaultMutators(ctx, b) + bundle.ApplyContext(ctx, b, mutator.SelectTarget(target)) + bundle.ApplySeqContext(ctx, b, resourcemutator.MergeJobTasks()) + + blocks := newBlockResolver(ctx, b) + require.NotNil(t, blocks) + return b, blocks +} + +// A task defined in both blocks, where one scalar and one map are contributed by +// each. Merging records a scalar's winning location first but a map's *base* +// location first, so the two kinds disagree about what Locations()[0] means. +const twoBlockTaskBundle = `bundle: + name: split + +resources: + jobs: + j: + tasks: + - task_key: shared + max_retries: 1 + email_notifications: + on_failure: + - base@example.com + - task_key: top_only + max_retries: 5 + +targets: + dev: + resources: + jobs: + j: + tasks: + - task_key: shared + max_retries: 2 + email_notifications: + no_alert_for_skipped_runs: true + - task_key: target_only + max_retries: 6 +` + +func TestBlockResolverRoutesToDefiningBlock(t *testing.T) { + b, blocks := loadSplitBundle(t, "dev", twoBlockTaskBundle) + + tests := []struct { + name string + path string + override bool + want string + }{ + { + name: "field defined only top-level", + path: "resources.jobs.j.tasks[task_key='top_only'].max_retries", + override: false, + want: "resources.jobs.j.tasks[1].max_retries", + }, + { + name: "field defined only in the target", + path: "resources.jobs.j.tasks[task_key='target_only'].max_retries", + override: true, + want: "resources.jobs.j.tasks[1].max_retries", + }, + { + // A scalar in both blocks: the target's value is the deployed one, so + // writing the top-level copy would not change the effective value. + name: "scalar defined in both blocks routes to the target", + path: "resources.jobs.j.tasks[task_key='shared'].max_retries", + override: true, + want: "resources.jobs.j.tasks[0].max_retries", + }, + { + // A leaf under the map still has exactly one definition of its own. + name: "leaf under a two-block map routes to its own block", + path: "resources.jobs.j.tasks[task_key='shared'].email_notifications.on_failure", + override: false, + want: "resources.jobs.j.tasks[0].email_notifications.on_failure", + }, + { + // The map as a whole: mergeMap records the base's location first even + // though the target contributed keys, so the first location says nothing + // about precedence and the narrower scope is used instead. + name: "map defined in both blocks routes to the target", + path: "resources.jobs.j.tasks[task_key='shared'].email_notifications", + override: true, + want: "resources.jobs.j.tasks[0].email_notifications", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resolved, err := resolveSelectors(tc.path, b, OperationReplace) + require.NoError(t, err) + + destination, err := blocks.singleDestination(resolved) + require.NoError(t, err) + assert.Equal(t, tc.override, destination.block.override) + assert.Equal(t, tc.want, destination.path.String()) + }) + } +} + +func TestBlockResolverRoutesElementToEveryDefiningBlock(t *testing.T) { + b, blocks := loadSplitBundle(t, "dev", twoBlockTaskBundle) + + // Removing or renaming an element defined in both blocks has to reach the part + // in each, and each part has its own index inside its own block. + resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='shared']", b, OperationRemove) + require.NoError(t, err) + + destinations, err := blocks.routeDestinations(resolved) + require.NoError(t, err) + + got := map[bool]string{} + for _, d := range destinations { + got[d.block.override] = d.path.String() + } + assert.Equal(t, map[bool]string{ + false: "resources.jobs.j.tasks[0]", + true: "resources.jobs.j.tasks[0]", + }, got) +} + +func TestBlockResolverCountsIndexPerFile(t *testing.T) { + // One target's override spread over two included files. Resource keys are + // unique across top-level files but that is not enforced inside targets, so a + // task second in the concatenated target region can be first in the file that + // defines it -- and the write has to use the latter. + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "overrides"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "databricks.yml"), []byte(`bundle: + name: split + +include: + - overrides/*.yml + +resources: + jobs: + j: + tasks: + - task_key: base +`), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "overrides", "10-first.yml"), []byte(`targets: + dev: + resources: + jobs: + j: + tasks: + - task_key: from_first + max_retries: 1 +`), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "overrides", "20-second.yml"), []byte(`targets: + dev: + resources: + jobs: + j: + tasks: + - task_key: from_second + max_retries: 2 +`), 0o600)) + + ctx := logdiag.InitContext(t.Context()) + b, err := bundle.Load(ctx, dir) + require.NoError(t, err) + mutator.DefaultMutators(ctx, b) + bundle.ApplyContext(ctx, b, mutator.SelectTarget("dev")) + bundle.ApplySeqContext(ctx, b, resourcemutator.MergeJobTasks()) + blocks := newBlockResolver(ctx, b) + require.NotNil(t, blocks) + + resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='from_second'].max_retries", b, OperationReplace) + require.NoError(t, err) + + destination, err := blocks.singleDestination(resolved) + require.NoError(t, err) + assert.Equal(t, "20-second.yml", filepath.Base(destination.block.file)) + assert.Equal(t, "resources.jobs.j.tasks[0].max_retries", destination.path.String()) +} diff --git a/bundle/configsync/rename.go b/bundle/configsync/rename.go new file mode 100644 index 00000000000..a1729e2903d --- /dev/null +++ b/bundle/configsync/rename.go @@ -0,0 +1,212 @@ +package configsync + +import ( + "errors" + "fmt" + "maps" + "reflect" + "slices" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/structs/structpath" +) + +// renamedElement is a keyed sequence element whose key changed remotely. +type renamedElement struct { + // keyField is the field holding the key, e.g. "task_key". + keyField string + oldKey string + newKey string + // addPath is the change path of the add half, kept so it can be skipped. + addPath string +} + +type renameSet struct { + // byRemovePath maps the remove half's change path to the pair. + byRemovePath map[string]renamedElement + // addPaths are the add halves, which must not be routed on their own. + addPaths map[string]struct{} + // unpairedPaths are the halves of a suspected key change on an element that + // is defined in several blocks, where the two halves could not be matched. + // Applying them separately would delete the element from every block and + // recreate it in one, collapsing the split and moving fields into a scope the + // user did not choose, so neither half is applied. + unpairedPaths map[string]struct{} +} + +// pairRenames matches removes of keyed elements against adds in the same sequence +// that carry the same content apart from the key. +// +// A remote key change is reported as an unrelated remove plus add, so without +// pairing the element is deleted and recreated: the recreated copy has to be +// placed somewhere, and for an element defined in several blocks there is no +// single right place. Recognising the pair turns it into a key rewrite, which +// every defining block can apply to its own part. +func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, changes ResourceChanges) renameSet { + set := renameSet{ + byRemovePath: map[string]renamedElement{}, + addPaths: map[string]struct{}{}, + unpairedPaths: map[string]struct{}{}, + } + if blocks == nil { + return set + } + + removes, adds := keyedElementChanges(changes) + for _, remove := range removes { + paired := false + for _, add := range adds { + if _, taken := set.addPaths[add.path]; taken { + continue + } + if remove.parent != add.parent || remove.keyField != add.keyField { + continue + } + if !sameElementApartFromKey(b, resourceKey, remove, add) { + continue + } + set.byRemovePath[remove.path] = renamedElement{ + keyField: remove.keyField, + oldKey: remove.key, + newKey: add.key, + addPath: add.path, + } + set.addPaths[add.path] = struct{}{} + paired = true + break + } + if paired { + continue + } + + // The removal was not matched to an addition. A plain removal is fine: it + // deletes every part of the element, which is what the user asked for. + // But if an unmatched addition to the same sequence is present, the two are + // most likely one key change whose element also had a field edited. Applying + // them separately would delete a split element from every block and recreate + // it in one, collapsing the split and moving fields into a scope the user did + // not choose, so hold both halves back. + var candidates []string + for _, add := range adds { + if remove.parent != add.parent || remove.keyField != add.keyField { + continue + } + if _, taken := set.addPaths[add.path]; !taken { + candidates = append(candidates, add.path) + } + } + if len(candidates) == 0 { + continue + } + if !multiBlockElement(b, blocks, resourceKey, remove.path) { + continue + } + set.unpairedPaths[remove.path] = struct{}{} + for _, path := range candidates { + set.unpairedPaths[path] = struct{}{} + } + } + return set +} + +// multiBlockElement reports whether the element at path is assembled from more +// than one physical block. +func multiBlockElement(b *bundle.Bundle, blocks *blockResolver, resourceKey, path string) bool { + resolved, err := resolveSelectors(resourceKey+"."+path, b, OperationRemove) + if err != nil || len(resolved.steps) == 0 { + return false + } + last := resolved.steps[len(resolved.steps)-1] + return len(blocks.blocksOf(last.element)) > 1 +} + +// keyedElement is one side of a candidate rename. +type keyedElement struct { + path string + parent string + keyField string + key string + value any +} + +// keyedElementChanges splits the changes that address a whole keyed element into +// removes and adds, in a deterministic order. +func keyedElementChanges(changes ResourceChanges) (removes, adds []keyedElement) { + for _, path := range slices.Sorted(maps.Keys(changes)) { + change := changes[path] + if change.Operation != OperationRemove && change.Operation != OperationAdd { + continue + } + node, err := structpath.ParsePath(path) + if err != nil { + continue + } + keyField, key, ok := node.KeyValue() + if !ok { + continue + } + element := keyedElement{ + path: path, + parent: node.Parent().String(), + keyField: keyField, + key: key, + value: change.Value, + } + if change.Operation == OperationRemove { + removes = append(removes, element) + } else { + adds = append(adds, element) + } + } + return removes, adds +} + +// sameElementApartFromKey reports whether the added element is the removed one +// with a different key. The remove half carries no value, so the old element is +// read from the merged configuration. +func sameElementApartFromKey(b *bundle.Bundle, resourceKey string, remove, add keyedElement) bool { + resolved, err := resolveSelectors(resourceKey+"."+remove.path, b, OperationRemove) + if err != nil || !resolved.leaf.IsValid() { + return false + } + oldValue, ok := resolved.leaf.AsAny().(map[string]any) + if !ok { + return false + } + newValue, ok := add.value.(map[string]any) + if !ok { + return false + } + return reflect.DeepEqual(withoutKey(oldValue, remove.keyField), withoutKey(newValue, add.keyField)) +} + +func withoutKey(value map[string]any, keyField string) map[string]any { + out := make(map[string]any, len(value)) + for field, fieldValue := range value { + if field != keyField { + out[field] = fieldValue + } + } + return out +} + +// routeRenameElement locates the renamed element in every block that defines it. +// The caller turns each destination into a rewrite of the key field, so the +// element's other fields stay where they are and a split element keeps its parts +// in their original scopes. +func routeRenameElement(b *bundle.Bundle, blocks *blockResolver, resourceKey, removePath string) ([]routeDestination, error) { + fullPath := resourceKey + "." + removePath + resolved, err := resolveSelectors(fullPath, b, OperationRemove) + if err != nil { + return nil, fmt.Errorf("failed to resolve selectors in path %s: %w", fullPath, err) + } + + destinations, err := blocks.routeDestinations(resolved) + if err != nil { + if errors.Is(err, errAmbiguousBlock) { + return nil, nil + } + return nil, fmt.Errorf("failed to route rename %s: %w", fullPath, err) + } + return destinations, nil +} diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index 01018365e67..929fd85bc92 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -3,6 +3,7 @@ package configsync import ( "cmp" "context" + "errors" "fmt" "io/fs" "maps" @@ -23,23 +24,50 @@ type FieldChange struct { FieldCandidates []string } -// resolveSelectors converts key-value selectors to numeric indices that match -// the YAML file positions. It also returns the location of the resolved leaf value. +// sequenceStep records a sequence element that a change path navigated through. +// The element's index inside a physical block is only known once the block has +// been chosen, so the merged position is kept until then. +type sequenceStep struct { + // position in the pattern built so far, i.e. how many components precede + // this sequence's index. + component int + // path of the sequence relative to a block, e.g. resources.jobs.j.tasks. + sequencePath dyn.Path + element dyn.Value + // newElement marks an Add whose key is not in the merged sequence yet. + newElement bool +} + +// resolvedChange locates a change in the source YAML. +type resolvedChange struct { + // path is relative to the block, with merged indices still in place. + path *structpath.PatternNode + steps []sequenceStep + // leaf is the merged value the change addresses, invalid for a new field. + leaf dyn.Value +} + +// resolveSelectors converts key-value selectors to the indices of the merged +// configuration and records the sequence elements traversed on the way, so the +// caller can map those positions onto the physical block that owns them. // Example: "resources.jobs.foo.tasks[task_key='main'].name" -> "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) { +func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) (resolvedChange, error) { node, err := structpath.ParsePath(pathStr) if err != nil { - return nil, dyn.Location{}, fmt.Errorf("failed to parse path %s: %w", pathStr, err) + return resolvedChange{}, fmt.Errorf("failed to parse path %s: %w", pathStr, err) } nodes := node.AsSlice() var result *structpath.PatternNode + var steps []sequenceStep + var currentPath dyn.Path currentValue := b.Config.Value() - for _, n := range nodes { + for component, n := range nodes { if key, ok := n.StringKey(); ok { result = structpath.NewPatternStringKey(result, key) + currentPath = append(currentPath, dyn.Key(key)) if currentValue.IsValid() { currentValue, _ = dyn.GetByPath(currentValue, dyn.Path{dyn.Key(key)}) } @@ -47,17 +75,26 @@ func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) } if idx, ok := n.Index(); ok { + sequencePath := slices.Clone(currentPath) result = structpath.NewPatternIndex(result, idx) + currentPath = append(currentPath, dyn.Index(idx)) + var element dyn.Value if currentValue.IsValid() { - currentValue, _ = dyn.GetByPath(currentValue, dyn.Path{dyn.Index(idx)}) + element, _ = dyn.GetByPath(currentValue, dyn.Path{dyn.Index(idx)}) + currentValue = element } + steps = append(steps, sequenceStep{ + component: component, + sequencePath: sequencePath, + element: element, + }) continue } // 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 resolvedChange{}, fmt.Errorf("cannot apply [%s='%s'] selector to non-array value in path %s", key, value, pathStr) } seq, _ := currentValue.AsSequence() @@ -75,50 +112,36 @@ func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) } } + sequencePath := slices.Clone(currentPath) + if foundIndex == -1 { if operation == OperationAdd { result = structpath.NewPatternBracketStar(result) + steps = append(steps, sequenceStep{ + component: component, + sequencePath: sequencePath, + newElement: true, + }) // Can't navigate further into non-existent element currentValue = dyn.Value{} continue } - return nil, dyn.Location{}, fmt.Errorf("no array element found with %s='%s' in path %s", key, value, pathStr) + return resolvedChange{}, 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) - result = structpath.NewPatternIndex(result, yamlIndex) + result = structpath.NewPatternIndex(result, foundIndex) + currentPath = append(currentPath, dyn.Index(foundIndex)) + steps = append(steps, sequenceStep{ + component: component, + sequencePath: sequencePath, + element: seq[foundIndex], + }) currentValue = seq[foundIndex] continue } } - return result, currentValue.Location(), nil -} - -// 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 { - matchLocation := seq[sortedIndex].Location() - if matchLocation.File == "" { - return sortedIndex - } - - yamlIndex := 0 - for i, elem := range seq { - if i == sortedIndex { - continue - } - loc := elem.Location() - if loc.File == matchLocation.File && loc.Line < matchLocation.Line { - yamlIndex++ - } - } - return yamlIndex + return resolvedChange{path: result, steps: steps, leaf: currentValue}, nil } func pathDepth(pathStr string) int { @@ -132,7 +155,7 @@ func pathDepth(pathStr string) int { // adjustArrayIndex adjusts the index in a PatternNode based on previous operations. // When operations are applied sequentially, removals and additions shift array indices. // This function adjusts the index to account for those shifts. -func adjustArrayIndex(path *structpath.PatternNode, operations map[string][]struct { +func adjustArrayIndex(path *structpath.PatternNode, scope string, operations map[string][]struct { index int operation OperationType }, @@ -143,7 +166,7 @@ func adjustArrayIndex(path *structpath.PatternNode, operations map[string][]stru } parentPath := path.Parent() - parentPathStr := parentPath.String() + parentPathStr := scope + parentPath.String() ops := operations[parentPathStr] adjustment := 0 @@ -168,6 +191,7 @@ func adjustArrayIndex(path *structpath.PatternNode, operations map[string][]stru func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes) ([]FieldChange, error) { var result []FieldChange targetName := b.Config.Bundle.Target + blocks := newBlockResolver(ctx, b) resourceKeys := slices.Sorted(maps.Keys(configChanges)) @@ -203,6 +227,12 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes return cmp.Compare(a, b) }) + // A key change on a keyed element arrives as a remove of the old key plus an + // add of the new one, with nothing linking them. Pairing them back up lets a + // rename be written as a key rewrite in every block that defines the element, + // which keeps a split element split instead of collapsing it into one scope. + renames := pairRenames(b, blocks, resourceKey, resourceChanges) + // Create indices map for this resource, path -> indices, that we could use to replace with added elements indicesToReplaceMap := make(map[string][]int) @@ -215,86 +245,188 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes configChange := resourceChanges[fieldPath] fullPath := resourceKey + "." + fieldPath - resolvedPath, resolvedLocation, err := resolveSelectors(fullPath, b, configChange.Operation) + // The add half of a rename carries no source location of its own; the + // pair is written from the remove half, which does. + if _, ok := renames.addPaths[fieldPath]; ok { + continue + } + if _, ok := renames.unpairedPaths[fieldPath]; ok { + log.Debugf(ctx, "config-remote-sync: skipping %s: a split element cannot be removed and recreated in one run", fullPath) + continue + } + rename, isRename := renames.byRemovePath[fieldPath] + + resolved, err := resolveSelectors(fullPath, b, configChange.Operation) if err != nil { return nil, fmt.Errorf("failed to resolve selectors in path %s: %w", fullPath, err) } - - // If the element is removed, we can use the index to replace it with added element - // That may improve the diff in cases when the task is recreated because of renaming - if configChange.Operation == OperationRemove { - freeIndex, ok := resolvedPath.Index() - if ok { - parentPath := resolvedPath.Parent().String() - indicesToReplaceMap[parentPath] = append(indicesToReplaceMap[parentPath], freeIndex) + resolvedPath := resolved.path + + // A sequence element addressed by the merged view has to be mapped onto + // the physical block that defines it, because the merged order and the + // per-block order differ once a sequence is split across blocks. An + // element assembled from several blocks has a part in each, so removing + // or renaming it yields one destination per block. + destinations := []routeDestination{{path: resolvedPath}} + routed := false + if blocks != nil && len(resolved.steps) > 0 { + var routeErr error + if isRename { + destinations, routeErr = routeRenameElement(b, blocks, resourceKey, fieldPath) + if routeErr == nil && len(destinations) == 0 { + // The element could not be attributed to a block; leave the + // whole pair for a later run rather than half-applying it. + continue + } + } else { + destinations, routeErr = blocks.routeDestinations(resolved) + } + if routeErr != nil { + if errors.Is(routeErr, errAmbiguousBlock) { + // Applying this change would mean guessing a location. + // Leave it unapplied; a later run can pick it up. + log.Debugf(ctx, "config-remote-sync: skipping %s: %v", fullPath, routeErr) + continue + } + return nil, fmt.Errorf("failed to route change %s: %w", fullPath, routeErr) } + routed = true } - if configChange.Operation == OperationAdd && resolvedPath.BracketStar() { - parentPath := resolvedPath.Parent().String() - indices, ok := indicesToReplaceMap[parentPath] - if ok && len(indices) > 0 { - index := indices[0] - indicesToReplaceMap[parentPath] = indices[1:] - resolvedPath = structpath.NewPatternIndex(resolvedPath.Parent(), index) + for _, destination := range destinations { + block := destination.block + resolvedPath := destination.path + // Each destination gets its own copy: the operation and the value + // are rewritten below per destination, and one block's rewrite must + // not leak into the next. + destChange := &ConfigChangeDesc{ + Operation: configChange.Operation, + Value: configChange.Value, + LocalEdit: configChange.LocalEdit, } - } - resolvedPath = adjustArrayIndex(resolvedPath, indexOperations) + // Index bookkeeping is scoped to a block so that shifts caused by + // operations on one block cannot move indices in another. + scope := "" + if routed { + scope = block.scopeKey() + } - // Track this operation for future index adjustments (only for array element operations) - if originalIndex, ok := resolvedPath.Index(); ok { - parentPath := resolvedPath.Parent().String() - indexOperations[parentPath] = append(indexOperations[parentPath], struct { - index int - operation OperationType - }{originalIndex, configChange.Operation}) - } + // A rename rewrites only the key field of the element, so it is a + // replace at the element's position rather than a change to the + // element itself. The index is still adjusted below, because + // removals earlier in the same block shift it. + if isRename { + resolvedPath = adjustArrayIndex(resolvedPath, scope, indexOperations) + destChange.Operation = OperationReplace + destChange.Value = rename.newKey + resolvedPath = structpath.NewPatternStringKey(resolvedPath, rename.keyField) + candidate := blocks.candidatePath(block, resolvedPath.String()) + result = append(result, FieldChange{ + FilePath: block.file, + Change: destChange, + FieldCandidates: []string{candidate}, + }) + continue + } - resolvedPathStr := resolvedPath.String() - candidates := []string{resolvedPathStr} - if targetName != "" { - targetPrefixedPath := "targets." + targetName + "." + resolvedPathStr - candidates = append(candidates, targetPrefixedPath) - } + // If the element is removed, we can use the index to replace it with added element + // That may improve the diff in cases when the task is recreated because of renaming + if destChange.Operation == OperationRemove { + freeIndex, ok := resolvedPath.Index() + if ok { + parentPath := scope + resolvedPath.Parent().String() + indicesToReplaceMap[parentPath] = append(indicesToReplaceMap[parentPath], freeIndex) + } + } - filePath := resolvedLocation.File + if destChange.Operation == OperationAdd && resolvedPath.BracketStar() { + parentPath := scope + resolvedPath.Parent().String() + indices, ok := indicesToReplaceMap[parentPath] + if ok && len(indices) > 0 { + index := indices[0] + indicesToReplaceMap[parentPath] = indices[1:] + resolvedPath = structpath.NewPatternIndex(resolvedPath.Parent(), index) + } + } - isDefinedInConfig := filePath != "" - if !isDefinedInConfig { - if configChange.Operation == OperationRemove { - // If the field is not defined in the config and the operation is remove, it is more likely a CLI default - // in this case we skip the change - continue + resolvedPath = adjustArrayIndex(resolvedPath, scope, indexOperations) + + // Track this operation for future index adjustments (only for array element operations) + if originalIndex, ok := resolvedPath.Index(); ok { + parentPath := scope + resolvedPath.Parent().String() + indexOperations[parentPath] = append(indexOperations[parentPath], struct { + index int + operation OperationType + }{originalIndex, destChange.Operation}) } - if configChange.Operation == OperationReplace { - // If the field is not defined in the config and the operation is replace, it is more likely a CLI default - // in this case we add it explicitly to the resource location - configChange.Operation = OperationAdd + resolvedPathStr := resolvedPath.String() + var candidates []string + if routed { + // The block is known, so there is exactly one path to write. + candidates = []string{blocks.candidatePath(block, resolvedPathStr)} + } else { + candidates = []string{resolvedPathStr} + if targetName != "" { + targetPrefixedPath := "targets." + targetName + "." + resolvedPathStr + candidates = append(candidates, targetPrefixedPath) + } } - resourceLocation := b.Config.GetLocation(resourceKey) - filePath = resourceLocation.File - if filePath == "" { - return nil, fmt.Errorf("failed to find location for resource %s for a field %s", resourceKey, fieldPath) + // A routed change has a known destination file even when the leaf + // itself is new, but "defined in the config" must still be decided by + // the leaf: a field with no source location is added, not replaced. + filePath := resolved.leaf.Location().File + isDefinedInConfig := filePath != "" + if routed && isDefinedInConfig { + filePath = block.file } - log.Debugf(ctx, "Field %s has no location, using resource location: %s", fullPath, filePath) - } + if !isDefinedInConfig { + if destChange.Operation == OperationRemove { + // If the field is not defined in the config and the operation is remove, it is more likely a CLI default + // in this case we skip the change + continue + } + + if destChange.Operation == OperationReplace { + // If the field is not defined in the config and the operation is replace, it is more likely a CLI default + // in this case we add it explicitly to the resource location. + // The reclassification is also recorded on the shared change so + // the command's output reports what was actually written. + destChange.Operation = OperationAdd + configChange.Operation = OperationAdd + } + + if routed { + // The enclosing element was resolved to a block, so a new field + // on it belongs in that same block. + filePath = block.file + } else { + resourceLocation := b.Config.GetLocation(resourceKey) + filePath = resourceLocation.File + } + if filePath == "" { + return nil, fmt.Errorf("failed to find location for resource %s for a field %s", resourceKey, fieldPath) + } + + log.Debugf(ctx, "Field %s has no location, using %s", fullPath, filePath) + } - if (configChange.Operation == OperationAdd || configChange.Operation == OperationReplace) && b.SyncRootPath != "" { - configChange = &ConfigChangeDesc{ - Operation: configChange.Operation, - Value: translateWorkspacePaths(configChange.Value, b.SyncRootPath, b.SyncRoot, filepath.Dir(filePath)), + if (destChange.Operation == OperationAdd || destChange.Operation == OperationReplace) && b.SyncRootPath != "" { + destChange = &ConfigChangeDesc{ + Operation: destChange.Operation, + Value: translateWorkspacePaths(destChange.Value, b.SyncRootPath, b.SyncRoot, filepath.Dir(filePath)), + } } - } - result = append(result, FieldChange{ - FilePath: filePath, - Change: configChange, - FieldCandidates: candidates, - }) + result = append(result, FieldChange{ + FilePath: filePath, + Change: destChange, + FieldCandidates: candidates, + }) + } } } diff --git a/bundle/configsync/resolve_test.go b/bundle/configsync/resolve_test.go index 9264ad7f5dc..9d85fc51119 100644 --- a/bundle/configsync/resolve_test.go +++ b/bundle/configsync/resolve_test.go @@ -8,7 +8,6 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/logdiag" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -32,9 +31,9 @@ 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()) + assert.Equal(t, "resources.jobs.test_job.name", result.path.String()) } func TestResolveSelectors_NumericIndices(t *testing.T) { @@ -57,13 +56,13 @@ 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()) + assert.Equal(t, "resources.jobs.test_job.tasks[0].task_key", result.path.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()) + assert.Equal(t, "resources.jobs.test_job.tasks[1].task_key", result.path.String()) } func TestResolveSelectors_KeyValueSelector(t *testing.T) { @@ -90,13 +89,13 @@ 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()) + assert.Equal(t, "resources.jobs.test_job.tasks[1].notebook_task.notebook_path", result.path.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()) + assert.Equal(t, "resources.jobs.test_job.tasks[0].notebook_task.notebook_path", result.path.String()) } func TestResolveSelectors_SelectorNotFound(t *testing.T) { @@ -120,7 +119,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 +142,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,9 +173,9 @@ 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()) + assert.Equal(t, "resources.jobs.test_job.tasks[1].libraries[0].pypi.package", result.path.String()) } func TestResolveSelectors_WildcardNotSupported(t *testing.T) { @@ -200,52 +199,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") } - -func TestYamlFileIndex(t *testing.T) { - // Simulate a sequence that was sorted alphabetically by a mutator. - // Original YAML order: notebook_task (line 10), python_wheel_task (line 20), pipeline_task (line 30), extra (line 40) - // Sorted order: extra (line 40), notebook_task (line 10), pipeline_task (line 30), python_wheel_task (line 20) - seq := []dyn.Value{ - dyn.NewValue(nil, []dyn.Location{{File: "a.yml", Line: 40}}), // extra - dyn.NewValue(nil, []dyn.Location{{File: "a.yml", Line: 10}}), // notebook_task - 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 - } - - 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 -} - -func TestYamlFileIndex_MultipleFiles(t *testing.T) { - // Tasks from two different files, sorted alphabetically by mutator. - // File A (lines 10, 20): task_a, task_b - // File B (lines 5, 15): task_c, task_d - // Sorted order: task_a (A:10), task_b (A:20), task_c (B:5), task_d (B:15) - seq := []dyn.Value{ - dyn.NewValue(nil, []dyn.Location{{File: "a.yml", Line: 10}}), // task_a - dyn.NewValue(nil, []dyn.Location{{File: "a.yml", Line: 20}}), // task_b - dyn.NewValue(nil, []dyn.Location{{File: "b.yml", Line: 5}}), // task_c - dyn.NewValue(nil, []dyn.Location{{File: "b.yml", Line: 15}}), // task_d - } - - // 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 -} - -func TestYamlFileIndex_NoLocation(t *testing.T) { - seq := []dyn.Value{ - dyn.NewValue(nil, nil), - dyn.NewValue(nil, nil), - } - assert.Equal(t, 0, yamlFileIndex(seq, 0)) - assert.Equal(t, 1, yamlFileIndex(seq, 1)) -} From c23e7ed0562f17206eaf6191728cb846fddd7534 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 13:33:17 +0000 Subject: [PATCH 02/28] Pick the merge winner by load order when two files define the same field Co-authored-by: Isaac --- .../split/multifile/databricks.yml.tmpl | 6 ++++ .../split/multifile/output.txt | 30 +++++++++++++++++-- .../multifile/overrides/10-first.yml.tmpl | 2 ++ .../multifile/overrides/20-second.yml.tmpl | 2 ++ .../config-remote-sync/split/multifile/script | 22 ++++++++++++++ bundle/configsync/blockindex.go | 23 ++++++++++---- 6 files changed, 77 insertions(+), 8 deletions(-) diff --git a/acceptance/bundle/config-remote-sync/split/multifile/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/split/multifile/databricks.yml.tmpl index 67c8c4cf852..f2d063a7889 100644 --- a/acceptance/bundle/config-remote-sync/split/multifile/databricks.yml.tmpl +++ b/acceptance/bundle/config-remote-sync/split/multifile/databricks.yml.tmpl @@ -10,6 +10,12 @@ bundle: # files within the same target scope: max_retries from the first, timeout_seconds # from the second. Each field still has one definition, so each edit has one # destination. +# +# "same_field_both_files" sets max_retries in BOTH override files. Includes are merged +# in load order, so the later file wins and its value is the one deployed -- note that +# order is not the alphabetical order of the filenames, so the destination cannot be +# chosen by sorting them. Writing the losing copy would leave the effective value +# unchanged and the sync would re-detect the same change on every run. include: - overrides/*.yml diff --git a/acceptance/bundle/config-remote-sync/split/multifile/output.txt b/acceptance/bundle/config-remote-sync/split/multifile/output.txt index 06a956dba30..1bfe1d0c40b 100644 --- a/acceptance/bundle/config-remote-sync/split/multifile/output.txt +++ b/acceptance/bundle/config-remote-sync/split/multifile/output.txt @@ -47,20 +47,46 @@ Resource: resources.jobs.multifile_job >>> diff.py overrides/10-first.yml.backup overrides/10-first.yml --- overrides/10-first.yml.backup +++ overrides/10-first.yml -@@ -10,3 +10,3 @@ +@@ -10,5 +10,5 @@ notebook_path: /Users/{{workspace_user_name}}/first - task_key: in_both_files - max_retries: 1 + max_retries: 9 + - task_key: same_field_both_files + max_retries: 1 >>> diff.py overrides/20-second.yml.backup overrides/20-second.yml --- overrides/20-second.yml.backup +++ overrides/20-second.yml -@@ -10,3 +10,3 @@ +@@ -10,5 +10,5 @@ timeout_seconds: 777 - task_key: in_both_files - timeout_seconds: 30 + timeout_seconds: 900 + - task_key: same_field_both_files + max_retries: 2 + +=== Edit a field that both override files define +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.multifile_job + tasks[task_key='same_field_both_files'].max_retries: replace + + + +=== The winning file is updated; the shadowed copy is left alone + +>>> diff.py overrides/10-first.yml.backup overrides/10-first.yml + +>>> diff.py overrides/20-second.yml.backup overrides/20-second.yml +--- overrides/20-second.yml.backup ++++ overrides/20-second.yml +@@ -12,3 +12,3 @@ + timeout_seconds: 900 + - task_key: same_field_both_files +- max_retries: 2 ++ max_retries: 9 >>> [CLI] bundle destroy --auto-approve -t dev The following resources will be deleted: diff --git a/acceptance/bundle/config-remote-sync/split/multifile/overrides/10-first.yml.tmpl b/acceptance/bundle/config-remote-sync/split/multifile/overrides/10-first.yml.tmpl index dd4438f6a98..5f3fdf1ccc5 100644 --- a/acceptance/bundle/config-remote-sync/split/multifile/overrides/10-first.yml.tmpl +++ b/acceptance/bundle/config-remote-sync/split/multifile/overrides/10-first.yml.tmpl @@ -10,3 +10,5 @@ targets: notebook_path: /Users/{{workspace_user_name}}/first - task_key: in_both_files max_retries: 1 + - task_key: same_field_both_files + max_retries: 1 diff --git a/acceptance/bundle/config-remote-sync/split/multifile/overrides/20-second.yml.tmpl b/acceptance/bundle/config-remote-sync/split/multifile/overrides/20-second.yml.tmpl index 6dffbbfca0e..229f449ebf3 100644 --- a/acceptance/bundle/config-remote-sync/split/multifile/overrides/20-second.yml.tmpl +++ b/acceptance/bundle/config-remote-sync/split/multifile/overrides/20-second.yml.tmpl @@ -9,3 +9,5 @@ targets: notebook_path: /Users/{{workspace_user_name}}/second - task_key: in_both_files timeout_seconds: 30 + - task_key: same_field_both_files + max_retries: 2 diff --git a/acceptance/bundle/config-remote-sync/split/multifile/script b/acceptance/bundle/config-remote-sync/split/multifile/script index f8dc6c4cee1..4f82441d08e 100644 --- a/acceptance/bundle/config-remote-sync/split/multifile/script +++ b/acceptance/bundle/config-remote-sync/split/multifile/script @@ -68,3 +68,25 @@ echo trace diff.py overrides/10-first.yml.backup overrides/10-first.yml trace diff.py overrides/20-second.yml.backup overrides/20-second.yml rm overrides/10-first.yml.backup overrides/20-second.yml.backup + + +# max_retries is set in both override files, so only one of them holds the value that +# was deployed. The edit has to land there; the other copy is shadowed. +title "Edit a field that both override files define" +edit_resource.py jobs $job_id < Date: Mon, 3 Aug 2026 13:33:26 +0000 Subject: [PATCH 03/28] Treat an out-of-range positional index as a new element Co-authored-by: Isaac --- .../split/nested_sequence/databricks.yml.tmpl | 32 ++++++++++++++++ .../split/nested_sequence/out.test.toml | 4 ++ .../split/nested_sequence/output.txt | 38 +++++++++++++++++++ .../split/nested_sequence/script | 33 ++++++++++++++++ bundle/configsync/resolve.go | 4 ++ 5 files changed, 111 insertions(+) create mode 100644 acceptance/bundle/config-remote-sync/split/nested_sequence/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/nested_sequence/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/nested_sequence/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/nested_sequence/script diff --git a/acceptance/bundle/config-remote-sync/split/nested_sequence/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/split/nested_sequence/databricks.yml.tmpl new file mode 100644 index 00000000000..afee7a28aef --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/nested_sequence/databricks.yml.tmpl @@ -0,0 +1,32 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +# libraries is a sequence nested inside tasks, which is itself a sequence. "second" is +# not the first task, so its position differs between the merged list and the physical +# block once the target block contributes a task that sorts ahead of it. Adding a +# library therefore has to translate the enclosing task's index before it can find the +# receiving sequence. +resources: + jobs: + nested_job: + tasks: + - task_key: first + libraries: + - pypi: + package: requests + - task_key: second + libraries: + - pypi: + package: urllib3 + +targets: + dev: + mode: development + resources: + jobs: + nested_job: + tasks: + - task_key: aaa_sorts_first + libraries: + - pypi: + package: certifi diff --git a/acceptance/bundle/config-remote-sync/split/nested_sequence/out.test.toml b/acceptance/bundle/config-remote-sync/split/nested_sequence/out.test.toml new file mode 100644 index 00000000000..4b5914daa2c --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/nested_sequence/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = false +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/split/nested_sequence/output.txt b/acceptance/bundle/config-remote-sync/split/nested_sequence/output.txt new file mode 100644 index 00000000000..271ddaf0223 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/nested_sequence/output.txt @@ -0,0 +1,38 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Add a library to a task that is not first after the merge sort +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.nested_job + tasks[task_key='second'].libraries: replace + + + +=== The new library lands on the task that received it + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -19,4 +19,6 @@ + - pypi: + package: urllib3 ++ - pypi: ++ package: pyyaml + + targets: + +>>> grep -c pyyaml databricks.yml +1 + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.nested_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/nested_sequence/script b/acceptance/bundle/config-remote-sync/split/nested_sequence/script new file mode 100644 index 00000000000..26d26468995 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/nested_sequence/script @@ -0,0 +1,33 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve -t dev +} +trap cleanup EXIT + +$CLI bundle deploy -t dev +job_id="$(read_id.py nested_job)" + + +# Add an element to a sequence nested inside another sequence. The enclosing task sits +# at a different index in the merged list than in the block it is written in, so the +# outer index has to be translated before the receiving sequence can be found. +title "Add a library to a task that is not first after the merge sort" +edit_resource.py jobs $job_id < Date: Mon, 3 Aug 2026 14:00:03 +0000 Subject: [PATCH 04/28] Keep variable references when writing to a target override block Co-authored-by: Isaac --- .../split/target_variable/databricks.yml.tmpl | 22 ++++++ .../split/target_variable/out.test.toml | 4 ++ .../split/target_variable/output.txt | 36 ++++++++++ .../split/target_variable/script | 30 ++++++++ bundle/configsync/resolve.go | 11 ++- bundle/configsync/variables.go | 69 +++++++++---------- 6 files changed, 133 insertions(+), 39 deletions(-) create mode 100644 acceptance/bundle/config-remote-sync/split/target_variable/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/target_variable/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/target_variable/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/target_variable/script diff --git a/acceptance/bundle/config-remote-sync/split/target_variable/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/split/target_variable/databricks.yml.tmpl new file mode 100644 index 00000000000..4bbe54528e1 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/target_variable/databricks.yml.tmpl @@ -0,0 +1,22 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +variables: + root: + default: base + +resources: + jobs: + var_job: + max_concurrent_runs: 1 + +targets: + dev: + mode: development + resources: + jobs: + var_job: + tasks: + - task_key: main + notebook_task: + notebook_path: /Users/${var.root}/nb diff --git a/acceptance/bundle/config-remote-sync/split/target_variable/out.test.toml b/acceptance/bundle/config-remote-sync/split/target_variable/out.test.toml new file mode 100644 index 00000000000..4b5914daa2c --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/target_variable/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = false +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/split/target_variable/output.txt b/acceptance/bundle/config-remote-sync/split/target_variable/output.txt new file mode 100644 index 00000000000..d81480fb0f8 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/target_variable/output.txt @@ -0,0 +1,36 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Rename the notebook inside a target-block field that uses a variable +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.var_job + tasks[task_key='main'].notebook_task.notebook_path: replace + + + +=== The variable reference is preserved + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -20,3 +20,3 @@ + - task_key: main + notebook_task: +- notebook_path: /Users/${var.root}/nb ++ notebook_path: /Users/${var.root}/renamed + +>>> grep -c var.root databricks.yml +1 + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.var_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/target_variable/script b/acceptance/bundle/config-remote-sync/split/target_variable/script new file mode 100644 index 00000000000..a5f19a4065f --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/target_variable/script @@ -0,0 +1,30 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve -t dev +} +trap cleanup EXIT + +$CLI bundle deploy -t dev +job_id="$(read_id.py var_job)" + + +# Edit only the trailing segment of a path whose leading segment came from ${var.root}. +# The reference must survive; hardcoding it would bake the dev value into the source. +title "Rename the notebook inside a target-block field that uses a variable" +edit_resource.py jobs $job_id < prefix that a write to an override block needs. Selecting a + // target folds the override into resources and drops the targets subtree, so a + // prefixed path does not resolve there. + mergedPath string } // sequenceStep records a sequence element that a change path navigated through. @@ -325,11 +330,12 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes destChange.Operation = OperationReplace destChange.Value = rename.newKey resolvedPath = structpath.NewPatternStringKey(resolvedPath, rename.keyField) - candidate := blocks.candidatePath(block, resolvedPath.String()) + mergedPath := resolvedPath.String() result = append(result, FieldChange{ FilePath: block.file, Change: destChange, - FieldCandidates: []string{candidate}, + FieldCandidates: []string{blocks.candidatePath(block, mergedPath)}, + mergedPath: mergedPath, }) continue } @@ -429,6 +435,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes FilePath: filePath, Change: destChange, FieldCandidates: candidates, + mergedPath: resolvedPathStr, }) } } diff --git a/bundle/configsync/variables.go b/bundle/configsync/variables.go index 055a47dc934..f4f5149c011 100644 --- a/bundle/configsync/variables.go +++ b/bundle/configsync/variables.go @@ -94,13 +94,13 @@ func RestoreVariableReferences(ctx context.Context, b *bundle.Bundle, fieldChang var newValue any switch fc.Change.Operation { case OperationReplace: - fieldValue, ok := preResolvedValueAt(preResolved, fc.FieldCandidates) + fieldValue, ok := preResolvedValueAt(preResolved, fc.mergedPath) if !ok { continue } newValue = restoreOriginalRefs(fc.Change.Value, fieldValue, resolved, stats) case OperationAdd: - siblings, ok := sequenceSiblings(preResolved, fc.FieldCandidates) + siblings, ok := sequenceSiblings(preResolved, fc.mergedPath) if !ok { continue } @@ -556,18 +556,16 @@ func parseTemplateSegments(template string, resolved dyn.Value) []templateSegmen // preResolvedValueAt returns the pre-resolved dyn.Value at the field path, // if the field exists in the merged pre-resolved config. -func preResolvedValueAt(preResolved dyn.Value, candidates []string) (dyn.Value, bool) { - for _, candidate := range candidates { - p, err := dyn.NewPathFromString(candidate) - if err != nil { - continue - } - v, err := dyn.GetByPath(preResolved, p) - if err == nil { - return v, true - } +func preResolvedValueAt(preResolved dyn.Value, fieldPath string) (dyn.Value, bool) { + p, err := dyn.NewPathFromString(fieldPath) + if err != nil { + return dyn.InvalidValue, false } - return dyn.InvalidValue, false + v, err := dyn.GetByPath(preResolved, p) + if err != nil { + return dyn.InvalidValue, false + } + return v, true } // sequenceSiblings returns the sibling elements of the parent sequence when @@ -575,29 +573,26 @@ func preResolvedValueAt(preResolved dyn.Value, candidates []string) (dyn.Value, // last component must be an index ([*] or [N]) and the parent must resolve // to a sequence in the pre-resolved config. Returns false for non-sequence // Adds (e.g., new map fields). -func sequenceSiblings(preResolved dyn.Value, candidates []string) ([]dyn.Value, bool) { - for _, candidate := range candidates { - node, err := structpath.ParsePattern(candidate) - if err != nil { - continue - } - _, hasIndex := node.Index() - if !hasIndex && !node.BracketStar() { - continue - } - p, err := dyn.NewPathFromString(node.Parent().String()) - if err != nil { - continue - } - parentValue, err := dyn.GetByPath(preResolved, p) - if err != nil { - continue - } - seq, ok := parentValue.AsSequence() - if !ok { - continue - } - return seq, true +func sequenceSiblings(preResolved dyn.Value, fieldPath string) ([]dyn.Value, bool) { + node, err := structpath.ParsePattern(fieldPath) + if err != nil { + return nil, false + } + _, hasIndex := node.Index() + if !hasIndex && !node.BracketStar() { + return nil, false + } + p, err := dyn.NewPathFromString(node.Parent().String()) + if err != nil { + return nil, false + } + parentValue, err := dyn.GetByPath(preResolved, p) + if err != nil { + return nil, false + } + seq, ok := parentValue.AsSequence() + if !ok { + return nil, false } - return nil, false + return seq, true } From 8c81393b71ab1f8aa4bc5f571e1fed56fe03f0cb Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 14:31:13 +0000 Subject: [PATCH 05/28] Fix three write-back defects around removals and rename pairing Co-authored-by: Isaac --- .../databricks.yml.tmpl | 26 ++++++ .../remove_field_both_blocks/out.test.toml | 4 + .../split/remove_field_both_blocks/output.txt | 41 +++++++++ .../split/remove_field_both_blocks/script | 23 +++++ .../databricks.yml.tmpl | 30 +++++++ .../remove_with_unrelated_add/out.test.toml | 4 + .../remove_with_unrelated_add/output.txt | 53 ++++++++++++ .../split/remove_with_unrelated_add/script | 26 ++++++ .../databricks.yml.tmpl | 28 ++++++ .../rename_ambiguous_pairing/out.test.toml | 4 + .../split/rename_ambiguous_pairing/output.txt | 35 ++++++++ .../split/rename_ambiguous_pairing/script | 28 ++++++ bundle/configsync/blockindex.go | 34 +++++--- bundle/configsync/rename.go | 86 ++++++++++++++----- bundle/configsync/resolve.go | 9 +- 15 files changed, 394 insertions(+), 37 deletions(-) create mode 100644 acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/script create mode 100644 acceptance/bundle/config-remote-sync/split/remove_with_unrelated_add/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/remove_with_unrelated_add/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/remove_with_unrelated_add/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/remove_with_unrelated_add/script create mode 100644 acceptance/bundle/config-remote-sync/split/rename_ambiguous_pairing/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/rename_ambiguous_pairing/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/rename_ambiguous_pairing/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/rename_ambiguous_pairing/script diff --git a/acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/databricks.yml.tmpl new file mode 100644 index 00000000000..7f8de9c8476 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/databricks.yml.tmpl @@ -0,0 +1,26 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +# max_retries is defined in BOTH blocks. An edit only has to reach the definition that +# wins the merge, but a removal has to reach every copy: deleting only the winner lets +# the shadowed copy take effect, so the next deploy restores the value the user just +# removed in the UI. + +resources: + jobs: + shared_field_job: + tasks: + - task_key: shared + max_retries: 1 + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/nb + +targets: + dev: + mode: development + resources: + jobs: + shared_field_job: + tasks: + - task_key: shared + max_retries: 5 diff --git a/acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/out.test.toml b/acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/out.test.toml new file mode 100644 index 00000000000..4b5914daa2c --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = false +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/output.txt b/acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/output.txt new file mode 100644 index 00000000000..9b03d7f9b07 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/output.txt @@ -0,0 +1,41 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Remove max_retries, which both blocks define +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.shared_field_job + tasks[task_key='shared'].max_retries: remove + + + +=== Both copies must be gone, else the next deploy pushes the value back + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -12,5 +12,4 @@ + tasks: + - task_key: shared +- max_retries: 1 + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/nb +@@ -24,3 +23,2 @@ + tasks: + - task_key: shared +- max_retries: 5 + +>>> grep -c max_retries databricks.yml +1 + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.shared_field_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/script b/acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/script new file mode 100644 index 00000000000..876cb123a51 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/remove_field_both_blocks/script @@ -0,0 +1,23 @@ +#!/bin/bash +envsubst < databricks.yml.tmpl > databricks.yml +cleanup() { trace $CLI bundle destroy --auto-approve -t dev; } +trap cleanup EXIT +$CLI bundle deploy -t dev +job_id="$(read_id.py shared_field_job)" + +title "Remove max_retries, which both blocks define" +edit_resource.py jobs $job_id <>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -11,8 +11,8 @@ + unrelated_add_job: + tasks: +- - task_key: shared +- max_retries: 1 ++ - max_retries: 9 + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/nb ++ task_key: brand_new + - task_key: keep + max_retries: 2 +@@ -26,5 +26,3 @@ + jobs: + unrelated_add_job: +- tasks: +- - task_key: shared +- timeout_seconds: 45 ++ tasks: [] + +>>> grep -c task_key: brand_new databricks.yml +1 + +>>> grep -c task_key: keep databricks.yml +1 + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.unrelated_add_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/remove_with_unrelated_add/script b/acceptance/bundle/config-remote-sync/split/remove_with_unrelated_add/script new file mode 100644 index 00000000000..26f73be2078 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/remove_with_unrelated_add/script @@ -0,0 +1,26 @@ +#!/bin/bash +envsubst < databricks.yml.tmpl > databricks.yml +cleanup() { trace $CLI bundle destroy --auto-approve -t dev; } +trap cleanup EXIT +$CLI bundle deploy -t dev +job_id="$(read_id.py unrelated_add_job)" + +# The removal of the two-block element is expected to be held back. brand_new shares +# nothing with it but the sequence, so it has to be applied in the same run. +title "Remove the two-block task AND add an unrelated new task in one run" +edit_resource.py jobs $job_id < zzz and beta (target block) -> aaa, bodies identical +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.ambiguous_rename_job + tasks[task_key='aaa']: add + tasks[task_key='alpha']: remove + tasks[task_key='beta']: remove + tasks[task_key='zzz']: add + + + +=== Neither rename is applied, and both blocks keep their own task + +>>> diff.py databricks.yml.backup databricks.yml + +>>> grep -c task_key: alpha databricks.yml +1 + +>>> grep -c task_key: beta databricks.yml +1 + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.ambiguous_rename_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/rename_ambiguous_pairing/script b/acceptance/bundle/config-remote-sync/split/rename_ambiguous_pairing/script new file mode 100644 index 00000000000..4d4d0bc5972 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/rename_ambiguous_pairing/script @@ -0,0 +1,28 @@ +#!/bin/bash +envsubst < databricks.yml.tmpl > databricks.yml +cleanup() { trace $CLI bundle destroy --auto-approve -t dev; } +trap cleanup EXIT +$CLI bundle deploy -t dev +job_id="$(read_id.py ambiguous_rename_job)" + +# alpha sorts before beta and zzz sorts after aaa, so an order-based pairing produces +# the crossed result: alpha->aaa (top-level) and beta->zzz (target block). +title "alpha (top-level) -> zzz and beta (target block) -> aaa, bodies identical" +edit_resource.py jobs $job_id < 1 { + const reason = "several elements with the same contents were renamed in one run, so the new keys cannot be matched to them" + set.unpairedPaths[remove.path] = reason + for _, add := range matches { + set.unpairedPaths[add.path] = reason + } continue } // The removal was not matched to an addition. A plain removal is fine: it // deletes every part of the element, which is what the user asked for. - // But if an unmatched addition to the same sequence is present, the two are - // most likely one key change whose element also had a field edited. Applying - // them separately would delete a split element from every block and recreate - // it in one, collapsing the split and moving fields into a scope the user did - // not choose, so hold both halves back. + // But an addition of the same element under a new key, with one of its + // fields also edited, cannot be recognised as a rename: applying the halves + // separately would delete a split element from every block and recreate it + // in one, collapsing the split and moving fields into a scope the user did + // not choose. Hold both halves back in that case only. + // + // "The same element with a field edited" means it still has the same fields; + // a genuinely new element is an unrelated addition that has to be applied, + // even when a removal happens to be in the same run. var candidates []string for _, add := range adds { if remove.parent != add.parent || remove.keyField != add.keyField { continue } - if _, taken := set.addPaths[add.path]; !taken { + if _, taken := set.addPaths[add.path]; taken { + continue + } + if sameFieldsApartFromKey(b, resourceKey, remove, add) { candidates = append(candidates, add.path) } } @@ -101,9 +122,10 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch if !multiBlockElement(b, blocks, resourceKey, remove.path) { continue } - set.unpairedPaths[remove.path] = struct{}{} + const reason = "a split element cannot be removed and recreated in one run" + set.unpairedPaths[remove.path] = reason for _, path := range candidates { - set.unpairedPaths[path] = struct{}{} + set.unpairedPaths[path] = reason } } return set @@ -180,6 +202,28 @@ func sameElementApartFromKey(b *bundle.Bundle, resourceKey string, remove, add k return reflect.DeepEqual(withoutKey(oldValue, remove.keyField), withoutKey(newValue, add.keyField)) } +// sameFieldsApartFromKey reports whether the added element has the same fields as +// the removed one, ignoring their values. A rename that also edited a field keeps +// the element's shape; an unrelated new element generally does not. +func sameFieldsApartFromKey(b *bundle.Bundle, resourceKey string, remove, add keyedElement) bool { + resolved, err := resolveSelectors(resourceKey+"."+remove.path, b, OperationRemove) + if err != nil || !resolved.leaf.IsValid() { + return false + } + oldValue, ok := resolved.leaf.AsAny().(map[string]any) + if !ok { + return false + } + newValue, ok := add.value.(map[string]any) + if !ok { + return false + } + return slices.Equal( + slices.Sorted(maps.Keys(withoutKey(oldValue, remove.keyField))), + slices.Sorted(maps.Keys(withoutKey(newValue, add.keyField))), + ) +} + func withoutKey(value map[string]any, keyField string) map[string]any { out := make(map[string]any, len(value)) for field, fieldValue := range value { diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index 66a1263c4dd..3d61b6df2fe 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -50,6 +50,9 @@ type resolvedChange struct { steps []sequenceStep // leaf is the merged value the change addresses, invalid for a new field. leaf dyn.Value + // operation decides how many destinations the change needs: a removal has to + // reach every definition, an edit only the one that wins the merge. + operation OperationType } // resolveSelectors converts key-value selectors to the indices of the merged @@ -150,7 +153,7 @@ func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) } } - return resolvedChange{path: result, steps: steps, leaf: currentValue}, nil + return resolvedChange{path: result, steps: steps, leaf: currentValue, operation: operation}, nil } func pathDepth(pathStr string) int { @@ -259,8 +262,8 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes if _, ok := renames.addPaths[fieldPath]; ok { continue } - if _, ok := renames.unpairedPaths[fieldPath]; ok { - log.Debugf(ctx, "config-remote-sync: skipping %s: a split element cannot be removed and recreated in one run", fullPath) + if reason, ok := renames.unpairedPaths[fieldPath]; ok { + log.Debugf(ctx, "config-remote-sync: skipping %s: %s", fullPath, reason) continue } rename, isRename := renames.byRemovePath[fieldPath] From 8b3e0a80be9cabe7f183d8e177683c1d37030b2b Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 14:41:29 +0000 Subject: [PATCH 06/28] Route a mutator-inserted field on a split element as an addition Co-authored-by: Isaac --- .../databricks.yml.tmpl | 32 +++++++++++++++ .../cli_default_split_element/out.test.toml | 4 ++ .../cli_default_split_element/output.txt | 41 +++++++++++++++++++ .../split/cli_default_split_element/script | 26 ++++++++++++ bundle/configsync/blockindex.go | 16 ++++---- 5 files changed, 110 insertions(+), 9 deletions(-) create mode 100644 acceptance/bundle/config-remote-sync/split/cli_default_split_element/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/cli_default_split_element/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/cli_default_split_element/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/cli_default_split_element/script diff --git a/acceptance/bundle/config-remote-sync/split/cli_default_split_element/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/split/cli_default_split_element/databricks.yml.tmpl new file mode 100644 index 00000000000..e2aae808876 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/cli_default_split_element/databricks.yml.tmpl @@ -0,0 +1,32 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +# cluster_id makes OverrideCompute replace each task's compute with +# existing_cluster_id. It builds that value in Go, so the leaf is present in the +# merged config but carries no source location -- unlike a field absent from the +# config, which has no value either. Routing by location therefore finds nothing, and +# on a task defined in both blocks there is no winning definition to fall back on. The +# field is not declared anywhere, so writing it is an addition. + +resources: + jobs: + cli_default_job: + tasks: + - task_key: shared + max_retries: 1 + new_cluster: + spark_version: 13.3.x-scala2.12 + num_workers: 1 + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/nb + +targets: + dev: + mode: development + cluster_id: 0101-000000-abcd1234 + resources: + jobs: + cli_default_job: + tasks: + - task_key: shared + timeout_seconds: 45 diff --git a/acceptance/bundle/config-remote-sync/split/cli_default_split_element/out.test.toml b/acceptance/bundle/config-remote-sync/split/cli_default_split_element/out.test.toml new file mode 100644 index 00000000000..4b5914daa2c --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/cli_default_split_element/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = false +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/split/cli_default_split_element/output.txt b/acceptance/bundle/config-remote-sync/split/cli_default_split_element/output.txt new file mode 100644 index 00000000000..a5938894b03 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/cli_default_split_element/output.txt @@ -0,0 +1,41 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== existing_cluster_id is injected by OverrideCompute; the YAML has none +>>> grep -c existing_cluster_id databricks.yml +1 + +=== Edit that locationless field remotely on a two-block task +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.cli_default_job + tasks[task_key='shared'].existing_cluster_id: add + + + +=== The field is added to the block declaring the resource + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -20,4 +20,5 @@ + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/nb ++ existing_cluster_id: 0101-000000-remote1 + + targets: + +>>> grep -c existing_cluster_id: 0101-000000-remote1 databricks.yml +1 + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.cli_default_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/cli_default_split_element/script b/acceptance/bundle/config-remote-sync/split/cli_default_split_element/script new file mode 100644 index 00000000000..a691d865e98 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/cli_default_split_element/script @@ -0,0 +1,26 @@ +#!/bin/bash +envsubst < databricks.yml.tmpl > databricks.yml +cleanup() { trace $CLI bundle destroy --auto-approve -t dev; } +trap cleanup EXIT +$CLI bundle deploy -t dev +job_id="$(read_id.py cli_default_job)" + +title "existing_cluster_id is injected by OverrideCompute; the YAML has none" +errcode trace grep -c "existing_cluster_id" databricks.yml + +title "Edit that locationless field remotely on a two-block task" +edit_resource.py jobs $job_id < Date: Mon, 3 Aug 2026 16:19:35 +0000 Subject: [PATCH 07/28] Use if instead of switch for routing; drop unit tests covered by acceptance --- bundle/configsync/blockindex.go | 29 +++--- bundle/configsync/blockindex_test.go | 149 ++------------------------- 2 files changed, 26 insertions(+), 152 deletions(-) diff --git a/bundle/configsync/blockindex.go b/bundle/configsync/blockindex.go index 260d44ef897..481dd7508ba 100644 --- a/bundle/configsync/blockindex.go +++ b/bundle/configsync/blockindex.go @@ -320,20 +320,16 @@ type routeDestination struct { // or renamed in each block that contributes a part, so that a split element keeps // its parts in their original scopes. func (r *blockResolver) routeDestinations(change resolvedChange) ([]routeDestination, error) { - var blocks []sourceBlock - switch { - case addressesWholeElement(change): - element := change.steps[len(change.steps)-1].element - blocks = r.blocksOf(element) - if len(blocks) == 0 { - return nil, fmt.Errorf("%w: no source location for the addressed element", errAmbiguousBlock) - } - case change.operation == OperationRemove && change.leaf.IsValid(): - blocks = r.blocksOf(change.leaf) - if len(blocks) == 0 { - return nil, fmt.Errorf("%w: no source location for the removed field", errAmbiguousBlock) - } - default: + // The value whose definitions have to be reached: the element itself when the + // change addresses one, otherwise the field being removed. + var target dyn.Value + if addressesWholeElement(change) { + target = change.steps[len(change.steps)-1].element + } else if change.operation == OperationRemove { + target = change.leaf + } + + if !target.IsValid() { destination, err := r.singleDestination(change) if err != nil { return nil, err @@ -341,6 +337,11 @@ func (r *blockResolver) routeDestinations(change resolvedChange) ([]routeDestina return []routeDestination{destination}, nil } + blocks := r.blocksOf(target) + if len(blocks) == 0 { + return nil, fmt.Errorf("%w: no source location for the value being changed", errAmbiguousBlock) + } + destinations := make([]routeDestination, 0, len(blocks)) for _, block := range blocks { path, err := r.pathWithinBlock(block, change) diff --git a/bundle/configsync/blockindex_test.go b/bundle/configsync/blockindex_test.go index 7a2df2e420f..9311f20f8db 100644 --- a/bundle/configsync/blockindex_test.go +++ b/bundle/configsync/blockindex_test.go @@ -34,9 +34,13 @@ func loadSplitBundle(t *testing.T, target, content string) (*bundle.Bundle, *blo return b, blocks } -// A task defined in both blocks, where one scalar and one map are contributed by -// each. Merging records a scalar's winning location first but a map's *base* -// location first, so the two kinds disagree about what Locations()[0] means. +// A task defined in both blocks, contributing a map to it from each. +// +// This case cannot be reached from an acceptance test: structdiff decomposes an object +// change into leaf paths, so a change never addresses a map as a whole, and breaking +// the map branch of winningBlock leaves every fixture passing. It still has to be +// right, because a map's locations do not order the way a scalar's do -- mergeMap +// records the base first even when the target contributed the winning keys. const twoBlockTaskBundle = `bundle: name: split @@ -45,12 +49,9 @@ resources: j: tasks: - task_key: shared - max_retries: 1 email_notifications: on_failure: - base@example.com - - task_key: top_only - max_retries: 5 targets: dev: @@ -59,146 +60,18 @@ targets: j: tasks: - task_key: shared - max_retries: 2 email_notifications: no_alert_for_skipped_runs: true - - task_key: target_only - max_retries: 6 ` -func TestBlockResolverRoutesToDefiningBlock(t *testing.T) { +func TestBlockResolverRoutesTwoBlockMapToTarget(t *testing.T) { b, blocks := loadSplitBundle(t, "dev", twoBlockTaskBundle) - tests := []struct { - name string - path string - override bool - want string - }{ - { - name: "field defined only top-level", - path: "resources.jobs.j.tasks[task_key='top_only'].max_retries", - override: false, - want: "resources.jobs.j.tasks[1].max_retries", - }, - { - name: "field defined only in the target", - path: "resources.jobs.j.tasks[task_key='target_only'].max_retries", - override: true, - want: "resources.jobs.j.tasks[1].max_retries", - }, - { - // A scalar in both blocks: the target's value is the deployed one, so - // writing the top-level copy would not change the effective value. - name: "scalar defined in both blocks routes to the target", - path: "resources.jobs.j.tasks[task_key='shared'].max_retries", - override: true, - want: "resources.jobs.j.tasks[0].max_retries", - }, - { - // A leaf under the map still has exactly one definition of its own. - name: "leaf under a two-block map routes to its own block", - path: "resources.jobs.j.tasks[task_key='shared'].email_notifications.on_failure", - override: false, - want: "resources.jobs.j.tasks[0].email_notifications.on_failure", - }, - { - // The map as a whole: mergeMap records the base's location first even - // though the target contributed keys, so the first location says nothing - // about precedence and the narrower scope is used instead. - name: "map defined in both blocks routes to the target", - path: "resources.jobs.j.tasks[task_key='shared'].email_notifications", - override: true, - want: "resources.jobs.j.tasks[0].email_notifications", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - resolved, err := resolveSelectors(tc.path, b, OperationReplace) - require.NoError(t, err) - - destination, err := blocks.singleDestination(resolved) - require.NoError(t, err) - assert.Equal(t, tc.override, destination.block.override) - assert.Equal(t, tc.want, destination.path.String()) - }) - } -} - -func TestBlockResolverRoutesElementToEveryDefiningBlock(t *testing.T) { - b, blocks := loadSplitBundle(t, "dev", twoBlockTaskBundle) - - // Removing or renaming an element defined in both blocks has to reach the part - // in each, and each part has its own index inside its own block. - resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='shared']", b, OperationRemove) - require.NoError(t, err) - - destinations, err := blocks.routeDestinations(resolved) - require.NoError(t, err) - - got := map[bool]string{} - for _, d := range destinations { - got[d.block.override] = d.path.String() - } - assert.Equal(t, map[bool]string{ - false: "resources.jobs.j.tasks[0]", - true: "resources.jobs.j.tasks[0]", - }, got) -} - -func TestBlockResolverCountsIndexPerFile(t *testing.T) { - // One target's override spread over two included files. Resource keys are - // unique across top-level files but that is not enforced inside targets, so a - // task second in the concatenated target region can be first in the file that - // defines it -- and the write has to use the latter. - dir := t.TempDir() - require.NoError(t, os.MkdirAll(filepath.Join(dir, "overrides"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(dir, "databricks.yml"), []byte(`bundle: - name: split - -include: - - overrides/*.yml - -resources: - jobs: - j: - tasks: - - task_key: base -`), 0o600)) - require.NoError(t, os.WriteFile(filepath.Join(dir, "overrides", "10-first.yml"), []byte(`targets: - dev: - resources: - jobs: - j: - tasks: - - task_key: from_first - max_retries: 1 -`), 0o600)) - require.NoError(t, os.WriteFile(filepath.Join(dir, "overrides", "20-second.yml"), []byte(`targets: - dev: - resources: - jobs: - j: - tasks: - - task_key: from_second - max_retries: 2 -`), 0o600)) - - ctx := logdiag.InitContext(t.Context()) - b, err := bundle.Load(ctx, dir) - require.NoError(t, err) - mutator.DefaultMutators(ctx, b) - bundle.ApplyContext(ctx, b, mutator.SelectTarget("dev")) - bundle.ApplySeqContext(ctx, b, resourcemutator.MergeJobTasks()) - blocks := newBlockResolver(ctx, b) - require.NotNil(t, blocks) - - resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='from_second'].max_retries", b, OperationReplace) + resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='shared'].email_notifications", b, OperationReplace) require.NoError(t, err) destination, err := blocks.singleDestination(resolved) require.NoError(t, err) - assert.Equal(t, "20-second.yml", filepath.Base(destination.block.file)) - assert.Equal(t, "resources.jobs.j.tasks[0].max_retries", destination.path.String()) + assert.True(t, destination.block.override) + assert.Equal(t, "resources.jobs.j.tasks[0].email_notifications", destination.path.String()) } From 13a950a33d595f6989fcbba6728424f6ac168792 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 16:31:17 +0000 Subject: [PATCH 08/28] Drop unit test for an unreachable case Co-authored-by: Isaac --- bundle/configsync/blockindex.go | 14 ++--- bundle/configsync/blockindex_test.go | 77 ---------------------------- 2 files changed, 8 insertions(+), 83 deletions(-) delete mode 100644 bundle/configsync/blockindex_test.go diff --git a/bundle/configsync/blockindex.go b/bundle/configsync/blockindex.go index 481dd7508ba..c0b103fc9eb 100644 --- a/bundle/configsync/blockindex.go +++ b/bundle/configsync/blockindex.go @@ -210,10 +210,12 @@ func (r *blockResolver) blocksOf(value dyn.Value) []sourceBlock { // The target override is preferred. For a scalar that is also the block that won // the merge, because mergePrimitive keeps the incoming value and records its // location first, so writing the other copy would leave the effective value -// unchanged. For a map or sequence, merging records the base's location first -// (mergeMap, mergeSequence) even though the target contributed keys, so the first -// location is not a statement about precedence; the target is still the narrower -// scope and the one a remote edit made under that target belongs in. +// unchanged. A sequence is different: mergeSequence records the base's location +// first even when the target contributed entries, so the first location is not a +// statement about precedence. This is reachable, because a sequence with no keyed +// diffing (pipeline clusters) arrives as a change to the whole list when its length +// changes. The target is the narrower scope and the one a remote edit made under +// that target belongs in. func (r *blockResolver) winningBlock(value dyn.Value) (sourceBlock, bool) { // Locations are in merge order, so the first one that maps to a block is the // definition the merged value took. Two blocks in the same scope are only @@ -229,8 +231,8 @@ func (r *blockResolver) winningBlock(value dyn.Value) (sourceBlock, bool) { if !found { first, found = block, true } - // A scalar's winner is already first, but a map or sequence records the base - // first even when the target contributed keys, so an override still wins. + // A scalar's winner is already first, but a sequence records the base first + // even when the target contributed entries, so an override still wins. if block.override { return block, true } diff --git a/bundle/configsync/blockindex_test.go b/bundle/configsync/blockindex_test.go deleted file mode 100644 index 9311f20f8db..00000000000 --- a/bundle/configsync/blockindex_test.go +++ /dev/null @@ -1,77 +0,0 @@ -package configsync - -import ( - "os" - "path/filepath" - "testing" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/mutator" - "github.com/databricks/cli/bundle/config/mutator/resourcemutator" - "github.com/databricks/cli/libs/logdiag" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// loadSplitBundle loads a bundle the way the sync command does, so a target's -// overrides are merged into the resources tree and keyed sequences are merged by -// key. Merging keyed sequences happens in the initialize phase, after the target -// is selected, so it has to be applied explicitly here. -func loadSplitBundle(t *testing.T, target, content string) (*bundle.Bundle, *blockResolver) { - t.Helper() - dir := t.TempDir() - require.NoError(t, os.WriteFile(filepath.Join(dir, "databricks.yml"), []byte(content), 0o600)) - - ctx := logdiag.InitContext(t.Context()) - b, err := bundle.Load(ctx, dir) - require.NoError(t, err) - mutator.DefaultMutators(ctx, b) - bundle.ApplyContext(ctx, b, mutator.SelectTarget(target)) - bundle.ApplySeqContext(ctx, b, resourcemutator.MergeJobTasks()) - - blocks := newBlockResolver(ctx, b) - require.NotNil(t, blocks) - return b, blocks -} - -// A task defined in both blocks, contributing a map to it from each. -// -// This case cannot be reached from an acceptance test: structdiff decomposes an object -// change into leaf paths, so a change never addresses a map as a whole, and breaking -// the map branch of winningBlock leaves every fixture passing. It still has to be -// right, because a map's locations do not order the way a scalar's do -- mergeMap -// records the base first even when the target contributed the winning keys. -const twoBlockTaskBundle = `bundle: - name: split - -resources: - jobs: - j: - tasks: - - task_key: shared - email_notifications: - on_failure: - - base@example.com - -targets: - dev: - resources: - jobs: - j: - tasks: - - task_key: shared - email_notifications: - no_alert_for_skipped_runs: true -` - -func TestBlockResolverRoutesTwoBlockMapToTarget(t *testing.T) { - b, blocks := loadSplitBundle(t, "dev", twoBlockTaskBundle) - - resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='shared'].email_notifications", b, OperationReplace) - require.NoError(t, err) - - destination, err := blocks.singleDestination(resolved) - require.NoError(t, err) - assert.True(t, destination.block.override) - assert.Equal(t, "resources.jobs.j.tasks[0].email_notifications", destination.path.String()) -} From 6865e8dbfd97666df861fd3689fbb57f048097c0 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 17:19:36 +0000 Subject: [PATCH 09/28] Drop two unreachable branches in block routing Co-authored-by: Isaac --- bundle/configsync/blockindex.go | 53 ++++++++++----------------------- 1 file changed, 15 insertions(+), 38 deletions(-) diff --git a/bundle/configsync/blockindex.go b/bundle/configsync/blockindex.go index c0b103fc9eb..20908379ad6 100644 --- a/bundle/configsync/blockindex.go +++ b/bundle/configsync/blockindex.go @@ -204,44 +204,30 @@ func (r *blockResolver) blocksOf(value dyn.Value) []sourceBlock { return blocks } -// winningBlock returns the block a change to value has to be written to when +// winningBlock returns the block whose definition the merged value took, when // several blocks define it. // -// The target override is preferred. For a scalar that is also the block that won -// the merge, because mergePrimitive keeps the incoming value and records its -// location first, so writing the other copy would leave the effective value -// unchanged. A sequence is different: mergeSequence records the base's location -// first even when the target contributed entries, so the first location is not a -// statement about precedence. This is reachable, because a sequence with no keyed -// diffing (pipeline clusters) arrives as a change to the whole list when its length -// changes. The target is the narrower scope and the one a remote edit made under -// that target belongs in. +// Locations accumulate in merge order, so the first one that maps to a block is the +// winner: mergePrimitive records the incoming (overriding) value's location first, +// and writing any other copy would leave the effective value unchanged. Load order +// is the only thing that distinguishes two blocks in the same scope, and no property +// of the blocks themselves reproduces it. func (r *blockResolver) winningBlock(value dyn.Value) (sourceBlock, bool) { - // Locations are in merge order, so the first one that maps to a block is the - // definition the merged value took. Two blocks in the same scope are only - // distinguishable this way: they are ordered by load order, which no property of - // the blocks themselves reproduces. - var first sourceBlock - found := false for _, location := range value.Locations() { - block, ok := r.byLocation[location] - if !ok { - continue - } - if !found { - first, found = block, true - } - // A scalar's winner is already first, but a sequence records the base first - // even when the target contributed entries, so an override still wins. - if block.override { + if block, ok := r.byLocation[location]; ok { return block, true } } - return first, found + return sourceBlock{}, false } // indexWithinBlock returns the position of element in the sequence that block -// writes at sequencePath, where sequencePath is relative to the block. +// writes at sequencePath, where sequencePath is relative to the block. That position +// differs from the element's position in the merged sequence, which concatenates +// every block's entries and sorts keyed ones by key. +// +// A block is one parsed file, so the sequence read here holds only that file's +// entries and a plain index into it addresses the right entry. func (r *blockResolver) indexWithinBlock(block sourceBlock, sequencePath dyn.Path, element dyn.Value) (int, bool) { parsed, ok := r.blocks[block] if !ok { @@ -261,21 +247,12 @@ func (r *blockResolver) indexWithinBlock(block sourceBlock, sequencePath dyn.Pat locations[location] = struct{}{} } - // A region's sequence is the concatenation of the entries contributed by each - // included file, but the write targets one file, so the index has to be - // counted among that file's entries only. - local := 0 - for _, entry := range entries { - entryFile := entry.Location().File - if entryFile != block.file { - continue - } + for local, entry := range entries { for _, location := range entry.Locations() { if _, ok := locations[location]; ok { return local, true } } - local++ } return 0, false } From 6f09c49f64f95d66b2f722d494f576a36c51031f Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 17:01:09 +0000 Subject: [PATCH 10/28] Simplify rename pairing and index scoping --- bundle/configsync/blockindex.go | 6 ++-- bundle/configsync/rename.go | 57 +++++++++++++++------------------ bundle/configsync/resolve.go | 15 ++++++--- 3 files changed, 38 insertions(+), 40 deletions(-) diff --git a/bundle/configsync/blockindex.go b/bundle/configsync/blockindex.go index 20908379ad6..02247bbb3a9 100644 --- a/bundle/configsync/blockindex.go +++ b/bundle/configsync/blockindex.go @@ -446,10 +446,8 @@ func (r *blockResolver) blocksDefiningSequence(sequencePath dyn.Path) []sourceBl if err != nil { continue } - for _, location := range sequence.Locations() { - if location.File == block.file && !slices.Contains(blocks, block) { - blocks = append(blocks, block) - } + if slices.ContainsFunc(sequence.Locations(), func(l dyn.Location) bool { return l.File == block.file }) { + blocks = append(blocks, block) } } return blocks diff --git a/bundle/configsync/rename.go b/bundle/configsync/rename.go index bda769b010b..3231ed17a40 100644 --- a/bundle/configsync/rename.go +++ b/bundle/configsync/rename.go @@ -54,6 +54,19 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch removes, adds := keyedElementChanges(changes) for _, remove := range removes { + // The remove half carries no value, so the old element is read from the + // merged configuration. Resolving it once also gives the sequence steps that + // multiBlockElement needs below. + resolved, err := resolveSelectors(resourceKey+"."+remove.path, b, OperationRemove) + if err != nil || !resolved.leaf.IsValid() { + continue + } + element, ok := resolved.leaf.AsAny().(map[string]any) + if !ok { + continue + } + oldFields := withoutKey(element, remove.keyField) + // Every add the removed element could equally well have become. Two // identically-bodied elements renamed in one run produce two removes that // each match both adds, and the pairing decides which key goes to which @@ -69,7 +82,7 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch if remove.parent != add.parent || remove.keyField != add.keyField { continue } - if sameElementApartFromKey(b, resourceKey, remove, add) { + if sameElementApartFromKey(oldFields, add) { matches = append(matches, add) } } @@ -112,14 +125,14 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch if _, taken := set.addPaths[add.path]; taken { continue } - if sameFieldsApartFromKey(b, resourceKey, remove, add) { + if sameFieldsApartFromKey(oldFields, add) { candidates = append(candidates, add.path) } } if len(candidates) == 0 { continue } - if !multiBlockElement(b, blocks, resourceKey, remove.path) { + if !multiBlockElement(blocks, resolved) { continue } const reason = "a split element cannot be removed and recreated in one run" @@ -131,14 +144,13 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch return set } -// multiBlockElement reports whether the element at path is assembled from more -// than one physical block. -func multiBlockElement(b *bundle.Bundle, blocks *blockResolver, resourceKey, path string) bool { - resolved, err := resolveSelectors(resourceKey+"."+path, b, OperationRemove) - if err != nil || len(resolved.steps) == 0 { +// multiBlockElement reports whether the element the change addresses is assembled +// from more than one physical block. +func multiBlockElement(blocks *blockResolver, change resolvedChange) bool { + if len(change.steps) == 0 { return false } - last := resolved.steps[len(resolved.steps)-1] + last := change.steps[len(change.steps)-1] return len(blocks.blocksOf(last.element)) > 1 } @@ -184,42 +196,25 @@ func keyedElementChanges(changes ResourceChanges) (removes, adds []keyedElement) } // sameElementApartFromKey reports whether the added element is the removed one -// with a different key. The remove half carries no value, so the old element is -// read from the merged configuration. -func sameElementApartFromKey(b *bundle.Bundle, resourceKey string, remove, add keyedElement) bool { - resolved, err := resolveSelectors(resourceKey+"."+remove.path, b, OperationRemove) - if err != nil || !resolved.leaf.IsValid() { - return false - } - oldValue, ok := resolved.leaf.AsAny().(map[string]any) - if !ok { - return false - } +// with a different key. oldFields is the removed element without its key field. +func sameElementApartFromKey(oldFields map[string]any, add keyedElement) bool { newValue, ok := add.value.(map[string]any) if !ok { return false } - return reflect.DeepEqual(withoutKey(oldValue, remove.keyField), withoutKey(newValue, add.keyField)) + return reflect.DeepEqual(oldFields, withoutKey(newValue, add.keyField)) } // sameFieldsApartFromKey reports whether the added element has the same fields as // the removed one, ignoring their values. A rename that also edited a field keeps // the element's shape; an unrelated new element generally does not. -func sameFieldsApartFromKey(b *bundle.Bundle, resourceKey string, remove, add keyedElement) bool { - resolved, err := resolveSelectors(resourceKey+"."+remove.path, b, OperationRemove) - if err != nil || !resolved.leaf.IsValid() { - return false - } - oldValue, ok := resolved.leaf.AsAny().(map[string]any) - if !ok { - return false - } +func sameFieldsApartFromKey(oldFields map[string]any, add keyedElement) bool { newValue, ok := add.value.(map[string]any) if !ok { return false } return slices.Equal( - slices.Sorted(maps.Keys(withoutKey(oldValue, remove.keyField))), + slices.Sorted(maps.Keys(oldFields)), slices.Sorted(maps.Keys(withoutKey(newValue, add.keyField))), ) } diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index 3d61b6df2fe..73bb3cc5ed6 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -164,6 +164,12 @@ func pathDepth(pathStr string) int { return len(node.AsSlice()) } +// scopedParent keys index bookkeeping by the sequence a path addresses, within the +// scope of one block, so operations on one block cannot shift indices in another. +func scopedParent(scope string, path *structpath.PatternNode) string { + return scope + path.Parent().String() +} + // adjustArrayIndex adjusts the index in a PatternNode based on previous operations. // When operations are applied sequentially, removals and additions shift array indices. // This function adjusts the index to account for those shifts. @@ -178,8 +184,7 @@ func adjustArrayIndex(path *structpath.PatternNode, scope string, operations map } parentPath := path.Parent() - parentPathStr := scope + parentPath.String() - ops := operations[parentPathStr] + ops := operations[scopedParent(scope, path)] adjustment := 0 for _, op := range ops { @@ -348,13 +353,13 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes if destChange.Operation == OperationRemove { freeIndex, ok := resolvedPath.Index() if ok { - parentPath := scope + resolvedPath.Parent().String() + parentPath := scopedParent(scope, resolvedPath) indicesToReplaceMap[parentPath] = append(indicesToReplaceMap[parentPath], freeIndex) } } if destChange.Operation == OperationAdd && resolvedPath.BracketStar() { - parentPath := scope + resolvedPath.Parent().String() + parentPath := scopedParent(scope, resolvedPath) indices, ok := indicesToReplaceMap[parentPath] if ok && len(indices) > 0 { index := indices[0] @@ -367,7 +372,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes // Track this operation for future index adjustments (only for array element operations) if originalIndex, ok := resolvedPath.Index(); ok { - parentPath := scope + resolvedPath.Parent().String() + parentPath := scopedParent(scope, resolvedPath) indexOperations[parentPath] = append(indexOperations[parentPath], struct { index int operation OperationType From 4c00ff700b797ca12d55d0ec63c7fea23f3b8853 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 19:17:34 +0000 Subject: [PATCH 11/28] Require a forced pairing from both sides, and only refuse ambiguity across blocks Co-authored-by: Isaac --- .../databricks.yml.tmpl | 30 ++++ .../out.test.toml | 4 + .../rename_ambiguous_single_block/output.txt | 85 +++++++++ .../rename_ambiguous_single_block/script | 40 +++++ .../databricks.yml.tmpl | 29 ++++ .../rename_two_removes_one_add/out.test.toml | 4 + .../rename_two_removes_one_add/output.txt | 35 ++++ .../split/rename_two_removes_one_add/script | 38 ++++ bundle/configsync/rename.go | 164 +++++++++++++----- 9 files changed, 387 insertions(+), 42 deletions(-) create mode 100644 acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/script create mode 100644 acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/script diff --git a/acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/databricks.yml.tmpl new file mode 100644 index 00000000000..be5e4dd17b6 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/databricks.yml.tmpl @@ -0,0 +1,30 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +# A depends_on entry holds nothing but a task_key, so stripping the key leaves an +# empty body and every removal matches every addition by content. Renaming two tasks +# rewrites gamma's two depends_on entries, and that pairing is unforced. +# +# Holding those back would be wrong: all of these elements are in one block, where +# every pairing writes the same keys to the same place. Refusing them applies the task +# renames while leaving depends_on pointing at keys that no longer exist, which breaks +# the next deploy. Ambiguity only has to be refused when it crosses blocks. + +resources: + jobs: + ambiguous_single_block_job: + tasks: + - task_key: alpha + max_retries: 1 + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/alpha + - task_key: beta + max_retries: 1 + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/beta + - task_key: gamma + depends_on: + - task_key: alpha + - task_key: beta + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/gamma diff --git a/acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/out.test.toml b/acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/out.test.toml new file mode 100644 index 00000000000..4b5914daa2c --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = false +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/output.txt b/acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/output.txt new file mode 100644 index 00000000000..e67f1d1330c --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/output.txt @@ -0,0 +1,85 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Rename alpha -> zzz and beta -> aaa, and point gamma at the new keys +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.ambiguous_single_block_job + tasks[task_key='aaa']: add + tasks[task_key='alpha']: remove + tasks[task_key='beta']: remove + tasks[task_key='gamma'].depends_on[task_key='aaa']: add + tasks[task_key='gamma'].depends_on[task_key='alpha']: remove + tasks[task_key='gamma'].depends_on[task_key='beta']: remove + tasks[task_key='gamma'].depends_on[task_key='zzz']: add + tasks[task_key='zzz']: add + + + +=== Both renames apply and depends_on follows them + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -15,9 +15,9 @@ + ambiguous_single_block_job: + tasks: +- - task_key: alpha ++ - task_key: zzz + max_retries: 1 + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/alpha +- - task_key: beta ++ - task_key: aaa + max_retries: 1 + notebook_task: +@@ -25,6 +25,6 @@ + - task_key: gamma + depends_on: +- - task_key: alpha +- - task_key: beta ++ - task_key: zzz ++ - {task_key: aaa} + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/gamma + +=== Checking for old task keys (should be gone if renames applied): + +>>> grep -c task_key: alpha databricks.yml +0 + +Exit code: 1 + +>>> grep -c task_key: beta databricks.yml +0 + +Exit code: 1 + +=== Checking for new task keys: + +>>> grep -c task_key: zzz databricks.yml +2 + +>>> grep -c task_key: aaa databricks.yml +2 + +=== Checking depends_on (gamma's dependencies): + +>>> grep -A5 task_key: gamma databricks.yml + depends_on: + - task_key: zzz + - {task_key: aaa} + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/gamma + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.jobs.ambiguous_single_block_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/split/rename_ambiguous_single_block/script b/acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/script new file mode 100644 index 00000000000..ffbbfc09eab --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/rename_ambiguous_single_block/script @@ -0,0 +1,40 @@ +#!/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 ambiguous_single_block_job)" + +title "Rename alpha -> zzz and beta -> aaa, and point gamma at the new keys" +edit_resource.py jobs $job_id <>> diff.py databricks.yml.backup databricks.yml + + +=== Checking task locations in YAML:=== Top-level block (should have task_renamed): + +>>> grep -A2 resources: databricks.yml + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.two_removes_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! + +Exit code: 1 diff --git a/acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/script b/acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/script new file mode 100644 index 00000000000..f24998bf43a --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/script @@ -0,0 +1,38 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml +cleanup() { trace $CLI bundle destroy --auto-approve -t dev; } +trap cleanup EXIT +$CLI bundle deploy -t dev +job_id="$(read_id.py two_removes_job)" + +title "Two identical tasks, one in each block; DELETE one (remote), RENAME the other" +edit_resource.py jobs $job_id < task_renamed +new_tasks = [] +for task in r["tasks"]: + if task["task_key"] == "task_target": + # Delete this one (remove only, no add) + pass + elif task["task_key"] == "task_toplevel": + # Rename this one + task["task_key"] = "task_renamed" + new_tasks.append(task) +r["tasks"] = new_tasks +PYEDIT + +title "Sync" +echo +cp databricks.yml databricks.yml.backup +errcode $CLI bundle config-remote-sync -t dev --save + +title "Neither block is rewritten: the pairing is not forced" +echo +trace diff.py databricks.yml.backup databricks.yml +echo +title "Checking task locations in YAML:" +echo "=== Top-level block (should have task_renamed):" +errcode trace grep -A2 "resources:" databricks.yml | grep -A10 "find_a_job:" +echo "=== Target block (should have nothing, task_target removed):" +errcode trace grep -A20 "targets:" databricks.yml | grep -A5 "dev:" +rm databricks.yml.backup diff --git a/bundle/configsync/rename.go b/bundle/configsync/rename.go index 3231ed17a40..64b63f2f4f6 100644 --- a/bundle/configsync/rename.go +++ b/bundle/configsync/rename.go @@ -53,10 +53,12 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch } removes, adds := keyedElementChanges(changes) + + // The remove half carries no value, so each removed element is read from the + // merged configuration once: its fields decide which additions it could have + // become, and its source blocks decide whether a choice between them is safe. + candidates := make([]renameCandidate, 0, len(removes)) for _, remove := range removes { - // The remove half carries no value, so the old element is read from the - // merged configuration. Resolving it once also gives the sequence steps that - // multiBlockElement needs below. resolved, err := resolveSelectors(resourceKey+"."+remove.path, b, OperationRemove) if err != nil || !resolved.leaf.IsValid() { continue @@ -65,85 +67,163 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch if !ok { continue } - oldFields := withoutKey(element, remove.keyField) + candidates = append(candidates, renameCandidate{ + remove: remove, + resolved: resolved, + oldFields: withoutKey(element, remove.keyField), + }) + } - // Every add the removed element could equally well have become. Two - // identically-bodied elements renamed in one run produce two removes that - // each match both adds, and the pairing decides which key goes to which - // element: getting it backwards moves a key into the wrong block, and - // anything referring to the element by key (depends_on) then points at the - // wrong one. Nothing in the change set says which pairing was intended, so - // hold every half back rather than pick one. - var matches []keyedElement + // Which additions each removal could equally well have become. Matching is by + // content, so it is symmetric: an addition can be a match for several removals + // just as a removal can match several additions. + matches := make([][]keyedElement, len(candidates)) + for i, candidate := range candidates { for _, add := range adds { - if _, taken := set.addPaths[add.path]; taken { - continue - } - if remove.parent != add.parent || remove.keyField != add.keyField { + if candidate.remove.parent != add.parent || candidate.remove.keyField != add.keyField { continue } - if sameElementApartFromKey(oldFields, add) { - matches = append(matches, add) + if sameElementApartFromKey(candidate.oldFields, add) { + matches[i] = append(matches[i], add) } } - if len(matches) == 1 { - add := matches[0] - set.byRemovePath[remove.path] = renamedElement{ - keyField: remove.keyField, - oldKey: remove.key, + } + + for i, candidate := range candidates { + // A pair is only unambiguous when the choice is forced from both sides. One + // removal matching several additions is the obvious case, but so is several + // removals matching the same addition: pairing the first and deleting the + // rest would move a key into a block the user did not choose. + if len(matches[i]) == 1 && countMatchesOf(matches, matches[i][0].path) == 1 { + add := matches[i][0] + set.byRemovePath[candidate.remove.path] = renamedElement{ + keyField: candidate.remove.keyField, + oldKey: candidate.remove.key, newKey: add.key, addPath: add.path, } set.addPaths[add.path] = struct{}{} continue } - if len(matches) > 1 { - const reason = "several elements with the same contents were renamed in one run, so the new keys cannot be matched to them" - set.unpairedPaths[remove.path] = reason - for _, add := range matches { + + if len(matches[i]) > 0 { + // The pairing is not forced. Which pairing was intended only matters when + // the elements live in different blocks, because then it decides which + // block each new key is written to. Within one block every pairing writes + // the same set of elements to the same place, so the halves can be applied + // as a plain removal and addition instead of being held back -- otherwise a + // rename of two same-bodied elements is dropped, and anything referring to + // them by key (depends_on) keeps pointing at keys that no longer exist. + if !ambiguityCrossesBlocks(blocks, candidates, matches, i) { + continue + } + const reason = "several elements with the same contents were renamed in one run and they are not in the same block, so the new keys cannot be matched to them" + set.unpairedPaths[candidate.remove.path] = reason + for _, add := range matches[i] { set.unpairedPaths[add.path] = reason } continue } - // The removal was not matched to an addition. A plain removal is fine: it - // deletes every part of the element, which is what the user asked for. - // But an addition of the same element under a new key, with one of its - // fields also edited, cannot be recognised as a rename: applying the halves - // separately would delete a split element from every block and recreate it - // in one, collapsing the split and moving fields into a scope the user did - // not choose. Hold both halves back in that case only. + // The removal matched no addition. A plain removal is fine: it deletes every + // part of the element, which is what the user asked for. But an addition of + // the same element under a new key, with one of its fields also edited, cannot + // be recognised as a rename: applying the halves separately would delete a + // split element from every block and recreate it in one, collapsing the split + // and moving fields into a scope the user did not choose. Hold both halves + // back in that case only. // // "The same element with a field edited" means it still has the same fields; // a genuinely new element is an unrelated addition that has to be applied, // even when a removal happens to be in the same run. - var candidates []string + var edited []string for _, add := range adds { - if remove.parent != add.parent || remove.keyField != add.keyField { + if candidate.remove.parent != add.parent || candidate.remove.keyField != add.keyField { continue } if _, taken := set.addPaths[add.path]; taken { continue } - if sameFieldsApartFromKey(oldFields, add) { - candidates = append(candidates, add.path) + if sameFieldsApartFromKey(candidate.oldFields, add) { + edited = append(edited, add.path) } } - if len(candidates) == 0 { + if len(edited) == 0 { continue } - if !multiBlockElement(blocks, resolved) { + if !multiBlockElement(blocks, candidate.resolved) { continue } const reason = "a split element cannot be removed and recreated in one run" - set.unpairedPaths[remove.path] = reason - for _, path := range candidates { + set.unpairedPaths[candidate.remove.path] = reason + for _, path := range edited { set.unpairedPaths[path] = reason } } return set } +// renameCandidate is a removal of a keyed element, resolved once so its fields and +// source blocks can be consulted without walking the tree again. +type renameCandidate struct { + remove keyedElement + resolved resolvedChange + // oldFields is the removed element without its key field, i.e. what an addition + // has to look like to be the same element under a new key. + oldFields map[string]any +} + +// countMatchesOf returns how many removals could have become the addition at +// addPath. +func countMatchesOf(matches [][]keyedElement, addPath string) int { + count := 0 + for _, candidateMatches := range matches { + for _, add := range candidateMatches { + if add.path == addPath { + count++ + break + } + } + } + return count +} + +// ambiguityCrossesBlocks reports whether an unforced pairing for candidates[i] +// would have to choose between blocks. Every removal that could have become one of +// the same additions is considered, because the choice is between them. +func ambiguityCrossesBlocks(blocks *blockResolver, candidates []renameCandidate, matches [][]keyedElement, i int) bool { + var seen []sourceBlock + for j, candidate := range candidates { + if j != i && !sharesAnyAdd(matches[i], matches[j]) { + continue + } + if len(candidate.resolved.steps) == 0 { + return true + } + last := candidate.resolved.steps[len(candidate.resolved.steps)-1] + elementBlocks := blocks.blocksOf(last.element) + if len(elementBlocks) != 1 { + return true + } + if !slices.Contains(seen, elementBlocks[0]) { + seen = append(seen, elementBlocks[0]) + } + } + return len(seen) != 1 +} + +// sharesAnyAdd reports whether two removals could have become the same addition. +func sharesAnyAdd(a, b []keyedElement) bool { + for _, x := range a { + for _, y := range b { + if x.path == y.path { + return true + } + } + } + return false +} + // multiBlockElement reports whether the element the change addresses is assembled // from more than one physical block. func multiBlockElement(blocks *blockResolver, change resolvedChange) bool { From 1544e89c2c64b112d0b6509e28980da4149fb4c0 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 14:45:59 +0000 Subject: [PATCH 12/28] Quote the target name when addressing an override block Co-authored-by: Isaac --- .../split/dotted_target/databricks.yml.tmpl | 24 ++++++++++++++ .../split/dotted_target/out.test.toml | 4 +++ .../split/dotted_target/output.txt | 33 +++++++++++++++++++ .../split/dotted_target/script | 22 +++++++++++++ bundle/configsync/blockindex.go | 27 +++++++++++++-- bundle/configsync/resolve.go | 7 ++-- 6 files changed, 110 insertions(+), 7 deletions(-) create mode 100644 acceptance/bundle/config-remote-sync/split/dotted_target/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/dotted_target/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/dotted_target/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/dotted_target/script diff --git a/acceptance/bundle/config-remote-sync/split/dotted_target/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/split/dotted_target/databricks.yml.tmpl new file mode 100644 index 00000000000..244e7a93eaa --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/dotted_target/databricks.yml.tmpl @@ -0,0 +1,24 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +# A target name may contain a dot. The write path to an override block is +# targets.., which is parsed back into segments, so a dotted name has to +# be quoted or "dev.eu" becomes two keys and addresses a node that does not exist. +resources: + jobs: + dotted_job: + tasks: + - task_key: main + max_retries: 1 + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/nb + +targets: + dev.eu: + mode: development + resources: + jobs: + dotted_job: + tasks: + - task_key: main + timeout_seconds: 45 diff --git a/acceptance/bundle/config-remote-sync/split/dotted_target/out.test.toml b/acceptance/bundle/config-remote-sync/split/dotted_target/out.test.toml new file mode 100644 index 00000000000..4b5914daa2c --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/dotted_target/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = false +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/split/dotted_target/output.txt b/acceptance/bundle/config-remote-sync/split/dotted_target/output.txt new file mode 100644 index 00000000000..a19a7f4344b --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/dotted_target/output.txt @@ -0,0 +1,33 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev.eu/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Edit a field in the override block of a target whose name contains a dot +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.dotted_job + tasks[task_key='main'].timeout_seconds: replace + + + +=== The override block is updated + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -22,3 +22,3 @@ + tasks: + - task_key: main +- timeout_seconds: 45 ++ timeout_seconds: 900 + +>>> [CLI] bundle destroy --auto-approve -t dev.eu +The following resources will be deleted: + delete resources.jobs.dotted_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev.eu + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/dotted_target/script b/acceptance/bundle/config-remote-sync/split/dotted_target/script new file mode 100644 index 00000000000..c3d05534ab2 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/dotted_target/script @@ -0,0 +1,22 @@ +#!/bin/bash +envsubst < databricks.yml.tmpl > databricks.yml +cleanup() { trace $CLI bundle destroy --auto-approve -t dev.eu; } +trap cleanup EXIT +$CLI bundle deploy -t dev.eu +job_id="$(read_id.py -t dev.eu dotted_job)" + +title "Edit a field in the override block of a target whose name contains a dot" +edit_resource.py jobs $job_id < prefix. -func (r *blockResolver) candidatePath(block sourceBlock, path string) string { +// +// The prefix is built as path nodes rather than concatenated as text: a target name +// may contain a dot ("dev.eu" is a legal name), and the result is parsed back into +// segments, so text would split one name into two keys and address the wrong node. +func (r *blockResolver) candidatePath(block sourceBlock, path *structpath.PatternNode) string { if !block.override { - return path + return path.String() + } + return targetPrefixedPath(r.target, path) +} + +// targetPrefixedPath prefixes path with targets., quoting the target name +// where necessary so the result parses back into the same segments. +func targetPrefixedPath(target string, path *structpath.PatternNode) string { + prefixed := structpath.NewPatternStringKey(nil, "targets") + prefixed = structpath.NewPatternStringKey(prefixed, target) + for _, node := range path.AsSlice() { + if key, ok := node.StringKey(); ok { + prefixed = structpath.NewPatternStringKey(prefixed, key) + } else if index, ok := node.Index(); ok { + prefixed = structpath.NewPatternIndex(prefixed, index) + } else { + prefixed = structpath.NewPatternBracketStar(prefixed) + } } - return "targets." + r.target + "." + path + return prefixed.String() } // sortedBlocks lists the known blocks with the top-level ones first and a total diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index 73bb3cc5ed6..128b9b2928f 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -342,7 +342,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes result = append(result, FieldChange{ FilePath: block.file, Change: destChange, - FieldCandidates: []string{blocks.candidatePath(block, mergedPath)}, + FieldCandidates: []string{blocks.candidatePath(block, resolvedPath)}, mergedPath: mergedPath, }) continue @@ -383,12 +383,11 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes var candidates []string if routed { // The block is known, so there is exactly one path to write. - candidates = []string{blocks.candidatePath(block, resolvedPathStr)} + candidates = []string{blocks.candidatePath(block, resolvedPath)} } else { candidates = []string{resolvedPathStr} if targetName != "" { - targetPrefixedPath := "targets." + targetName + "." + resolvedPathStr - candidates = append(candidates, targetPrefixedPath) + candidates = append(candidates, targetPrefixedPath(targetName, resolvedPath)) } } From a5b9de5ad11c8ee7be00b706310696605f857345 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 22:15:43 +0000 Subject: [PATCH 13/28] Rename mergedPath to preResolvedPath Co-authored-by: Isaac --- bundle/configsync/resolve.go | 18 ++++++++++-------- bundle/configsync/variables.go | 4 ++-- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index 128b9b2928f..d6cee41608c 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -22,11 +22,13 @@ type FieldChange struct { FilePath string Change *ConfigChangeDesc FieldCandidates []string - // mergedPath addresses the field in a merged configuration, i.e. without the - // targets. prefix that a write to an override block needs. Selecting a - // target folds the override into resources and drops the targets subtree, so a - // prefixed path does not resolve there. - mergedPath string + // preResolvedPath addresses the field in the pre-resolved configuration, the one + // read to recover a ${var.X} reference that deployment replaced with its value. + // That configuration is loaded with the target selected, which folds the override + // into resources and drops the targets subtree, so this path carries no + // targets. prefix -- unlike FieldCandidates, which addresses the raw file + // where the override still lives under that prefix. + preResolvedPath string } // sequenceStep records a sequence element that a change path navigated through. @@ -338,12 +340,12 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes destChange.Operation = OperationReplace destChange.Value = rename.newKey resolvedPath = structpath.NewPatternStringKey(resolvedPath, rename.keyField) - mergedPath := resolvedPath.String() + preResolvedPath := resolvedPath.String() result = append(result, FieldChange{ FilePath: block.file, Change: destChange, FieldCandidates: []string{blocks.candidatePath(block, resolvedPath)}, - mergedPath: mergedPath, + preResolvedPath: preResolvedPath, }) continue } @@ -442,7 +444,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes FilePath: filePath, Change: destChange, FieldCandidates: candidates, - mergedPath: resolvedPathStr, + preResolvedPath: resolvedPathStr, }) } } diff --git a/bundle/configsync/variables.go b/bundle/configsync/variables.go index f4f5149c011..92d39a3f26f 100644 --- a/bundle/configsync/variables.go +++ b/bundle/configsync/variables.go @@ -94,13 +94,13 @@ func RestoreVariableReferences(ctx context.Context, b *bundle.Bundle, fieldChang var newValue any switch fc.Change.Operation { case OperationReplace: - fieldValue, ok := preResolvedValueAt(preResolved, fc.mergedPath) + fieldValue, ok := preResolvedValueAt(preResolved, fc.preResolvedPath) if !ok { continue } newValue = restoreOriginalRefs(fc.Change.Value, fieldValue, resolved, stats) case OperationAdd: - siblings, ok := sequenceSiblings(preResolved, fc.mergedPath) + siblings, ok := sequenceSiblings(preResolved, fc.preResolvedPath) if !ok { continue } From 72f83f77f0b13f087c9fc99f7d59128dfc3b0b53 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 22:22:40 +0000 Subject: [PATCH 14/28] Collapse duplicated field-stripping and match scanning in rename pairing Co-authored-by: Isaac --- bundle/configsync/rename.go | 54 +++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/bundle/configsync/rename.go b/bundle/configsync/rename.go index 64b63f2f4f6..e5b27c66961 100644 --- a/bundle/configsync/rename.go +++ b/bundle/configsync/rename.go @@ -173,16 +173,18 @@ type renameCandidate struct { oldFields map[string]any } +// matchesAdd reports whether the addition at addPath is among these matches. +func matchesAdd(matches []keyedElement, addPath string) bool { + return slices.ContainsFunc(matches, func(add keyedElement) bool { return add.path == addPath }) +} + // countMatchesOf returns how many removals could have become the addition at // addPath. func countMatchesOf(matches [][]keyedElement, addPath string) int { count := 0 for _, candidateMatches := range matches { - for _, add := range candidateMatches { - if add.path == addPath { - count++ - break - } + if matchesAdd(candidateMatches, addPath) { + count++ } } return count @@ -214,14 +216,7 @@ func ambiguityCrossesBlocks(blocks *blockResolver, candidates []renameCandidate, // sharesAnyAdd reports whether two removals could have become the same addition. func sharesAnyAdd(a, b []keyedElement) bool { - for _, x := range a { - for _, y := range b { - if x.path == y.path { - return true - } - } - } - return false + return slices.ContainsFunc(a, func(add keyedElement) bool { return matchesAdd(b, add.path) }) } // multiBlockElement reports whether the element the change addresses is assembled @@ -275,28 +270,29 @@ func keyedElementChanges(changes ResourceChanges) (removes, adds []keyedElement) return removes, adds } -// sameElementApartFromKey reports whether the added element is the removed one -// with a different key. oldFields is the removed element without its key field. +// sameElementApartFromKey reports whether the added element is the removed one with +// a different key, i.e. a plain rename. oldFields is the removed element without its +// key field. func sameElementApartFromKey(oldFields map[string]any, add keyedElement) bool { - newValue, ok := add.value.(map[string]any) - if !ok { - return false - } - return reflect.DeepEqual(oldFields, withoutKey(newValue, add.keyField)) + newFields, ok := fieldsApartFromKey(add) + return ok && reflect.DeepEqual(oldFields, newFields) } -// sameFieldsApartFromKey reports whether the added element has the same fields as -// the removed one, ignoring their values. A rename that also edited a field keeps -// the element's shape; an unrelated new element generally does not. +// sameFieldsApartFromKey reports whether the added element has the same fields as the +// removed one, ignoring their values. A rename that also edited a field keeps the +// element's shape; an unrelated new element generally does not. func sameFieldsApartFromKey(oldFields map[string]any, add keyedElement) bool { - newValue, ok := add.value.(map[string]any) + newFields, ok := fieldsApartFromKey(add) + return ok && slices.Equal(slices.Sorted(maps.Keys(oldFields)), slices.Sorted(maps.Keys(newFields))) +} + +// fieldsApartFromKey returns the added element's fields without its key field. +func fieldsApartFromKey(add keyedElement) (map[string]any, bool) { + value, ok := add.value.(map[string]any) if !ok { - return false + return nil, false } - return slices.Equal( - slices.Sorted(maps.Keys(oldFields)), - slices.Sorted(maps.Keys(withoutKey(newValue, add.keyField))), - ) + return withoutKey(value, add.keyField), true } func withoutKey(value map[string]any, keyField string) map[string]any { From 8addc15329a5a425d44ea15b699f26e297398298 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 22:31:36 +0000 Subject: [PATCH 15/28] Use structdiff.IsEqual instead of reflect.DeepEqual Co-authored-by: Isaac --- bundle/configsync/rename.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/configsync/rename.go b/bundle/configsync/rename.go index e5b27c66961..eccb9716a6e 100644 --- a/bundle/configsync/rename.go +++ b/bundle/configsync/rename.go @@ -4,10 +4,10 @@ import ( "errors" "fmt" "maps" - "reflect" "slices" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/structs/structdiff" "github.com/databricks/cli/libs/structs/structpath" ) @@ -275,7 +275,7 @@ func keyedElementChanges(changes ResourceChanges) (removes, adds []keyedElement) // key field. func sameElementApartFromKey(oldFields map[string]any, add keyedElement) bool { newFields, ok := fieldsApartFromKey(add) - return ok && reflect.DeepEqual(oldFields, newFields) + return ok && structdiff.IsEqual(oldFields, newFields) } // sameFieldsApartFromKey reports whether the added element has the same fields as the From 21eef0c1c9031ea0454167f20b98c27dbb4d666c Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 22:37:06 +0000 Subject: [PATCH 16/28] Restore variables using the merged-index path, not the block-local one Co-authored-by: Isaac --- .../split/target_variable/databricks.yml.tmpl | 23 ++++++++++++++++--- .../split/target_variable/output.txt | 17 ++++++++------ .../split/target_variable/script | 10 +++++--- bundle/configsync/resolve.go | 19 +++++++++------ 4 files changed, 49 insertions(+), 20 deletions(-) diff --git a/acceptance/bundle/config-remote-sync/split/target_variable/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/split/target_variable/databricks.yml.tmpl index 4bbe54528e1..da9cb6f3e66 100644 --- a/acceptance/bundle/config-remote-sync/split/target_variable/databricks.yml.tmpl +++ b/acceptance/bundle/config-remote-sync/split/target_variable/databricks.yml.tmpl @@ -1,14 +1,31 @@ bundle: name: test-bundle-$UNIQUE_NAME +# notebook_path is defined ONLY in the target override block, and its value contains a +# ${var.X} reference. Variable references are restored by looking the field up in the +# pre-resolved configuration, where the target has already been folded into resources, +# so that lookup path must not carry a targets. prefix -- even though the write +# itself has to. +# +# The lookup also has to keep MERGED sequence indices, which is why "aaa" exists: it +# sorts before "zzz" and lives in the top-level block, so zzz is index 1 after the +# merge but index 0 inside the block it is written to. Each task carries its own +# variable, so reading the wrong sibling shows up as the wrong variable rather than +# merely a lost reference. variables: - root: + shared_dir: + default: base + dev_dir: default: base resources: jobs: var_job: max_concurrent_runs: 1 + tasks: + - task_key: aaa + notebook_task: + notebook_path: /Users/${var.shared_dir}/aaa targets: dev: @@ -17,6 +34,6 @@ targets: jobs: var_job: tasks: - - task_key: main + - task_key: zzz notebook_task: - notebook_path: /Users/${var.root}/nb + notebook_path: /Users/${var.dev_dir}/zzz diff --git a/acceptance/bundle/config-remote-sync/split/target_variable/output.txt b/acceptance/bundle/config-remote-sync/split/target_variable/output.txt index d81480fb0f8..9e138def93b 100644 --- a/acceptance/bundle/config-remote-sync/split/target_variable/output.txt +++ b/acceptance/bundle/config-remote-sync/split/target_variable/output.txt @@ -8,22 +8,25 @@ Deployment complete! Detected changes in 1 resource(s): Resource: resources.jobs.var_job - tasks[task_key='main'].notebook_task.notebook_path: replace + tasks[task_key='zzz'].notebook_task.notebook_path: replace -=== The variable reference is preserved +=== zzz keeps its own variable, and aaa is untouched >>> diff.py databricks.yml.backup databricks.yml --- databricks.yml.backup +++ databricks.yml -@@ -20,3 +20,3 @@ - - task_key: main +@@ -37,3 +37,3 @@ + - task_key: zzz notebook_task: -- notebook_path: /Users/${var.root}/nb -+ notebook_path: /Users/${var.root}/renamed +- notebook_path: /Users/${var.dev_dir}/zzz ++ notebook_path: /Users/${var.dev_dir}/renamed ->>> grep -c var.root databricks.yml +>>> grep -c var.dev_dir databricks.yml +1 + +>>> grep -c var.shared_dir databricks.yml 1 >>> [CLI] bundle destroy --auto-approve -t dev diff --git a/acceptance/bundle/config-remote-sync/split/target_variable/script b/acceptance/bundle/config-remote-sync/split/target_variable/script index a5f19a4065f..da7ff428244 100644 --- a/acceptance/bundle/config-remote-sync/split/target_variable/script +++ b/acceptance/bundle/config-remote-sync/split/target_variable/script @@ -13,9 +13,12 @@ job_id="$(read_id.py var_job)" # Edit only the trailing segment of a path whose leading segment came from ${var.root}. # The reference must survive; hardcoding it would bake the dev value into the source. +# zzz is index 1 in the merged list but index 0 in the target block it is written to. title "Rename the notebook inside a target-block field that uses a variable" edit_resource.py jobs $job_id < prefix -- unlike FieldCandidates, which addresses the raw file - // where the override still lives under that prefix. + // + // That configuration is merged, so this path keeps the MERGED sequence indices and + // no targets. prefix. FieldCandidates is the opposite on both counts: it + // addresses the raw file, so its indices are block-local and an override carries + // the prefix. Deriving one from the other reads a different element whenever the + // two index spaces disagree, which restores a sibling's variable reference. preResolvedPath string } @@ -286,6 +288,10 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes // per-block order differ once a sequence is split across blocks. An // element assembled from several blocks has a part in each, so removing // or renaming it yields one destination per block. + // Variable restoration resolves against the pre-resolved configuration, + // which is merged, so it needs the path in merged index space -- captured + // here, before pathWithinBlock rewrites indices to be block-local. + mergedIndexPath := resolvedPath destinations := []routeDestination{{path: resolvedPath}} routed := false if blocks != nil && len(resolved.steps) > 0 { @@ -340,12 +346,11 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes destChange.Operation = OperationReplace destChange.Value = rename.newKey resolvedPath = structpath.NewPatternStringKey(resolvedPath, rename.keyField) - preResolvedPath := resolvedPath.String() result = append(result, FieldChange{ FilePath: block.file, Change: destChange, FieldCandidates: []string{blocks.candidatePath(block, resolvedPath)}, - preResolvedPath: preResolvedPath, + preResolvedPath: structpath.NewPatternStringKey(mergedIndexPath, rename.keyField).String(), }) continue } @@ -444,7 +449,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes FilePath: filePath, Change: destChange, FieldCandidates: candidates, - preResolvedPath: resolvedPathStr, + preResolvedPath: mergedIndexPath.String(), }) } } From 8ca0df5cd4c76e0c11650ad379a6142e93cf3b6c Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 22:46:15 +0000 Subject: [PATCH 17/28] Translate enclosing indices when placing a nested new element Co-authored-by: Isaac --- .../databricks.yml.tmpl | 33 ++++++++++++++ .../nested_add_split_parent/out.test.toml | 4 ++ .../split/nested_add_split_parent/output.txt | 43 +++++++++++++++++++ .../split/nested_add_split_parent/script | 25 +++++++++++ bundle/configsync/blockindex.go | 39 +++++++++++++++-- 5 files changed, 141 insertions(+), 3 deletions(-) create mode 100644 acceptance/bundle/config-remote-sync/split/nested_add_split_parent/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/nested_add_split_parent/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/nested_add_split_parent/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/nested_add_split_parent/script diff --git a/acceptance/bundle/config-remote-sync/split/nested_add_split_parent/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/split/nested_add_split_parent/databricks.yml.tmpl new file mode 100644 index 00000000000..efc32fb7472 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/nested_add_split_parent/databricks.yml.tmpl @@ -0,0 +1,33 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +# depends_on is a keyed sequence nested inside tasks. "zzz_shared" is defined in both +# blocks and sorts after the target-only "aaa_target", so it is index 1 in the merged +# list but index 0 in the top-level block that holds its depends_on. +# +# Placing a new depends_on entry means finding the sequence that receives it, and that +# lookup runs per block -- so the enclosing task index has to be translated first. +# Probing the merged index inside a block finds nothing, and the addition is dropped. +resources: + jobs: + nested_add_job: + tasks: + - task_key: zzz_shared + max_retries: 1 + depends_on: + - task_key: aaa_target + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/nb + +targets: + dev: + mode: development + resources: + jobs: + nested_add_job: + tasks: + - task_key: aaa_target + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/nb + - task_key: zzz_shared + timeout_seconds: 60 diff --git a/acceptance/bundle/config-remote-sync/split/nested_add_split_parent/out.test.toml b/acceptance/bundle/config-remote-sync/split/nested_add_split_parent/out.test.toml new file mode 100644 index 00000000000..4b5914daa2c --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/nested_add_split_parent/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = false +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/split/nested_add_split_parent/output.txt b/acceptance/bundle/config-remote-sync/split/nested_add_split_parent/output.txt new file mode 100644 index 00000000000..21182810044 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/nested_add_split_parent/output.txt @@ -0,0 +1,43 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Add a task, then depend on it from the split task zzz_shared +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.nested_add_job + tasks[task_key='mid_new']: add + tasks[task_key='zzz_shared'].depends_on[task_key='mid_new']: add + + + +=== The dependency lands on zzz_shared, in the block that defines it + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -17,6 +17,10 @@ + depends_on: + - task_key: aaa_target ++ - task_key: mid_new + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/nb ++ - notebook_task: ++ notebook_path: /Users/{{workspace_user_name}}/nb ++ task_key: mid_new + + targets: + +>>> grep -c task_key: mid_new databricks.yml +2 + +>>> [CLI] bundle destroy --auto-approve -t dev +The following resources will be deleted: + delete resources.jobs.nested_add_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/split/nested_add_split_parent/script b/acceptance/bundle/config-remote-sync/split/nested_add_split_parent/script new file mode 100644 index 00000000000..cc23a266e2a --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/nested_add_split_parent/script @@ -0,0 +1,25 @@ +#!/bin/bash +envsubst < databricks.yml.tmpl > databricks.yml +cleanup() { trace $CLI bundle destroy --auto-approve -t dev; } +trap cleanup EXIT +$CLI bundle deploy -t dev +job_id="$(read_id.py nested_add_job)" + +title "Add a task, then depend on it from the split task zzz_shared" +edit_resource.py jobs $job_id <= len(sequencePath) { + continue + } + // The step's own path is a prefix of sequencePath, so the indices translated + // so far already apply to it. + index, ok := r.indexWithinBlock(block, result[:at], step.element) + if !ok { + return nil, false + } + result[at] = dyn.Index(index) + } + return result, true +} + // pathWithinBlock rewrites a change's path so every sequence index addresses the // element's position inside block rather than its position in the merged list. func (r *blockResolver) pathWithinBlock(block sourceBlock, change resolvedChange) (*structpath.PatternNode, error) { From ffb1028e419e575f20f0622b1ed1b1855e4ba7ad Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 23:18:43 +0000 Subject: [PATCH 18/28] Drop unreachable zero-step branch in block routing Co-authored-by: Isaac --- bundle/configsync/blockindex.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/bundle/configsync/blockindex.go b/bundle/configsync/blockindex.go index 8816142a6ea..cb482415f5b 100644 --- a/bundle/configsync/blockindex.go +++ b/bundle/configsync/blockindex.go @@ -312,6 +312,11 @@ type routeDestination struct { // routeDestinations returns every physical place a change has to be written. // +// The change must address something inside a sequence: ResolveChanges only routes when +// the path has a sequence step, and a rename path always ends in a [key='...'] +// selector. A change with no sequence on its path needs no per-block translation and +// is written at its resolved path directly. +// // An edit has one destination: only the definition that wins the merge decides the // deployed value. A removal has one per definition, because the field is gone only // once every copy is: deleting just the winning copy lets the shadowed one take @@ -381,14 +386,6 @@ func (r *blockResolver) blockFor(change resolvedChange) (sourceBlock, error) { } } - // No sequence on the path: route by the leaf's own location. - if len(change.steps) == 0 { - if block, ok := r.winningBlock(change.leaf); ok { - return block, nil - } - return sourceBlock{}, fmt.Errorf("%w: no source location for the change", errAmbiguousBlock) - } - element := change.steps[len(change.steps)-1].element blocks := r.blocksOf(element) if len(blocks) == 0 { From ecbad190a32188d861fceafb716f1054e0ab5b68 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 23:31:34 +0000 Subject: [PATCH 19/28] Add unit tests for block resolution and index translation Co-authored-by: Isaac --- bundle/configsync/blockindex_test.go | 228 +++++++++++++++++++++++++++ 1 file changed, 228 insertions(+) create mode 100644 bundle/configsync/blockindex_test.go diff --git a/bundle/configsync/blockindex_test.go b/bundle/configsync/blockindex_test.go new file mode 100644 index 00000000000..a91dd130ebc --- /dev/null +++ b/bundle/configsync/blockindex_test.go @@ -0,0 +1,228 @@ +package configsync + +import ( + "os" + "path/filepath" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/mutator" + "github.com/databricks/cli/bundle/config/mutator/resourcemutator" + "github.com/databricks/cli/libs/logdiag" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// loadBundle loads files the way the sync command does, so a target's overrides are +// merged into the resources tree and keyed sequences are merged by key. Keyed +// sequences merge in the initialize phase, after the target is selected, so that +// mutator has to be applied explicitly here. +func loadBundle(t *testing.T, target string, files map[string]string) (*bundle.Bundle, *blockResolver) { + t.Helper() + dir := t.TempDir() + for name, content := range files { + path := filepath.Join(dir, name) + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) + } + + ctx := logdiag.InitContext(t.Context()) + b, err := bundle.Load(ctx, dir) + require.NoError(t, err) + mutator.DefaultMutators(ctx, b) + if target != "" { + bundle.ApplyContext(ctx, b, mutator.SelectTarget(target)) + } + bundle.ApplySeqContext(ctx, b, resourcemutator.MergeJobTasks()) + + blocks := newBlockResolver(ctx, b) + require.NotNil(t, blocks) + return b, blocks +} + +// "shared" is defined in both blocks, and the target contributes a task that sorts +// ahead of it, so the merged order differs from either block's own order. +const splitTasks = `bundle: + name: split + +resources: + jobs: + j: + tasks: + - task_key: shared + max_retries: 1 + depends_on: + - task_key: zzz_top + - task_key: zzz_top + max_retries: 5 + +targets: + dev: + resources: + jobs: + j: + tasks: + - task_key: aaa_dev + max_retries: 6 + - task_key: shared + timeout_seconds: 60 +` + +func TestBlockResolverRoutesFieldToDefiningBlock(t *testing.T) { + b, blocks := loadBundle(t, "dev", map[string]string{"databricks.yml": splitTasks}) + + tests := []struct { + name string + path string + override bool + want string + }{ + { + name: "field defined only top-level", + path: "resources.jobs.j.tasks[task_key='zzz_top'].max_retries", + override: false, + want: "resources.jobs.j.tasks[1].max_retries", + }, + { + name: "field defined only in the target", + path: "resources.jobs.j.tasks[task_key='aaa_dev'].max_retries", + override: true, + want: "resources.jobs.j.tasks[0].max_retries", + }, + { + // The target's value is the deployed one, so writing the top-level copy + // would leave the effective value unchanged. + name: "field of a two-block element routes to its own block", + path: "resources.jobs.j.tasks[task_key='shared'].timeout_seconds", + override: true, + want: "resources.jobs.j.tasks[1].timeout_seconds", + }, + { + // "shared" is merged index 1 but index 0 in the top-level block, so the + // enclosing index has to be translated before the nested lookup. + name: "nested sequence under a shifted element", + path: "resources.jobs.j.tasks[task_key='shared'].depends_on[task_key='zzz_top']", + override: false, + want: "resources.jobs.j.tasks[0].depends_on[0]", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resolved, err := resolveSelectors(tc.path, b, OperationReplace) + require.NoError(t, err) + + destination, err := blocks.singleDestination(resolved) + require.NoError(t, err) + assert.Equal(t, tc.override, destination.block.override) + assert.Equal(t, tc.want, destination.path.String()) + }) + } +} + +func TestBlockResolverRoutesElementToEveryDefiningBlock(t *testing.T) { + b, blocks := loadBundle(t, "dev", map[string]string{"databricks.yml": splitTasks}) + + // Removing an element defined in both blocks has to reach the part in each, and + // each part has its own index inside its own block. + resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='shared']", b, OperationRemove) + require.NoError(t, err) + + destinations, err := blocks.routeDestinations(resolved) + require.NoError(t, err) + + got := map[bool]string{} + for _, d := range destinations { + got[d.block.override] = d.path.String() + } + assert.Equal(t, map[bool]string{ + false: "resources.jobs.j.tasks[0]", + true: "resources.jobs.j.tasks[1]", + }, got) +} + +func TestBlockResolverPlacesNewNestedElementInParentBlock(t *testing.T) { + b, blocks := loadBundle(t, "dev", map[string]string{"databricks.yml": splitTasks}) + + // A new depends_on entry on a task whose merged index differs from its + // block-local one: finding the receiving sequence needs the enclosing index + // translated first. + resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='shared'].depends_on[task_key='aaa_dev']", b, OperationAdd) + require.NoError(t, err) + + destination, err := blocks.singleDestination(resolved) + require.NoError(t, err) + assert.False(t, destination.block.override) + assert.Equal(t, "resources.jobs.j.tasks[0].depends_on[*]", destination.path.String()) +} + +func TestBlockResolverRefusesElementSpanningBlocks(t *testing.T) { + b, blocks := loadBundle(t, "dev", map[string]string{"databricks.yml": splitTasks}) + + // A field the element does not define anywhere has no location to route by, so + // it is an addition and goes to the block declaring the resource. + resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='shared'].min_retry_interval_millis", b, OperationReplace) + require.NoError(t, err) + + destination, err := blocks.singleDestination(resolved) + require.NoError(t, err) + assert.False(t, destination.block.override) +} + +func TestBlockResolverCountsIndexPerBlockAcrossFiles(t *testing.T) { + // One target's override spread over two included files. Resource keys are unique + // across top-level files but that is not enforced inside targets, so a task + // second in the concatenated target region can be first in the file that defines + // it -- and the write has to use the latter. + b, blocks := loadBundle(t, "dev", map[string]string{ + "databricks.yml": `bundle: + name: split + +include: + - overrides/*.yml + +resources: + jobs: + j: + tasks: + - task_key: base +`, + "overrides/10-first.yml": `targets: + dev: + resources: + jobs: + j: + tasks: + - task_key: from_first + max_retries: 1 +`, + "overrides/20-second.yml": `targets: + dev: + resources: + jobs: + j: + tasks: + - task_key: from_second + max_retries: 2 +`, + }) + + resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='from_second'].max_retries", b, OperationReplace) + require.NoError(t, err) + + destination, err := blocks.singleDestination(resolved) + require.NoError(t, err) + assert.Equal(t, "20-second.yml", filepath.Base(destination.block.file)) + assert.Equal(t, "resources.jobs.j.tasks[0].max_retries", destination.path.String()) +} + +func TestCompareBlocksOrdersTopLevelFirst(t *testing.T) { + // blocksOf and sortedBlocks both rely on this order so that declaringBlock picks + // the top-level block and callers never depend on map iteration order. + top := sourceBlock{file: "z.yml"} + override := sourceBlock{override: true, file: "a.yml"} + assert.Negative(t, compareBlocks(top, override)) + assert.Positive(t, compareBlocks(override, top)) + assert.Negative(t, compareBlocks(sourceBlock{file: "a.yml"}, sourceBlock{file: "b.yml"})) + assert.Zero(t, compareBlocks(top, top)) +} From fa2140bd1d250dd86b6274afe2b310ffb45b809a Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 23:42:55 +0000 Subject: [PATCH 20/28] Simplify block routing: unify index translation, retire routed flag and FieldCandidates Co-authored-by: Isaac --- bundle/configsync/blockindex.go | 142 ++++++++++++++------------------ bundle/configsync/patch.go | 6 +- bundle/configsync/rename.go | 88 ++++++++++---------- bundle/configsync/resolve.go | 57 +++++++------ 4 files changed, 141 insertions(+), 152 deletions(-) diff --git a/bundle/configsync/blockindex.go b/bundle/configsync/blockindex.go index cb482415f5b..230fcc2f64f 100644 --- a/bundle/configsync/blockindex.go +++ b/bundle/configsync/blockindex.go @@ -16,17 +16,19 @@ import ( "github.com/databricks/cli/libs/structs/structpath" ) -// A sequence field of a resource may be defined in more than one physical YAML -// region: the top-level resources... block and the -// targets..resources... override block, either of -// which may live in its own included file. Loading merges those regions into one -// sequence, and keyed sequences are also sorted by key, so an index into the -// merged sequence does not address any single region. Write-back has to map a -// merged position back to a region and to the position inside that region. -// There are exactly two kinds of region, so a block is a file plus which of the -// two it is. Both kinds may occur in the same file, so the file alone does not -// identify a block, and one target's override may span several files, so the kind -// alone does not either. +// Write-back has to turn a position in the merged configuration into a position in a +// file, and the two do not correspond. +// +// A sequence field of a resource may be defined in two physical regions: the top-level +// resources... block and the targets..resources... override +// block, either of which may live in its own included file. Loading concatenates those +// regions into one sequence and sorts keyed ones by key, so a merged index addresses +// no single region. Every index therefore exists in one of two spaces -- merged, or +// local to one block -- and each value in this file belongs to exactly one of them; +// crossing them silently addresses a different element. +// +// A block is (kind, file), because both kinds may occur in one file and one target's +// override may span several files, so neither identifies a block alone. type sourceBlock struct { // override is false for the top-level block and true for the selected target's // override block. @@ -43,14 +45,10 @@ func (b sourceBlock) scopeKey() string { return "toplevel\x00" + b.file + "\x00" } -// blockResolver answers which physical block a merged value came from. -// -// It works by location: a merged value keeps the locations of the entries it was -// loaded from, and merging accumulates them (see libs/dyn/merge), so a value -// assembled from two blocks reports a location in each. Selecting a target folds -// its overrides into the resources tree and drops the targets subtree, so the -// blocks are recovered by parsing the contributing files again, where the two -// regions are still separate. +// blockResolver answers which physical block a merged value came from, by location: +// merging accumulates them (libs/dyn/merge), so a value assembled from two blocks +// reports a location in each. Selecting a target folds its overrides into resources and +// drops the targets subtree, so the blocks are recovered by parsing the files again. type blockResolver struct { // blocks holds one parsed file per contributing file, keyed by block, so a // path relative to a block can be looked up inside it. @@ -59,14 +57,12 @@ type blockResolver struct { byLocation map[dyn.Location]sourceBlock } -// newBlockResolver builds the location -> block mapping for the bundle's -// resources. It returns nil when the source cannot be re-read, in which case -// callers keep their existing single-block behaviour. +// newBlockResolver builds the location -> block mapping for the bundle's resources, +// returning nil when the source cannot be re-read so callers keep their existing +// single-block behaviour. // -// Only the files the merged configuration actually references are read, and they -// are parsed directly rather than reloaded through the mutator pipeline: the -// pipeline resolves includes, reports through logdiag and executes the bundle's -// preinit script, none of which belongs to reading back a source location. +// The contributing files are parsed directly rather than reloaded through the mutator +// pipeline, which would run the bundle's preinit script a second time. func newBlockResolver(ctx context.Context, b *bundle.Bundle) *blockResolver { root := b.Config.Value() if !root.IsValid() { @@ -219,20 +215,17 @@ func (r *blockResolver) blocksOf(value dyn.Value) []sourceBlock { blocks = append(blocks, block) } } - // Order is total and independent of how locations happened to accumulate, so - // callers that prefer the top-level block get a deterministic answer. + // Total order, so declaringBlock's choice never depends on how locations + // happened to accumulate. slices.SortFunc(blocks, compareBlocks) return blocks } -// winningBlock returns the block whose definition the merged value took, when -// several blocks define it. +// winningBlock returns the block whose definition the merged value took. // // Locations accumulate in merge order, so the first one that maps to a block is the -// winner: mergePrimitive records the incoming (overriding) value's location first, -// and writing any other copy would leave the effective value unchanged. Load order -// is the only thing that distinguishes two blocks in the same scope, and no property -// of the blocks themselves reproduces it. +// winner and writing any other copy would leave the effective value unchanged. Load +// order is the only thing that distinguishes two blocks in the same scope. func (r *blockResolver) winningBlock(value dyn.Value) (sourceBlock, bool) { for _, location := range value.Locations() { if block, ok := r.byLocation[location]; ok { @@ -242,13 +235,9 @@ func (r *blockResolver) winningBlock(value dyn.Value) (sourceBlock, bool) { return sourceBlock{}, false } -// indexWithinBlock returns the position of element in the sequence that block -// writes at sequencePath, where sequencePath is relative to the block. That position -// differs from the element's position in the merged sequence, which concatenates -// every block's entries and sorts keyed ones by key. -// -// A block is one parsed file, so the sequence read here holds only that file's -// entries and a plain index into it addresses the right entry. +// indexWithinBlock returns the position of element inside block, where sequencePath is +// relative to the block. A block is one parsed file, so the sequence read here holds +// only that file's entries and a plain index into it is block-local. func (r *blockResolver) indexWithinBlock(block sourceBlock, sequencePath dyn.Path, element dyn.Value) (int, bool) { parsed, ok := r.blocks[block] if !ok { @@ -283,8 +272,7 @@ func (r *blockResolver) indexWithinBlock(block sourceBlock, sequencePath dyn.Pat // location, since the sync runs unattended and a later run can retry. var errAmbiguousBlock = errors.New("change cannot be attributed to a single source block") -// singleDestination maps a change onto the one block that owns it, rewriting -// merged sequence indices into block-local ones. +// singleDestination maps a change onto the one block that owns it. // // The element the change addresses decides the block: an element defined in // exactly one block is written there. When the addressed element is assembled from @@ -301,29 +289,32 @@ func (r *blockResolver) singleDestination(change resolvedChange) (routeDestinati if err != nil { return routeDestination{}, err } - return routeDestination{block: block, path: path}, nil + return routeDestination{block: &block, path: path}, nil } -// routeDestination is one physical place a change has to be written. +// routeDestination is one physical place a change has to be written. block is nil when +// the change was not routed to a block, i.e. it is written at its resolved path in +// whichever file the resource was found in. type routeDestination struct { - block sourceBlock + block *sourceBlock path *structpath.PatternNode } -// routeDestinations returns every physical place a change has to be written. -// -// The change must address something inside a sequence: ResolveChanges only routes when -// the path has a sequence step, and a rename path always ends in a [key='...'] -// selector. A change with no sequence on its path needs no per-block translation and -// is written at its resolved path directly. +// scopeKey identifies the destination for index bookkeeping, so operations on one +// block cannot shift positions recorded for another. An unrouted destination shares +// one scope, which is the behaviour before blocks were known. +func (d routeDestination) scopeKey() string { + if d.block == nil { + return "" + } + return d.block.scopeKey() +} + +// routeDestinations returns every physical place a change has to be written: one for an +// edit, since only the definition that wins the merge decides the deployed value, and +// one per definition for a removal, since the value is gone only once every copy is. // -// An edit has one destination: only the definition that wins the merge decides the -// deployed value. A removal has one per definition, because the field is gone only -// once every copy is: deleting just the winning copy lets the shadowed one take -// effect, so the next deploy restores the value the removal was meant to drop. -// The same holds for a whole sequence element, which additionally has to be removed -// or renamed in each block that contributes a part, so that a split element keeps -// its parts in their original scopes. +// Callers only route a change that addresses something inside a sequence. func (r *blockResolver) routeDestinations(change resolvedChange) ([]routeDestination, error) { // The value whose definitions have to be reached: the element itself when the // change addresses one, otherwise the field being removed. @@ -353,7 +344,7 @@ func (r *blockResolver) routeDestinations(change resolvedChange) ([]routeDestina if err != nil { return nil, err } - destinations = append(destinations, routeDestination{block: block, path: path}) + destinations = append(destinations, routeDestination{block: &block, path: path}) } return destinations, nil } @@ -454,14 +445,9 @@ func (r *blockResolver) blockForNewElement(change resolvedChange) (sourceBlock, return declaringBlock(blocks), nil } -// blocksDefiningSequence returns the blocks that write the sequence at -// sequencePath. An existing value is traced through its locations instead; this is -// for a value that does not exist yet, where only the receiving sequence is known. -// -// sequencePath is in merged index space, so for a nested sequence its enclosing -// indices have to be translated per block before the lookup: a task at merged index 1 -// may be index 0 in the block that defines it, and probing the merged index there -// finds nothing (or the wrong element). +// blocksDefiningSequence returns the blocks that write the sequence at sequencePath. +// This is for a value that does not exist yet, where only the receiving sequence is +// known; an existing value is traced through its own locations instead. func (r *blockResolver) blocksDefiningSequence(change resolvedChange, sequencePath dyn.Path) []sourceBlock { var blocks []sourceBlock for _, block := range r.sortedBlocks() { @@ -480,9 +466,9 @@ func (r *blockResolver) blocksDefiningSequence(change resolvedChange, sequencePa return blocks } -// sequencePathWithinBlock rewrites the enclosing sequence indices of sequencePath -// from merged positions to block's own positions. Reports false when an enclosing -// element is not in this block, which means the block cannot receive the value. +// sequencePathWithinBlock rewrites sequencePath's enclosing indices from merged +// positions to block's own, reporting false when an enclosing element is not in this +// block. This is the one place merged indices become block-local ones. // // change.steps is ordered outermost first, so each translated index is already known // by the time a deeper step needs it. @@ -516,18 +502,10 @@ func (r *blockResolver) pathWithinBlock(block sourceBlock, change resolvedChange indices[i] = -1 continue } - // A nested sequence is reached through the outer sequences on the path, - // whose indices refer to the merged view. Rewrite them to the block's own - // indices, otherwise the lookup addresses the wrong parent element. - sequencePath := slices.Clone(step.sequencePath) - for j := range i { - outer := change.steps[j] - if indices[j] < 0 || len(outer.sequencePath) >= len(sequencePath) { - continue - } - sequencePath[len(outer.sequencePath)] = dyn.Index(indices[j]) + sequencePath, ok := r.sequencePathWithinBlock(block, change, step.sequencePath) + if !ok { + return nil, fmt.Errorf("%w: enclosing element has no position in %s", errAmbiguousBlock, block.file) } - index, ok := r.indexWithinBlock(block, sequencePath, step.element) if !ok { return nil, fmt.Errorf("%w: element has no position in %s", errAmbiguousBlock, block.file) diff --git a/bundle/configsync/patch.go b/bundle/configsync/patch.go index e002d31335c..573b5d2c1ba 100644 --- a/bundle/configsync/patch.go +++ b/bundle/configsync/patch.go @@ -40,7 +40,7 @@ func ApplyChangesToYAML(ctx context.Context, b *bundle.Bundle, fieldChanges []Fi modifiedContent, err := applyChange(ctx, modifiedFiles[filePath], fieldChange) if err != nil { - return nil, fmt.Errorf("failed to apply change to file %s for a field %s: %w", filePath, fieldChange.FieldCandidates[0], err) + return nil, fmt.Errorf("failed to apply change to file %s for a field %s: %w", filePath, fieldChange.WritePath, err) } modifiedFiles[filePath] = modifiedContent @@ -81,7 +81,7 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( var firstErr error var parentNodesToCreate []parentNode - for _, fieldPathCandidate := range fieldChange.FieldCandidates { + for _, fieldPathCandidate := range fieldChange.writePaths() { jsonPointer, err := strPathToJSONPointer(fieldPathCandidate) if err != nil { return nil, fmt.Errorf("failed to convert field path %q to JSON pointer: %w", fieldPathCandidate, err) @@ -285,7 +285,7 @@ func clearAddedFlowStyle(content []byte, fieldChanges []FieldChange) ([]byte, er return content, nil //nolint:nilerr // return original content if YAML parsing fails } for _, fc := range fieldChanges { - for _, candidate := range fc.FieldCandidates { + for _, candidate := range fc.writePaths() { clearFlowStyleAlongPath(&doc, candidate) } } diff --git a/bundle/configsync/rename.go b/bundle/configsync/rename.go index eccb9716a6e..2a31117cc6c 100644 --- a/bundle/configsync/rename.go +++ b/bundle/configsync/rename.go @@ -79,13 +79,15 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch // just as a removal can match several additions. matches := make([][]keyedElement, len(candidates)) for i, candidate := range candidates { - for _, add := range adds { - if candidate.remove.parent != add.parent || candidate.remove.keyField != add.keyField { - continue - } - if sameElementApartFromKey(candidate.oldFields, add) { - matches[i] = append(matches[i], add) - } + matches[i] = matchingAdds(candidate, adds, sameElementApartFromKey) + } + + // Which removals each addition matches, so "forced from both sides" is a lookup + // rather than a rescan of every other removal's matches. + matchedBy := map[string][]int{} + for i, candidateMatches := range matches { + for _, add := range candidateMatches { + matchedBy[add.path] = append(matchedBy[add.path], i) } } @@ -94,7 +96,7 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch // removal matching several additions is the obvious case, but so is several // removals matching the same addition: pairing the first and deleting the // rest would move a key into a block the user did not choose. - if len(matches[i]) == 1 && countMatchesOf(matches, matches[i][0].path) == 1 { + if len(matches[i]) == 1 && len(matchedBy[matches[i][0].path]) == 1 { add := matches[i][0] set.byRemovePath[candidate.remove.path] = renamedElement{ keyField: candidate.remove.keyField, @@ -114,7 +116,7 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch // as a plain removal and addition instead of being held back -- otherwise a // rename of two same-bodied elements is dropped, and anything referring to // them by key (depends_on) keeps pointing at keys that no longer exist. - if !ambiguityCrossesBlocks(blocks, candidates, matches, i) { + if !ambiguityCrossesBlocks(blocks, candidates, matches[i], matchedBy) { continue } const reason = "several elements with the same contents were renamed in one run and they are not in the same block, so the new keys cannot be matched to them" @@ -137,14 +139,8 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch // a genuinely new element is an unrelated addition that has to be applied, // even when a removal happens to be in the same run. var edited []string - for _, add := range adds { - if candidate.remove.parent != add.parent || candidate.remove.keyField != add.keyField { - continue - } - if _, taken := set.addPaths[add.path]; taken { - continue - } - if sameFieldsApartFromKey(candidate.oldFields, add) { + for _, add := range matchingAdds(candidate, adds, sameFieldsApartFromKey) { + if _, taken := set.addPaths[add.path]; !taken { edited = append(edited, add.path) } } @@ -173,37 +169,32 @@ type renameCandidate struct { oldFields map[string]any } -// matchesAdd reports whether the addition at addPath is among these matches. -func matchesAdd(matches []keyedElement, addPath string) bool { - return slices.ContainsFunc(matches, func(add keyedElement) bool { return add.path == addPath }) -} - -// countMatchesOf returns how many removals could have become the addition at -// addPath. -func countMatchesOf(matches [][]keyedElement, addPath string) int { - count := 0 - for _, candidateMatches := range matches { - if matchesAdd(candidateMatches, addPath) { - count++ +// matchingAdds returns the additions in the same sequence that satisfy pred, i.e. the +// ones this removal could have become. +func matchingAdds(candidate renameCandidate, adds []keyedElement, pred func(map[string]any, keyedElement) bool) []keyedElement { + var out []keyedElement + for _, add := range adds { + if candidate.remove.parent != add.parent || candidate.remove.keyField != add.keyField { + continue + } + if pred(candidate.oldFields, add) { + out = append(out, add) } } - return count + return out } -// ambiguityCrossesBlocks reports whether an unforced pairing for candidates[i] -// would have to choose between blocks. Every removal that could have become one of -// the same additions is considered, because the choice is between them. -func ambiguityCrossesBlocks(blocks *blockResolver, candidates []renameCandidate, matches [][]keyedElement, i int) bool { +// ambiguityCrossesBlocks reports whether an unforced pairing would have to choose +// between blocks. Every removal that could have become one of the same additions is +// considered, because the choice is between them. +func ambiguityCrossesBlocks(blocks *blockResolver, candidates []renameCandidate, matches []keyedElement, matchedBy map[string][]int) bool { var seen []sourceBlock - for j, candidate := range candidates { - if j != i && !sharesAnyAdd(matches[i], matches[j]) { - continue - } - if len(candidate.resolved.steps) == 0 { + for _, i := range competingRemovals(matches, matchedBy) { + steps := candidates[i].resolved.steps + if len(steps) == 0 { return true } - last := candidate.resolved.steps[len(candidate.resolved.steps)-1] - elementBlocks := blocks.blocksOf(last.element) + elementBlocks := blocks.blocksOf(steps[len(steps)-1].element) if len(elementBlocks) != 1 { return true } @@ -214,9 +205,18 @@ func ambiguityCrossesBlocks(blocks *blockResolver, candidates []renameCandidate, return len(seen) != 1 } -// sharesAnyAdd reports whether two removals could have become the same addition. -func sharesAnyAdd(a, b []keyedElement) bool { - return slices.ContainsFunc(a, func(add keyedElement) bool { return matchesAdd(b, add.path) }) +// competingRemovals returns the indices of every removal matching any of these +// additions, including the one they came from. +func competingRemovals(matches []keyedElement, matchedBy map[string][]int) []int { + var out []int + for _, add := range matches { + for _, i := range matchedBy[add.path] { + if !slices.Contains(out, i) { + out = append(out, i) + } + } + } + return out } // multiBlockElement reports whether the element the change addresses is assembled diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index e255c4ea154..519b807aa31 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -19,14 +19,20 @@ import ( ) type FieldChange struct { - FilePath string - Change *ConfigChangeDesc - FieldCandidates []string + FilePath string + Change *ConfigChangeDesc + // WritePath addresses the field in the file at FilePath, so for an override block + // it carries the targets. prefix and its sequence indices are block-local. + WritePath string + // AltWritePath is tried when WritePath does not exist in the file. It is only set + // when the change could not be routed to a block, i.e. when the scope had to be + // guessed; a routed change has exactly one correct path. + AltWritePath string // preResolvedPath addresses the field in the pre-resolved configuration, the one // read to recover a ${var.X} reference that deployment replaced with its value. // // That configuration is merged, so this path keeps the MERGED sequence indices and - // no targets. prefix. FieldCandidates is the opposite on both counts: it + // no targets. prefix. WritePath is the opposite on both counts: it // addresses the raw file, so its indices are block-local and an override carries // the prefix. Deriving one from the other reads a different element whenever the // two index spaces disagree, which restores a sibling's variable reference. @@ -293,7 +299,6 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes // here, before pathWithinBlock rewrites indices to be block-local. mergedIndexPath := resolvedPath destinations := []routeDestination{{path: resolvedPath}} - routed := false if blocks != nil && len(resolved.steps) > 0 { var routeErr error if isRename { @@ -315,7 +320,6 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes } return nil, fmt.Errorf("failed to route change %s: %w", fullPath, routeErr) } - routed = true } for _, destination := range destinations { @@ -332,10 +336,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes // Index bookkeeping is scoped to a block so that shifts caused by // operations on one block cannot move indices in another. - scope := "" - if routed { - scope = block.scopeKey() - } + scope := destination.scopeKey() // A rename rewrites only the key field of the element, so it is a // replace at the element's position rather than a change to the @@ -349,7 +350,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes result = append(result, FieldChange{ FilePath: block.file, Change: destChange, - FieldCandidates: []string{blocks.candidatePath(block, resolvedPath)}, + WritePath: blocks.candidatePath(*block, resolvedPath), preResolvedPath: structpath.NewPatternStringKey(mergedIndexPath, rename.keyField).String(), }) continue @@ -387,23 +388,23 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes } resolvedPathStr := resolvedPath.String() - var candidates []string - if routed { + writePath := resolvedPathStr + altWritePath := "" + if block != nil { // The block is known, so there is exactly one path to write. - candidates = []string{blocks.candidatePath(block, resolvedPath)} - } else { - candidates = []string{resolvedPathStr} - if targetName != "" { - candidates = append(candidates, targetPrefixedPath(targetName, resolvedPath)) - } + writePath = blocks.candidatePath(*block, resolvedPath) + } else if targetName != "" { + // The scope is unknown, so the override is tried if the top-level + // path turns out not to exist. + altWritePath = targetPrefixedPath(targetName, resolvedPath) } - // A routed change has a known destination file even when the leaf + // A change routed to a block has a known destination file even when the leaf // itself is new, but "defined in the config" must still be decided by // the leaf: a field with no source location is added, not replaced. filePath := resolved.leaf.Location().File isDefinedInConfig := filePath != "" - if routed && isDefinedInConfig { + if block != nil && isDefinedInConfig { filePath = block.file } @@ -423,7 +424,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes configChange.Operation = OperationAdd } - if routed { + if block != nil { // The enclosing element was resolved to a block, so a new field // on it belongs in that same block. filePath = block.file @@ -448,7 +449,8 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes result = append(result, FieldChange{ FilePath: filePath, Change: destChange, - FieldCandidates: candidates, + WritePath: writePath, + AltWritePath: altWritePath, preResolvedPath: mergedIndexPath.String(), }) } @@ -513,3 +515,12 @@ func resolveNotebookExtension(syncRoot fs.FS, relPath string) string { } return relPath } + +// writePaths lists the paths to try, in order: the addressed one, then the fallback +// for a change whose scope had to be guessed. +func (c FieldChange) writePaths() []string { + if c.AltWritePath == "" { + return []string{c.WritePath} + } + return []string{c.WritePath, c.AltWritePath} +} From e803e403e797def211eb364068ecdfb05916ab7e Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Tue, 4 Aug 2026 08:57:53 +0000 Subject: [PATCH 21/28] Move sequence and block routing into sequences.go Co-authored-by: Isaac --- bundle/configsync/rename.go | 33 ++--- bundle/configsync/resolve.go | 136 +++++------------- .../{blockindex.go => sequences.go} | 110 +++++++++++++- bundle/configsync/variables.go | 4 +- 4 files changed, 154 insertions(+), 129 deletions(-) rename bundle/configsync/{blockindex.go => sequences.go} (82%) diff --git a/bundle/configsync/rename.go b/bundle/configsync/rename.go index 2a31117cc6c..a5b3593b68c 100644 --- a/bundle/configsync/rename.go +++ b/bundle/configsync/rename.go @@ -2,7 +2,6 @@ package configsync import ( "errors" - "fmt" "maps" "slices" @@ -34,7 +33,7 @@ type renameSet struct { unpairedPaths map[string]string } -// pairRenames matches removes of keyed elements against adds in the same sequence +// matchRenamingPairs matches removes of keyed elements against adds in the same sequence // that carry the same content apart from the key. // // A remote key change is reported as an unrelated remove plus add, so without @@ -42,7 +41,7 @@ type renameSet struct { // placed somewhere, and for an element defined in several blocks there is no // single right place. Recognising the pair turns it into a key rewrite, which // every defining block can apply to its own part. -func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, changes ResourceChanges) renameSet { +func matchRenamingPairs(b *bundle.Bundle, blocks *blockResolver, resourceKey string, changes ResourceChanges) renameSet { set := renameSet{ byRemovePath: map[string]renamedElement{}, addPaths: map[string]struct{}{}, @@ -305,23 +304,17 @@ func withoutKey(value map[string]any, keyField string) map[string]any { return out } -// routeRenameElement locates the renamed element in every block that defines it. -// The caller turns each destination into a rewrite of the key field, so the -// element's other fields stay where they are and a split element keeps its parts -// in their original scopes. -func routeRenameElement(b *bundle.Bundle, blocks *blockResolver, resourceKey, removePath string) ([]routeDestination, error) { - fullPath := resourceKey + "." + removePath - resolved, err := resolveSelectors(fullPath, b, OperationRemove) - if err != nil { - return nil, fmt.Errorf("failed to resolve selectors in path %s: %w", fullPath, err) - } - +// routeRenameElement locates the renamed element in every block that defines it. The +// caller turns each destination into a rewrite of the key field, so the element's other +// fields stay where they are and a split element keeps its parts in their original +// scopes. +// +// Returns no destinations, and no error, when the element cannot be attributed to a +// block: the rename is left for a later run rather than half-applied. +func routeRenameElement(blocks *blockResolver, resolved resolvedChange) ([]routeDestination, error) { destinations, err := blocks.routeDestinations(resolved) - if err != nil { - if errors.Is(err, errAmbiguousBlock) { - return nil, nil - } - return nil, fmt.Errorf("failed to route rename %s: %w", fullPath, err) + if errors.Is(err, errAmbiguousBlock) { + return nil, nil } - return destinations, nil + return destinations, err } diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index 519b807aa31..cce34cc0d0f 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -28,29 +28,16 @@ type FieldChange struct { // when the change could not be routed to a block, i.e. when the scope had to be // guessed; a routed change has exactly one correct path. AltWritePath string - // preResolvedPath addresses the field in the pre-resolved configuration, the one - // read to recover a ${var.X} reference that deployment replaced with its value. + // originalPath addresses the field in the original configuration, before deployment + // replaced any ${var.X} reference with its value, so restoration can read the + // reference back. // // That configuration is merged, so this path keeps the MERGED sequence indices and - // no targets. prefix. WritePath is the opposite on both counts: it - // addresses the raw file, so its indices are block-local and an override carries - // the prefix. Deriving one from the other reads a different element whenever the - // two index spaces disagree, which restores a sibling's variable reference. - preResolvedPath string -} - -// sequenceStep records a sequence element that a change path navigated through. -// The element's index inside a physical block is only known once the block has -// been chosen, so the merged position is kept until then. -type sequenceStep struct { - // position in the pattern built so far, i.e. how many components precede - // this sequence's index. - component int - // path of the sequence relative to a block, e.g. resources.jobs.j.tasks. - sequencePath dyn.Path - element dyn.Value - // newElement marks an Add whose key is not in the merged sequence yet. - newElement bool + // no targets. prefix. WritePath is the opposite on both counts: it addresses + // the raw file, so its indices are block-local and an override carries the prefix. + // Deriving one from the other reads a different element whenever the two index + // spaces disagree, which restores a sibling's variable reference. + originalPath string } // resolvedChange locates a change in the source YAML. @@ -174,12 +161,6 @@ func pathDepth(pathStr string) int { return len(node.AsSlice()) } -// scopedParent keys index bookkeeping by the sequence a path addresses, within the -// scope of one block, so operations on one block cannot shift indices in another. -func scopedParent(scope string, path *structpath.PatternNode) string { - return scope + path.Parent().String() -} - // adjustArrayIndex adjusts the index in a PatternNode based on previous operations. // When operations are applied sequentially, removals and additions shift array indices. // This function adjusts the index to account for those shifts. @@ -258,15 +239,9 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes // add of the new one, with nothing linking them. Pairing them back up lets a // rename be written as a key rewrite in every block that defines the element, // which keeps a split element split instead of collapsing it into one scope. - renames := pairRenames(b, blocks, resourceKey, resourceChanges) + renames := matchRenamingPairs(b, blocks, resourceKey, resourceChanges) - // Create indices map for this resource, path -> indices, that we could use to replace with added elements - indicesToReplaceMap := make(map[string][]int) - - indexOperations := make(map[string][]struct { - index int - operation OperationType - }) + indices := newIndexTracker() for _, fieldPath := range fieldPaths { configChange := resourceChanges[fieldPath] @@ -289,37 +264,23 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes } resolvedPath := resolved.path - // A sequence element addressed by the merged view has to be mapped onto - // the physical block that defines it, because the merged order and the - // per-block order differ once a sequence is split across blocks. An - // element assembled from several blocks has a part in each, so removing - // or renaming it yields one destination per block. // Variable restoration resolves against the pre-resolved configuration, // which is merged, so it needs the path in merged index space -- captured - // here, before pathWithinBlock rewrites indices to be block-local. + // before routing rewrites indices to be block-local. mergedIndexPath := resolvedPath - destinations := []routeDestination{{path: resolvedPath}} - if blocks != nil && len(resolved.steps) > 0 { - var routeErr error - if isRename { - destinations, routeErr = routeRenameElement(b, blocks, resourceKey, fieldPath) - if routeErr == nil && len(destinations) == 0 { - // The element could not be attributed to a block; leave the - // whole pair for a later run rather than half-applying it. - continue - } - } else { - destinations, routeErr = blocks.routeDestinations(resolved) - } - if routeErr != nil { - if errors.Is(routeErr, errAmbiguousBlock) { - // Applying this change would mean guessing a location. - // Leave it unapplied; a later run can pick it up. - log.Debugf(ctx, "config-remote-sync: skipping %s: %v", fullPath, routeErr) - continue - } - return nil, fmt.Errorf("failed to route change %s: %w", fullPath, routeErr) + + destinations, err := routeChange(blocks, resolved, isRename) + if err != nil { + if !errors.Is(err, errAmbiguousBlock) { + return nil, err } + // Applying this change would mean guessing a location. Leave it + // unapplied; a later run can pick it up. + log.Debugf(ctx, "config-remote-sync: skipping %s: %v", fullPath, err) + continue + } + if len(destinations) == 0 { + continue } for _, destination := range destinations { @@ -343,49 +304,20 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes // element itself. The index is still adjusted below, because // removals earlier in the same block shift it. if isRename { - resolvedPath = adjustArrayIndex(resolvedPath, scope, indexOperations) + resolvedPath = adjustArrayIndex(resolvedPath, scope, indices.operations) destChange.Operation = OperationReplace destChange.Value = rename.newKey resolvedPath = structpath.NewPatternStringKey(resolvedPath, rename.keyField) result = append(result, FieldChange{ - FilePath: block.file, - Change: destChange, - WritePath: blocks.candidatePath(*block, resolvedPath), - preResolvedPath: structpath.NewPatternStringKey(mergedIndexPath, rename.keyField).String(), + FilePath: block.file, + Change: destChange, + WritePath: blocks.candidatePath(*block, resolvedPath), + originalPath: structpath.NewPatternStringKey(mergedIndexPath, rename.keyField).String(), }) continue } - // If the element is removed, we can use the index to replace it with added element - // That may improve the diff in cases when the task is recreated because of renaming - if destChange.Operation == OperationRemove { - freeIndex, ok := resolvedPath.Index() - if ok { - parentPath := scopedParent(scope, resolvedPath) - indicesToReplaceMap[parentPath] = append(indicesToReplaceMap[parentPath], freeIndex) - } - } - - if destChange.Operation == OperationAdd && resolvedPath.BracketStar() { - parentPath := scopedParent(scope, resolvedPath) - indices, ok := indicesToReplaceMap[parentPath] - if ok && len(indices) > 0 { - index := indices[0] - indicesToReplaceMap[parentPath] = indices[1:] - resolvedPath = structpath.NewPatternIndex(resolvedPath.Parent(), index) - } - } - - resolvedPath = adjustArrayIndex(resolvedPath, scope, indexOperations) - - // Track this operation for future index adjustments (only for array element operations) - if originalIndex, ok := resolvedPath.Index(); ok { - parentPath := scopedParent(scope, resolvedPath) - indexOperations[parentPath] = append(indexOperations[parentPath], struct { - index int - operation OperationType - }{originalIndex, destChange.Operation}) - } + resolvedPath = indices.place(scope, resolvedPath, destChange.Operation) resolvedPathStr := resolvedPath.String() writePath := resolvedPathStr @@ -447,11 +379,11 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes } result = append(result, FieldChange{ - FilePath: filePath, - Change: destChange, - WritePath: writePath, - AltWritePath: altWritePath, - preResolvedPath: mergedIndexPath.String(), + FilePath: filePath, + Change: destChange, + WritePath: writePath, + AltWritePath: altWritePath, + originalPath: mergedIndexPath.String(), }) } } diff --git a/bundle/configsync/blockindex.go b/bundle/configsync/sequences.go similarity index 82% rename from bundle/configsync/blockindex.go rename to bundle/configsync/sequences.go index 230fcc2f64f..3b9a7bfc975 100644 --- a/bundle/configsync/blockindex.go +++ b/bundle/configsync/sequences.go @@ -17,16 +17,27 @@ import ( ) // Write-back has to turn a position in the merged configuration into a position in a -// file, and the two do not correspond. +// file, and the two do not correspond. That translation is what this file does. // // A sequence field of a resource may be defined in two physical regions: the top-level // resources... block and the targets..resources... override // block, either of which may live in its own included file. Loading concatenates those -// regions into one sequence and sorts keyed ones by key, so a merged index addresses -// no single region. Every index therefore exists in one of two spaces -- merged, or -// local to one block -- and each value in this file belongs to exactly one of them; -// crossing them silently addresses a different element. +// regions into one sequence and sorts keyed ones by key, so a merged index addresses no +// single region. Every index therefore exists in one of two spaces -- merged, or local to +// one block -- and each value here belongs to exactly one of them; crossing them silently +// addresses a different element. // +// Two questions have to be answered, in order: +// +// - Which block receives the change? Its locations say where it was defined, and an +// element assembled from two blocks has a part in each. +// - Which position inside that block? Not just the block-local index, but the index the +// sequence will have by the time this change is applied, since earlier changes in the +// same run may already have moved it. +// +// Both apply equally to keyed and positional sequences; those differ only in how +// resolveSelectors arrives at a merged position. + // A block is (kind, file), because both kinds may occur in one file and one target's // override may span several files, so neither identifies a block alone. type sourceBlock struct { @@ -349,6 +360,24 @@ func (r *blockResolver) routeDestinations(change resolvedChange) ([]routeDestina return destinations, nil } +// routeChange returns every physical place a change has to be written: one per block +// that defines the addressed element, since an element split across blocks has a part in +// each. +// +// Returns no destinations when the change has to be left for a later run, and +// errAmbiguousBlock when applying it would mean guessing a location. +func routeChange(blocks *blockResolver, resolved resolvedChange, isRename bool) ([]routeDestination, error) { + // Without a resolver, or with no sequence on the path, the resolved path is written + // as-is in whichever file the resource was found in. + if blocks == nil || len(resolved.steps) == 0 { + return []routeDestination{{path: resolved.path}}, nil + } + if isRename { + return routeRenameElement(blocks, resolved) + } + return blocks.routeDestinations(resolved) +} + // addressesWholeElement reports whether the change targets an existing sequence // element itself rather than something inside it. Only such a change can need more // than one destination, since only it can span the blocks the element is built @@ -533,3 +562,74 @@ func (r *blockResolver) pathWithinBlock(block sourceBlock, change resolvedChange } return result, nil } + +// sequenceStep records a sequence element that a change path navigated through. +// The element's index inside a physical block is only known once the block has +// been chosen, so the merged position is kept until then. +type sequenceStep struct { + // component is how many path components precede this sequence's index. + component int + // sequencePath is the sequence itself, e.g. resources.jobs.j.tasks. + sequencePath dyn.Path + element dyn.Value + // newElement marks an Add whose key is not in the merged sequence yet. + newElement bool +} + +// scopedParent keys index bookkeeping by the sequence a path addresses, per block. +func scopedParent(scope string, path *structpath.PatternNode) string { + return scope + path.Parent().String() +} + +// indexTracker keeps the positions already spoken for while a resource's changes are +// applied one after another: removing an element shifts everything after it, and adding +// one can reuse a position a removal just freed. Keyed per block, so operations on one +// block cannot move positions in another. +type indexTracker struct { + // freed holds positions left by a removal, so an addition can reuse one instead of + // appending. That keeps a recreated element in place, which matters for a rename + // that could not be paired. + freed map[string][]int + // operations lets adjustArrayIndex shift a later index past earlier ones. + operations map[string][]struct { + index int + operation OperationType + } +} + +func newIndexTracker() *indexTracker { + return &indexTracker{ + freed: map[string][]int{}, + operations: map[string][]struct { + index int + operation OperationType + }{}, + } +} + +// place accounts for earlier operations in the same block, and records this one for the +// operations that follow. +func (t *indexTracker) place(scope string, path *structpath.PatternNode, operation OperationType) *structpath.PatternNode { + parent := scopedParent(scope, path) + switch { + case operation == OperationRemove: + if freeIndex, ok := path.Index(); ok { + t.freed[parent] = append(t.freed[parent], freeIndex) + } + case operation == OperationAdd && path.BracketStar(): + if reusable := t.freed[parent]; len(reusable) > 0 { + t.freed[parent] = reusable[1:] + path = structpath.NewPatternIndex(path.Parent(), reusable[0]) + } + } + + path = adjustArrayIndex(path, scope, t.operations) + if index, ok := path.Index(); ok { + key := scopedParent(scope, path) + t.operations[key] = append(t.operations[key], struct { + index int + operation OperationType + }{index, operation}) + } + return path +} diff --git a/bundle/configsync/variables.go b/bundle/configsync/variables.go index 92d39a3f26f..d323c337661 100644 --- a/bundle/configsync/variables.go +++ b/bundle/configsync/variables.go @@ -94,13 +94,13 @@ func RestoreVariableReferences(ctx context.Context, b *bundle.Bundle, fieldChang var newValue any switch fc.Change.Operation { case OperationReplace: - fieldValue, ok := preResolvedValueAt(preResolved, fc.preResolvedPath) + fieldValue, ok := preResolvedValueAt(preResolved, fc.originalPath) if !ok { continue } newValue = restoreOriginalRefs(fc.Change.Value, fieldValue, resolved, stats) case OperationAdd: - siblings, ok := sequenceSiblings(preResolved, fc.preResolvedPath) + siblings, ok := sequenceSiblings(preResolved, fc.originalPath) if !ok { continue } From c81059be1f795503c0a96975c2e9c1844e665ca0 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Tue, 4 Aug 2026 09:55:31 +0000 Subject: [PATCH 22/28] Trim duplicated comments in configsync Co-authored-by: Isaac --- bundle/configsync/rename.go | 19 +++++++------------ bundle/configsync/resolve.go | 14 ++++---------- bundle/configsync/sequences.go | 23 +++++++---------------- 3 files changed, 18 insertions(+), 38 deletions(-) diff --git a/bundle/configsync/rename.go b/bundle/configsync/rename.go index a5b3593b68c..98ed1bd0ee9 100644 --- a/bundle/configsync/rename.go +++ b/bundle/configsync/rename.go @@ -127,16 +127,11 @@ func matchRenamingPairs(b *bundle.Bundle, blocks *blockResolver, resourceKey str } // The removal matched no addition. A plain removal is fine: it deletes every - // part of the element, which is what the user asked for. But an addition of - // the same element under a new key, with one of its fields also edited, cannot - // be recognised as a rename: applying the halves separately would delete a - // split element from every block and recreate it in one, collapsing the split - // and moving fields into a scope the user did not choose. Hold both halves - // back in that case only. - // - // "The same element with a field edited" means it still has the same fields; - // a genuinely new element is an unrelated addition that has to be applied, - // even when a removal happens to be in the same run. + // part of the element. But the same element re-added under a new key with one of + // its fields also edited cannot be recognised as a rename: applying the halves + // separately would delete a split element from every block and recreate it in + // one, collapsing the split and moving fields into a scope the user did not + // choose. Hold both halves back in that case. var edited []string for _, add := range matchingAdds(candidate, adds, sameFieldsApartFromKey) { if _, taken := set.addPaths[add.path]; !taken { @@ -278,8 +273,8 @@ func sameElementApartFromKey(oldFields map[string]any, add keyedElement) bool { } // sameFieldsApartFromKey reports whether the added element has the same fields as the -// removed one, ignoring their values. A rename that also edited a field keeps the -// element's shape; an unrelated new element generally does not. +// removed one, ignoring their values, i.e. whether it could be that element with a field +// edited. An unrelated new element generally has a different shape. func sameFieldsApartFromKey(oldFields map[string]any, add keyedElement) bool { newFields, ok := fieldsApartFromKey(add) return ok && slices.Equal(slices.Sorted(maps.Keys(oldFields)), slices.Sorted(maps.Keys(newFields))) diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index cce34cc0d0f..1bd99e9c7aa 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -30,13 +30,9 @@ type FieldChange struct { AltWritePath string // originalPath addresses the field in the original configuration, before deployment // replaced any ${var.X} reference with its value, so restoration can read the - // reference back. - // - // That configuration is merged, so this path keeps the MERGED sequence indices and - // no targets. prefix. WritePath is the opposite on both counts: it addresses - // the raw file, so its indices are block-local and an override carries the prefix. - // Deriving one from the other reads a different element whenever the two index - // spaces disagree, which restores a sibling's variable reference. + // reference back. That configuration is merged, so unlike WritePath this path is in + // merged index space and has no prefix (see sequences.go). Deriving one from the + // other reads a different element and restores a sibling's reference. originalPath string } @@ -264,9 +260,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes } resolvedPath := resolved.path - // Variable restoration resolves against the pre-resolved configuration, - // which is merged, so it needs the path in merged index space -- captured - // before routing rewrites indices to be block-local. + // Captured before routing rewrites indices to be block-local. mergedIndexPath := resolvedPath destinations, err := routeChange(blocks, resolved, isRename) diff --git a/bundle/configsync/sequences.go b/bundle/configsync/sequences.go index 3b9a7bfc975..a4335f7db55 100644 --- a/bundle/configsync/sequences.go +++ b/bundle/configsync/sequences.go @@ -23,9 +23,7 @@ import ( // resources... block and the targets..resources... override // block, either of which may live in its own included file. Loading concatenates those // regions into one sequence and sorts keyed ones by key, so a merged index addresses no -// single region. Every index therefore exists in one of two spaces -- merged, or local to -// one block -- and each value here belongs to exactly one of them; crossing them silently -// addresses a different element. +// single region. Crossing the two index spaces silently addresses a different element. // // Two questions have to be answered, in order: // @@ -110,7 +108,6 @@ func newBlockResolver(ctx context.Context, b *bundle.Bundle) *blockResolver { return r } -// referencedFiles returns the files the merged configuration was loaded from. func referencedFiles(root dyn.Value) map[string]struct{} { files := map[string]struct{}{} _ = dyn.WalkReadOnly(root, func(_ dyn.Path, v dyn.Value) error { @@ -192,16 +189,14 @@ func targetPrefixedPath(target string, path *structpath.PatternNode) string { return prefixed.String() } -// sortedBlocks lists the known blocks with the top-level ones first and a total -// order within each scope, so a choice between blocks never depends on map or -// location iteration order. +// sortedBlocks orders top-level blocks first, so a choice between blocks never depends +// on map iteration order. func (r *blockResolver) sortedBlocks() []sourceBlock { blocks := slices.Collect(maps.Keys(r.blocks)) slices.SortFunc(blocks, compareBlocks) return blocks } -// compareBlocks orders top-level blocks before target blocks, then by file. func compareBlocks(a, b sourceBlock) int { if a.override != b.override { if !a.override { @@ -321,11 +316,9 @@ func (d routeDestination) scopeKey() string { return d.block.scopeKey() } -// routeDestinations returns every physical place a change has to be written: one for an -// edit, since only the definition that wins the merge decides the deployed value, and -// one per definition for a removal, since the value is gone only once every copy is. -// -// Callers only route a change that addresses something inside a sequence. +// routeDestinations returns one destination for an edit, since only the definition that +// wins the merge decides the deployed value, and one per definition for a removal, since +// the value is gone only once every copy is. func (r *blockResolver) routeDestinations(change resolvedChange) ([]routeDestination, error) { // The value whose definitions have to be reached: the element itself when the // change addresses one, otherwise the field being removed. @@ -396,7 +389,6 @@ func addressesWholeElement(change resolvedChange) bool { return len(change.path.AsSlice()) == last.component+1 } -// blockFor picks the block a change belongs to. func (r *blockResolver) blockFor(change resolvedChange) (sourceBlock, error) { // A new element has no source of its own; it is placed relative to the // sequence that receives it. @@ -436,8 +428,7 @@ func (r *blockResolver) blockFor(change resolvedChange) (sourceBlock, error) { return sourceBlock{}, fmt.Errorf("%w: element is defined in %d blocks", errAmbiguousBlock, len(blocks)) } -// declaringBlock picks the block a resource is declared in, which is the top-level -// one whenever there is one. blocksOf and sortedBlocks both order top-level first. +// declaringBlock relies on blocksOf and sortedBlocks ordering top-level first. func declaringBlock(blocks []sourceBlock) sourceBlock { for _, block := range blocks { if !block.override { From 48c6040c99b4b8ef1bedae6ec06f46257af2dd85 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Tue, 4 Aug 2026 10:29:20 +0000 Subject: [PATCH 23/28] Drop blockindex unit tests Co-authored-by: Isaac --- bundle/configsync/blockindex_test.go | 228 --------------------------- 1 file changed, 228 deletions(-) delete mode 100644 bundle/configsync/blockindex_test.go diff --git a/bundle/configsync/blockindex_test.go b/bundle/configsync/blockindex_test.go deleted file mode 100644 index a91dd130ebc..00000000000 --- a/bundle/configsync/blockindex_test.go +++ /dev/null @@ -1,228 +0,0 @@ -package configsync - -import ( - "os" - "path/filepath" - "testing" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/mutator" - "github.com/databricks/cli/bundle/config/mutator/resourcemutator" - "github.com/databricks/cli/libs/logdiag" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// loadBundle loads files the way the sync command does, so a target's overrides are -// merged into the resources tree and keyed sequences are merged by key. Keyed -// sequences merge in the initialize phase, after the target is selected, so that -// mutator has to be applied explicitly here. -func loadBundle(t *testing.T, target string, files map[string]string) (*bundle.Bundle, *blockResolver) { - t.Helper() - dir := t.TempDir() - for name, content := range files { - path := filepath.Join(dir, name) - require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) - require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) - } - - ctx := logdiag.InitContext(t.Context()) - b, err := bundle.Load(ctx, dir) - require.NoError(t, err) - mutator.DefaultMutators(ctx, b) - if target != "" { - bundle.ApplyContext(ctx, b, mutator.SelectTarget(target)) - } - bundle.ApplySeqContext(ctx, b, resourcemutator.MergeJobTasks()) - - blocks := newBlockResolver(ctx, b) - require.NotNil(t, blocks) - return b, blocks -} - -// "shared" is defined in both blocks, and the target contributes a task that sorts -// ahead of it, so the merged order differs from either block's own order. -const splitTasks = `bundle: - name: split - -resources: - jobs: - j: - tasks: - - task_key: shared - max_retries: 1 - depends_on: - - task_key: zzz_top - - task_key: zzz_top - max_retries: 5 - -targets: - dev: - resources: - jobs: - j: - tasks: - - task_key: aaa_dev - max_retries: 6 - - task_key: shared - timeout_seconds: 60 -` - -func TestBlockResolverRoutesFieldToDefiningBlock(t *testing.T) { - b, blocks := loadBundle(t, "dev", map[string]string{"databricks.yml": splitTasks}) - - tests := []struct { - name string - path string - override bool - want string - }{ - { - name: "field defined only top-level", - path: "resources.jobs.j.tasks[task_key='zzz_top'].max_retries", - override: false, - want: "resources.jobs.j.tasks[1].max_retries", - }, - { - name: "field defined only in the target", - path: "resources.jobs.j.tasks[task_key='aaa_dev'].max_retries", - override: true, - want: "resources.jobs.j.tasks[0].max_retries", - }, - { - // The target's value is the deployed one, so writing the top-level copy - // would leave the effective value unchanged. - name: "field of a two-block element routes to its own block", - path: "resources.jobs.j.tasks[task_key='shared'].timeout_seconds", - override: true, - want: "resources.jobs.j.tasks[1].timeout_seconds", - }, - { - // "shared" is merged index 1 but index 0 in the top-level block, so the - // enclosing index has to be translated before the nested lookup. - name: "nested sequence under a shifted element", - path: "resources.jobs.j.tasks[task_key='shared'].depends_on[task_key='zzz_top']", - override: false, - want: "resources.jobs.j.tasks[0].depends_on[0]", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - resolved, err := resolveSelectors(tc.path, b, OperationReplace) - require.NoError(t, err) - - destination, err := blocks.singleDestination(resolved) - require.NoError(t, err) - assert.Equal(t, tc.override, destination.block.override) - assert.Equal(t, tc.want, destination.path.String()) - }) - } -} - -func TestBlockResolverRoutesElementToEveryDefiningBlock(t *testing.T) { - b, blocks := loadBundle(t, "dev", map[string]string{"databricks.yml": splitTasks}) - - // Removing an element defined in both blocks has to reach the part in each, and - // each part has its own index inside its own block. - resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='shared']", b, OperationRemove) - require.NoError(t, err) - - destinations, err := blocks.routeDestinations(resolved) - require.NoError(t, err) - - got := map[bool]string{} - for _, d := range destinations { - got[d.block.override] = d.path.String() - } - assert.Equal(t, map[bool]string{ - false: "resources.jobs.j.tasks[0]", - true: "resources.jobs.j.tasks[1]", - }, got) -} - -func TestBlockResolverPlacesNewNestedElementInParentBlock(t *testing.T) { - b, blocks := loadBundle(t, "dev", map[string]string{"databricks.yml": splitTasks}) - - // A new depends_on entry on a task whose merged index differs from its - // block-local one: finding the receiving sequence needs the enclosing index - // translated first. - resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='shared'].depends_on[task_key='aaa_dev']", b, OperationAdd) - require.NoError(t, err) - - destination, err := blocks.singleDestination(resolved) - require.NoError(t, err) - assert.False(t, destination.block.override) - assert.Equal(t, "resources.jobs.j.tasks[0].depends_on[*]", destination.path.String()) -} - -func TestBlockResolverRefusesElementSpanningBlocks(t *testing.T) { - b, blocks := loadBundle(t, "dev", map[string]string{"databricks.yml": splitTasks}) - - // A field the element does not define anywhere has no location to route by, so - // it is an addition and goes to the block declaring the resource. - resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='shared'].min_retry_interval_millis", b, OperationReplace) - require.NoError(t, err) - - destination, err := blocks.singleDestination(resolved) - require.NoError(t, err) - assert.False(t, destination.block.override) -} - -func TestBlockResolverCountsIndexPerBlockAcrossFiles(t *testing.T) { - // One target's override spread over two included files. Resource keys are unique - // across top-level files but that is not enforced inside targets, so a task - // second in the concatenated target region can be first in the file that defines - // it -- and the write has to use the latter. - b, blocks := loadBundle(t, "dev", map[string]string{ - "databricks.yml": `bundle: - name: split - -include: - - overrides/*.yml - -resources: - jobs: - j: - tasks: - - task_key: base -`, - "overrides/10-first.yml": `targets: - dev: - resources: - jobs: - j: - tasks: - - task_key: from_first - max_retries: 1 -`, - "overrides/20-second.yml": `targets: - dev: - resources: - jobs: - j: - tasks: - - task_key: from_second - max_retries: 2 -`, - }) - - resolved, err := resolveSelectors("resources.jobs.j.tasks[task_key='from_second'].max_retries", b, OperationReplace) - require.NoError(t, err) - - destination, err := blocks.singleDestination(resolved) - require.NoError(t, err) - assert.Equal(t, "20-second.yml", filepath.Base(destination.block.file)) - assert.Equal(t, "resources.jobs.j.tasks[0].max_retries", destination.path.String()) -} - -func TestCompareBlocksOrdersTopLevelFirst(t *testing.T) { - // blocksOf and sortedBlocks both rely on this order so that declaringBlock picks - // the top-level block and callers never depend on map iteration order. - top := sourceBlock{file: "z.yml"} - override := sourceBlock{override: true, file: "a.yml"} - assert.Negative(t, compareBlocks(top, override)) - assert.Positive(t, compareBlocks(override, top)) - assert.Negative(t, compareBlocks(sourceBlock{file: "a.yml"}, sourceBlock{file: "b.yml"})) - assert.Zero(t, compareBlocks(top, top)) -} From e187626bbb143b379357fb7146ce52932639b8a8 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Tue, 4 Aug 2026 10:33:16 +0000 Subject: [PATCH 24/28] Inline the rename routing special case Co-authored-by: Isaac --- bundle/configsync/rename.go | 16 ---------------- bundle/configsync/sequences.go | 10 +++++++--- 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/bundle/configsync/rename.go b/bundle/configsync/rename.go index 98ed1bd0ee9..95783ab77d1 100644 --- a/bundle/configsync/rename.go +++ b/bundle/configsync/rename.go @@ -1,7 +1,6 @@ package configsync import ( - "errors" "maps" "slices" @@ -298,18 +297,3 @@ func withoutKey(value map[string]any, keyField string) map[string]any { } return out } - -// routeRenameElement locates the renamed element in every block that defines it. The -// caller turns each destination into a rewrite of the key field, so the element's other -// fields stay where they are and a split element keeps its parts in their original -// scopes. -// -// Returns no destinations, and no error, when the element cannot be attributed to a -// block: the rename is left for a later run rather than half-applied. -func routeRenameElement(blocks *blockResolver, resolved resolvedChange) ([]routeDestination, error) { - destinations, err := blocks.routeDestinations(resolved) - if errors.Is(err, errAmbiguousBlock) { - return nil, nil - } - return destinations, err -} diff --git a/bundle/configsync/sequences.go b/bundle/configsync/sequences.go index a4335f7db55..8f9f054d722 100644 --- a/bundle/configsync/sequences.go +++ b/bundle/configsync/sequences.go @@ -365,10 +365,14 @@ func routeChange(blocks *blockResolver, resolved resolvedChange, isRename bool) if blocks == nil || len(resolved.steps) == 0 { return []routeDestination{{path: resolved.path}}, nil } - if isRename { - return routeRenameElement(blocks, resolved) + + destinations, err := blocks.routeDestinations(resolved) + if isRename && errors.Is(err, errAmbiguousBlock) { + // A rename is a pair, so an element that cannot be attributed to a block leaves + // both halves for a later run instead of failing the resource. + return nil, nil } - return blocks.routeDestinations(resolved) + return destinations, err } // addressesWholeElement reports whether the change targets an existing sequence From 0138f433892de27440d7774c49f7490a70aed144 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Tue, 4 Aug 2026 10:36:14 +0000 Subject: [PATCH 25/28] Extract rename key write and write-address selection Co-authored-by: Isaac --- bundle/configsync/rename.go | 22 ++++++++++++++++++++++ bundle/configsync/resolve.go | 28 +++------------------------- bundle/configsync/sequences.go | 16 ++++++++++++++++ 3 files changed, 41 insertions(+), 25 deletions(-) diff --git a/bundle/configsync/rename.go b/bundle/configsync/rename.go index 95783ab77d1..76f6ead03b9 100644 --- a/bundle/configsync/rename.go +++ b/bundle/configsync/rename.go @@ -297,3 +297,25 @@ func withoutKey(value map[string]any, keyField string) map[string]any { } return out } + +// renameKeyChange turns a paired rename into a write of just the element's key field, so +// the element's other fields stay where they are and a split element keeps its parts in +// their original scopes. +// +// The index is still adjusted, because a removal earlier in the same block shifts the +// position the key rewrite has to target. +func renameKeyChange(blocks *blockResolver, block sourceBlock, scope string, indices *indexTracker, + blockPath, mergedPath *structpath.PatternNode, rename renamedElement, change *ConfigChangeDesc, +) FieldChange { + change.Operation = OperationReplace + change.Value = rename.newKey + + blockPath = adjustArrayIndex(blockPath, scope, indices.operations) + blockPath = structpath.NewPatternStringKey(blockPath, rename.keyField) + return FieldChange{ + FilePath: block.file, + Change: change, + WritePath: blocks.candidatePath(block, blockPath), + originalPath: structpath.NewPatternStringKey(mergedPath, rename.keyField).String(), + } +} diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index 1bd99e9c7aa..f9d89cbe18b 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -293,37 +293,15 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes // operations on one block cannot move indices in another. scope := destination.scopeKey() - // A rename rewrites only the key field of the element, so it is a - // replace at the element's position rather than a change to the - // element itself. The index is still adjusted below, because - // removals earlier in the same block shift it. if isRename { - resolvedPath = adjustArrayIndex(resolvedPath, scope, indices.operations) - destChange.Operation = OperationReplace - destChange.Value = rename.newKey - resolvedPath = structpath.NewPatternStringKey(resolvedPath, rename.keyField) - result = append(result, FieldChange{ - FilePath: block.file, - Change: destChange, - WritePath: blocks.candidatePath(*block, resolvedPath), - originalPath: structpath.NewPatternStringKey(mergedIndexPath, rename.keyField).String(), - }) + result = append(result, renameKeyChange(blocks, *block, scope, indices, + resolvedPath, mergedIndexPath, rename, destChange)) continue } resolvedPath = indices.place(scope, resolvedPath, destChange.Operation) - resolvedPathStr := resolvedPath.String() - writePath := resolvedPathStr - altWritePath := "" - if block != nil { - // The block is known, so there is exactly one path to write. - writePath = blocks.candidatePath(*block, resolvedPath) - } else if targetName != "" { - // The scope is unknown, so the override is tried if the top-level - // path turns out not to exist. - altWritePath = targetPrefixedPath(targetName, resolvedPath) - } + writePath, altWritePath := writeAddress(blocks, block, targetName, resolvedPath) // A change routed to a block has a known destination file even when the leaf // itself is new, but "defined in the config" must still be decided by diff --git a/bundle/configsync/sequences.go b/bundle/configsync/sequences.go index 8f9f054d722..05ffa2edd48 100644 --- a/bundle/configsync/sequences.go +++ b/bundle/configsync/sequences.go @@ -375,6 +375,22 @@ func routeChange(blocks *blockResolver, resolved resolvedChange, isRename bool) return destinations, err } +// writeAddress renders the path to write, plus a fallback to try when the first does not +// exist in the file. +// +// A change routed to a block has exactly one correct path, so it needs no fallback. One +// that was not routed has a guessed scope: the top-level path is tried first and the +// target override second. +func writeAddress(blocks *blockResolver, block *sourceBlock, target string, path *structpath.PatternNode) (writePath, altWritePath string) { + if block != nil { + return blocks.candidatePath(*block, path), "" + } + if target == "" { + return path.String(), "" + } + return path.String(), targetPrefixedPath(target, path) +} + // addressesWholeElement reports whether the change targets an existing sequence // element itself rather than something inside it. Only such a change can need more // than one destination, since only it can span the blocks the element is built From 9d10f4e02c6858956334ce9cba0cb9dfbec3a4c8 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Tue, 4 Aug 2026 10:40:34 +0000 Subject: [PATCH 26/28] Shorten hold-back reasons and collapse duplicated bookkeeping Co-authored-by: Isaac --- bundle/configsync/rename.go | 64 ++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/bundle/configsync/rename.go b/bundle/configsync/rename.go index 76f6ead03b9..916f37ef0c3 100644 --- a/bundle/configsync/rename.go +++ b/bundle/configsync/rename.go @@ -92,8 +92,8 @@ func matchRenamingPairs(b *bundle.Bundle, blocks *blockResolver, resourceKey str for i, candidate := range candidates { // A pair is only unambiguous when the choice is forced from both sides. One // removal matching several additions is the obvious case, but so is several - // removals matching the same addition: pairing the first and deleting the - // rest would move a key into a block the user did not choose. + // removals matching the same addition: pairing the first and deleting the rest + // would move a key into a block the user did not choose. if len(matches[i]) == 1 && len(matchedBy[matches[i][0].path]) == 1 { add := matches[i][0] set.byRemovePath[candidate.remove.path] = renamedElement{ @@ -107,20 +107,15 @@ func matchRenamingPairs(b *bundle.Bundle, blocks *blockResolver, resourceKey str } if len(matches[i]) > 0 { - // The pairing is not forced. Which pairing was intended only matters when - // the elements live in different blocks, because then it decides which - // block each new key is written to. Within one block every pairing writes - // the same set of elements to the same place, so the halves can be applied - // as a plain removal and addition instead of being held back -- otherwise a - // rename of two same-bodied elements is dropped, and anything referring to - // them by key (depends_on) keeps pointing at keys that no longer exist. - if !ambiguityCrossesBlocks(blocks, candidates, matches[i], matchedBy) { - continue - } - const reason = "several elements with the same contents were renamed in one run and they are not in the same block, so the new keys cannot be matched to them" - set.unpairedPaths[candidate.remove.path] = reason - for _, add := range matches[i] { - set.unpairedPaths[add.path] = reason + // Which pairing was intended only matters when the elements live in + // different blocks, because then it decides which block each new key is + // written to. Within one block every pairing writes the same elements to the + // same place, so the halves are applied as a plain removal and addition -- + // holding them back would drop the rename and leave anything referring to + // them by key (depends_on) pointing at keys that no longer exist. + if ambiguityCrossesBlocks(blocks, candidates, matches[i], matchedBy) { + set.holdBack(candidate.remove.path, addPaths(matches[i]), + "several elements with the same contents were renamed at once, in different blocks") } continue } @@ -130,23 +125,17 @@ func matchRenamingPairs(b *bundle.Bundle, blocks *blockResolver, resourceKey str // its fields also edited cannot be recognised as a rename: applying the halves // separately would delete a split element from every block and recreate it in // one, collapsing the split and moving fields into a scope the user did not - // choose. Hold both halves back in that case. - var edited []string + // choose. + var edited []keyedElement for _, add := range matchingAdds(candidate, adds, sameFieldsApartFromKey) { + // An addition already paired with another removal is that rename's half. if _, taken := set.addPaths[add.path]; !taken { - edited = append(edited, add.path) + edited = append(edited, add) } } - if len(edited) == 0 { - continue - } - if !multiBlockElement(blocks, candidate.resolved) { - continue - } - const reason = "a split element cannot be removed and recreated in one run" - set.unpairedPaths[candidate.remove.path] = reason - for _, path := range edited { - set.unpairedPaths[path] = reason + if len(edited) > 0 && multiBlockElement(blocks, candidate.resolved) { + set.holdBack(candidate.remove.path, addPaths(edited), + "a split element cannot be removed and recreated at once") } } return set @@ -212,6 +201,23 @@ func competingRemovals(matches []keyedElement, matchedBy map[string][]int) []int return out } +// holdBack refuses both halves of a suspected key change, so a rename is never applied +// as a removal that lands without its matching addition. +func (s renameSet) holdBack(removePath string, addPaths []string, reason string) { + s.unpairedPaths[removePath] = reason + for _, path := range addPaths { + s.unpairedPaths[path] = reason + } +} + +func addPaths(adds []keyedElement) []string { + paths := make([]string, 0, len(adds)) + for _, add := range adds { + paths = append(paths, add.path) + } + return paths +} + // multiBlockElement reports whether the element the change addresses is assembled // from more than one physical block. func multiBlockElement(blocks *blockResolver, change resolvedChange) bool { From 20434a5258359a08dbcd4be01e2c93ad280085d5 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Tue, 4 Aug 2026 13:13:06 +0000 Subject: [PATCH 27/28] Merge keyed sequences when loading config for variable restoration Co-authored-by: Isaac --- .../variable_file_order/databricks.yml.tmpl | 25 +++++++++++++ .../split/variable_file_order/out.test.toml | 4 +++ .../split/variable_file_order/output.txt | 36 +++++++++++++++++++ .../split/variable_file_order/script | 23 ++++++++++++ bundle/configsync/variables.go | 12 +++++++ 5 files changed, 100 insertions(+) create mode 100644 acceptance/bundle/config-remote-sync/split/variable_file_order/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/variable_file_order/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/variable_file_order/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/variable_file_order/script diff --git a/acceptance/bundle/config-remote-sync/split/variable_file_order/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/split/variable_file_order/databricks.yml.tmpl new file mode 100644 index 00000000000..4d8282db49d --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/variable_file_order/databricks.yml.tmpl @@ -0,0 +1,25 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +# A single block with no target override at all: the tasks are in file order zzz, aaa, +# but loading sorts keyed sequences by key, so aaa is index 0 in the merged view and +# index 1 in the file. Restoring a ${var.X} reference reads the field from a second load +# of the configuration, and that load has to be in the same index space -- otherwise the +# lookup reads zzz and rewrites aaa with whatever it found, destroying the reference. + +variables: + zzz_dir: + default: zzzhome + aaa_dir: + default: aaahome + +resources: + jobs: + file_order_job: + tasks: + - task_key: zzz + notebook_task: + notebook_path: /Users/${var.zzz_dir}/zzz + - task_key: aaa + notebook_task: + notebook_path: /Users/${var.aaa_dir}/aaa diff --git a/acceptance/bundle/config-remote-sync/split/variable_file_order/out.test.toml b/acceptance/bundle/config-remote-sync/split/variable_file_order/out.test.toml new file mode 100644 index 00000000000..4b5914daa2c --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/variable_file_order/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = false +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/split/variable_file_order/output.txt b/acceptance/bundle/config-remote-sync/split/variable_file_order/output.txt new file mode 100644 index 00000000000..691c2fa1c90 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/variable_file_order/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! + +=== Edit aaa, whose file position differs from its merged position +=== Sync +Detected changes in 1 resource(s): + +Resource: resources.jobs.file_order_job + tasks[task_key='aaa'].notebook_task.notebook_path: replace + + + +=== aaa keeps its own variable reference + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -23,3 +23,3 @@ + - task_key: aaa + notebook_task: +- notebook_path: /Users/${var.aaa_dir}/aaa ++ notebook_path: /Users/${var.aaa_dir}/renamed + +>>> grep -c var.aaa_dir databricks.yml +1 + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.jobs.file_order_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/split/variable_file_order/script b/acceptance/bundle/config-remote-sync/split/variable_file_order/script new file mode 100644 index 00000000000..b2e2b1d236f --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/variable_file_order/script @@ -0,0 +1,23 @@ +#!/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 file_order_job)" + +title "Edit aaa, whose file position differs from its merged position" +edit_resource.py jobs $job_id < Date: Tue, 4 Aug 2026 13:53:04 +0000 Subject: [PATCH 28/28] Fix an assertion that never ran and an unreachable error path Co-authored-by: Isaac --- .../split/rename_two_removes_one_add/output.txt | 14 ++++++++++---- .../split/rename_two_removes_one_add/script | 9 ++++----- bundle/configsync/sequences.go | 6 +++--- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/output.txt b/acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/output.txt index e77155dc0ea..00cea8dd843 100644 --- a/acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/output.txt +++ b/acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/output.txt @@ -19,9 +19,17 @@ Resource: resources.jobs.two_removes_job >>> diff.py databricks.yml.backup databricks.yml -=== Checking task locations in YAML:=== Top-level block (should have task_renamed): +=== Both tasks keep their original keys, and no new key was written +>>> grep -c task_key: task_toplevel databricks.yml +1 ->>> grep -A2 resources: databricks.yml +>>> grep -c task_key: task_target databricks.yml +1 + +>>> grep -c task_key: task_renamed databricks.yml +0 + +Exit code: 1 >>> [CLI] bundle destroy --auto-approve -t dev The following resources will be deleted: @@ -31,5 +39,3 @@ All files and directories at the following location will be deleted: /Workspace/ Deleting files... Destroy complete! - -Exit code: 1 diff --git a/acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/script b/acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/script index f24998bf43a..045fec9922e 100644 --- a/acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/script +++ b/acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/script @@ -30,9 +30,8 @@ title "Neither block is rewritten: the pairing is not forced" echo trace diff.py databricks.yml.backup databricks.yml echo -title "Checking task locations in YAML:" -echo "=== Top-level block (should have task_renamed):" -errcode trace grep -A2 "resources:" databricks.yml | grep -A10 "find_a_job:" -echo "=== Target block (should have nothing, task_target removed):" -errcode trace grep -A20 "targets:" databricks.yml | grep -A5 "dev:" +title "Both tasks keep their original keys, and no new key was written" +errcode trace grep -c "task_key: task_toplevel" databricks.yml +errcode trace grep -c "task_key: task_target" databricks.yml +errcode trace grep -c "task_key: task_renamed" databricks.yml rm databricks.yml.backup diff --git a/bundle/configsync/sequences.go b/bundle/configsync/sequences.go index 05ffa2edd48..d0b6f599c72 100644 --- a/bundle/configsync/sequences.go +++ b/bundle/configsync/sequences.go @@ -443,9 +443,9 @@ func (r *blockResolver) blockFor(change resolvedChange) (sourceBlock, error) { return declaringBlock(blocks), nil } - // The change addresses the element itself. Removing or recreating an element - // built from several blocks cannot be expressed against just one of them. - return sourceBlock{}, fmt.Errorf("%w: element is defined in %d blocks", errAmbiguousBlock, len(blocks)) + // A change addressing the element itself never reaches here: routeDestinations sends + // it to every defining block instead of asking for one. + return declaringBlock(blocks), nil } // declaringBlock relies on blocksOf and sortedBlocks ordering top-level first.