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/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 <., 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 <>> 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,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,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: + 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..5f3fdf1ccc5 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/multifile/overrides/10-first.yml.tmpl @@ -0,0 +1,14 @@ +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 + - 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 new file mode 100644 index 00000000000..229f449ebf3 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/multifile/overrides/20-second.yml.tmpl @@ -0,0 +1,13 @@ +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 + - 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 new file mode 100644 index 00000000000..4f82441d08e --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/multifile/script @@ -0,0 +1,92 @@ +#!/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 +@@ -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 <>> 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 <>> 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 +@@ -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 < 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 + + +=== Both tasks keep their original keys, and no new key was written +>>> grep -c task_key: task_toplevel databricks.yml +1 + +>>> 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: + 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! 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..045fec9922e --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/rename_two_removes_one_add/script @@ -0,0 +1,37 @@ +#!/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 "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/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..da9cb6f3e66 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/target_variable/databricks.yml.tmpl @@ -0,0 +1,39 @@ +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: + 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: + mode: development + resources: + jobs: + var_job: + tasks: + - task_key: zzz + notebook_task: + notebook_path: /Users/${var.dev_dir}/zzz 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..9e138def93b --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/target_variable/output.txt @@ -0,0 +1,39 @@ +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='zzz'].notebook_task.notebook_path: replace + + + +=== zzz keeps its own variable, and aaa is untouched + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -37,3 +37,3 @@ + - task_key: zzz + notebook_task: +- notebook_path: /Users/${var.dev_dir}/zzz ++ notebook_path: /Users/${var.dev_dir}/renamed + +>>> grep -c var.dev_dir databricks.yml +1 + +>>> grep -c var.shared_dir 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..da7ff428244 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/target_variable/script @@ -0,0 +1,34 @@ +#!/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. +# 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 <>> 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 <>> 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/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 new file mode 100644 index 00000000000..916f37ef0c3 --- /dev/null +++ b/bundle/configsync/rename.go @@ -0,0 +1,327 @@ +package configsync + +import ( + "maps" + "slices" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/structs/structdiff" + "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 that could not be + // matched up one-to-one. Applying them separately would delete the element from + // every block and recreate it in one, collapsing a split element and moving + // fields into a scope the user did not choose, so neither half is applied. The + // value is the reason, for reporting. + unpairedPaths map[string]string +} + +// 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 +// 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 matchRenamingPairs(b *bundle.Bundle, blocks *blockResolver, resourceKey string, changes ResourceChanges) renameSet { + set := renameSet{ + byRemovePath: map[string]renamedElement{}, + addPaths: map[string]struct{}{}, + unpairedPaths: map[string]string{}, + } + if blocks == nil { + return set + } + + 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 { + 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 + } + candidates = append(candidates, renameCandidate{ + remove: remove, + resolved: resolved, + oldFields: withoutKey(element, remove.keyField), + }) + } + + // 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 { + 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) + } + } + + 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 && len(matchedBy[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[i]) > 0 { + // 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 + } + + // The removal matched no addition. A plain removal is fine: it deletes every + // 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. + 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) + } + } + 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 +} + +// 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 +} + +// 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 out +} + +// 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 _, i := range competingRemovals(matches, matchedBy) { + steps := candidates[i].resolved.steps + if len(steps) == 0 { + return true + } + elementBlocks := blocks.blocksOf(steps[len(steps)-1].element) + if len(elementBlocks) != 1 { + return true + } + if !slices.Contains(seen, elementBlocks[0]) { + seen = append(seen, elementBlocks[0]) + } + } + return len(seen) != 1 +} + +// 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 +} + +// 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 { + if len(change.steps) == 0 { + return false + } + last := change.steps[len(change.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, i.e. a plain rename. oldFields is the removed element without its +// key field. +func sameElementApartFromKey(oldFields map[string]any, add keyedElement) bool { + newFields, ok := fieldsApartFromKey(add) + return ok && structdiff.IsEqual(oldFields, newFields) +} + +// sameFieldsApartFromKey reports whether the added element has the same fields as the +// 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))) +} + +// 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 nil, false + } + return withoutKey(value, add.keyField), true +} + +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 +} + +// 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 01018365e67..f9d89cbe18b 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" @@ -18,28 +19,56 @@ 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 + // 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 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 } -// resolveSelectors converts key-value selectors to numeric indices that match -// the YAML file positions. It also returns the location of the resolved leaf value. +// 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 + // 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 +// 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 +76,30 @@ 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, + // A positional path can address one past the end of the sequence, + // which is how an add to a positionally-diffed list arrives. There is + // no element to route by, so it is placed like any other new element. + newElement: !element.IsValid(), + }) 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 +117,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, operation: operation}, nil } func pathDepth(pathStr string) int { @@ -132,7 +160,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,8 +171,7 @@ func adjustArrayIndex(path *structpath.PatternNode, operations map[string][]stru } parentPath := path.Parent() - parentPathStr := parentPath.String() - ops := operations[parentPathStr] + ops := operations[scopedParent(scope, path)] adjustment := 0 for _, op := range ops { @@ -168,6 +195,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,98 +231,133 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes return cmp.Compare(a, b) }) - // Create indices map for this resource, path -> indices, that we could use to replace with added elements - indicesToReplaceMap := make(map[string][]int) + // 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 := matchRenamingPairs(b, blocks, resourceKey, resourceChanges) - indexOperations := make(map[string][]struct { - index int - operation OperationType - }) + indices := newIndexTracker() for _, fieldPath := range fieldPaths { 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 reason, ok := renames.unpairedPaths[fieldPath]; ok { + log.Debugf(ctx, "config-remote-sync: skipping %s: %s", fullPath, reason) + 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) } + resolvedPath := resolved.path - // 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) - } - } + // Captured before routing rewrites indices to be block-local. + mergedIndexPath := resolvedPath - 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) + 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 } - - resolvedPath = adjustArrayIndex(resolvedPath, indexOperations) - - // 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}) + if len(destinations) == 0 { + continue } - resolvedPathStr := resolvedPath.String() - candidates := []string{resolvedPathStr} - if targetName != "" { - targetPrefixedPath := "targets." + targetName + "." + resolvedPathStr - candidates = append(candidates, targetPrefixedPath) - } + 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, + } - filePath := resolvedLocation.File + // Index bookkeeping is scoped to a block so that shifts caused by + // operations on one block cannot move indices in another. + scope := destination.scopeKey() - 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 + if isRename { + result = append(result, renameKeyChange(blocks, *block, scope, indices, + resolvedPath, mergedIndexPath, rename, destChange)) continue } - 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 - } + resolvedPath = indices.place(scope, resolvedPath, destChange.Operation) + + writePath, altWritePath := writeAddress(blocks, block, targetName, resolvedPath) - 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 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 block != nil && 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 block != nil { + // 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, + WritePath: writePath, + AltWritePath: altWritePath, + originalPath: mergedIndexPath.String(), + }) + } } } @@ -356,3 +419,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} +} 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)) -} diff --git a/bundle/configsync/sequences.go b/bundle/configsync/sequences.go new file mode 100644 index 00000000000..d0b6f599c72 --- /dev/null +++ b/bundle/configsync/sequences.go @@ -0,0 +1,646 @@ +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" +) + +// Write-back has to turn a position in the merged configuration into a position in a +// 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. Crossing the two index spaces 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 { + // 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, 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. + blocks map[sourceBlock]dyn.Value + target string + byLocation map[dyn.Location]sourceBlock +} + +// 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. +// +// 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() { + 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 +} + +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. +// +// 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.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 prefixed.String() +} + +// 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 +} + +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) + } + } + // 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. +// +// Locations accumulate in merge order, so the first one that maps to a block is the +// 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 { + return block, true + } + } + return sourceBlock{}, false +} + +// 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 { + 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{}{} + } + + for local, entry := range entries { + for _, location := range entry.Locations() { + if _, ok := locations[location]; ok { + return local, true + } + } + } + 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. +// +// 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. 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 + path *structpath.PatternNode +} + +// 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 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. + 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 + } + 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) + if err != nil { + return nil, err + } + destinations = append(destinations, routeDestination{block: &block, path: path}) + } + 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 + } + + 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 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. 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 +} + +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) + } + } + + 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 block, ok := r.winningBlock(change.leaf); ok { + return block, nil + } + + // The field has no source location: either it is absent from the config, or a + // mutator inserted it (e.g. OverrideCompute setting existing_cluster_id), which + // builds a fresh value carrying no location. Either way no block declares it, so + // writing it is an addition; it goes to the block declaring the resource rather + // than into a target-specific scope the user did not choose. + if !addressesWholeElement(change) { + return declaringBlock(blocks), nil + } + + // 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. +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(change, 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. +// 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() { + blockPath, ok := r.sequencePathWithinBlock(block, change, sequencePath) + if !ok { + continue + } + sequence, err := dyn.GetByPath(r.blocks[block], r.regionPath(block, blockPath)) + if err != nil { + continue + } + if slices.ContainsFunc(sequence.Locations(), func(l dyn.Location) bool { return l.File == block.file }) { + blocks = append(blocks, block) + } + } + return blocks +} + +// 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. +func (r *blockResolver) sequencePathWithinBlock(block sourceBlock, change resolvedChange, sequencePath dyn.Path) (dyn.Path, bool) { + result := slices.Clone(sequencePath) + for _, step := range change.steps { + at := len(step.sequencePath) + if step.newElement || at >= 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) { + 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 + } + 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) + } + 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 +} + +// 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 055a47dc934..5cc6349331d 100644 --- a/bundle/configsync/variables.go +++ b/bundle/configsync/variables.go @@ -8,6 +8,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" + "github.com/databricks/cli/bundle/config/mutator/resourcemutator" "github.com/databricks/cli/bundle/direct/dstate" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/dyn/dynvar" @@ -94,13 +95,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.originalPath) if !ok { continue } newValue = restoreOriginalRefs(fc.Change.Value, fieldValue, resolved, stats) case OperationAdd: - siblings, ok := sequenceSiblings(preResolved, fc.FieldCandidates) + siblings, ok := sequenceSiblings(preResolved, fc.originalPath) if !ok { continue } @@ -133,6 +134,17 @@ func loadPreResolvedConfig(ctx context.Context, b *bundle.Bundle) dyn.Value { bundle.ApplyContext(ctx, fresh, mutator.SelectTarget(target)) } } + + // Keyed sequences merge in the initialize phase, which this reload skips. Without + // them the sequences here stay in file order while the change paths address the + // merged, key-sorted order, so a lookup would read a different element. + bundle.ApplySeqContext(ctx, fresh, + resourcemutator.MergeJobClusters(), + resourcemutator.MergeJobParameters(), + resourcemutator.MergeJobTasks(), + resourcemutator.MergePipelineClusters(), + resourcemutator.MergeApps(), + ) return fresh.Config.Value() } @@ -556,18 +568,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 + } + v, err := dyn.GetByPath(preResolved, p) + if err != nil { + return dyn.InvalidValue, false } - return dyn.InvalidValue, false + return v, true } // sequenceSiblings returns the sibling elements of the parent sequence when @@ -575,29 +585,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 }