From 490a0da19b18e1fe15d9ff1fb27ee7d46eb64447 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Sun, 2 Aug 2026 19:55:06 +0000 Subject: [PATCH] bundle: remove split config sync elements from every block Co-authored-by: Isaac --- .../split/isolation/databricks.yml.tmpl | 34 +++ .../split/isolation/out.test.toml | 4 + .../split/isolation/output.txt | 63 ++++++ .../config-remote-sync/split/isolation/script | 39 ++++ .../split/keyed_remove/databricks.yml.tmpl | 45 ++++ .../split/keyed_remove/out.test.toml | 4 + .../split/keyed_remove/output.txt | 87 ++++++++ .../split/keyed_remove/script | 54 +++++ bundle/configsync/blockindex.go | 42 ++++ bundle/configsync/resolve.go | 203 ++++++++++-------- 10 files changed, 486 insertions(+), 89 deletions(-) create mode 100644 acceptance/bundle/config-remote-sync/split/isolation/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/isolation/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/isolation/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/isolation/script create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_remove/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_remove/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_remove/output.txt create mode 100644 acceptance/bundle/config-remote-sync/split/keyed_remove/script diff --git a/acceptance/bundle/config-remote-sync/split/isolation/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/split/isolation/databricks.yml.tmpl new file mode 100644 index 00000000000..c5bc72f02f8 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/isolation/databricks.yml.tmpl @@ -0,0 +1,34 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +# job_a's task list is split across both blocks and one task, "shared", is defined +# in both. job_b is an ordinary single-block job. +# +# The point of the fixture: an unrelated resource's change must still be applied +# in the same run as a structural change to a split element. The sync is +# unattended, so one harder change must never stop the rest. +resources: + jobs: + job_a: + tasks: + - task_key: shared + max_retries: 1 + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/shared + + job_b: + max_concurrent_runs: 1 + tasks: + - task_key: simple + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/simple + +targets: + dev: + mode: development + resources: + jobs: + job_a: + tasks: + - task_key: shared + timeout_seconds: 45 diff --git a/acceptance/bundle/config-remote-sync/split/isolation/out.test.toml b/acceptance/bundle/config-remote-sync/split/isolation/out.test.toml new file mode 100644 index 00000000000..4b5914daa2c --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/isolation/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = false +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/split/isolation/output.txt b/acceptance/bundle/config-remote-sync/split/isolation/output.txt new file mode 100644 index 00000000000..400aa2c152f --- /dev/null +++ b/acceptance/bundle/config-remote-sync/split/isolation/output.txt @@ -0,0 +1,63 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Rename the two-block task on job_a, and edit job_b in the same run +=== Sync +Detected changes in 2 resource(s): + +Resource: resources.jobs.job_a + tasks[task_key='shared']: remove + tasks[task_key='shared_renamed']: add + +Resource: resources.jobs.job_b + max_concurrent_runs: replace + + + +=== job_b is updated, and job_a's rename is written in both blocks + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -12,11 +12,11 @@ + job_a: + tasks: +- - task_key: shared +- max_retries: 1 ++ - max_retries: 1 + notebook_task: +- notebook_path: /Users/{{workspace_user_name}}/shared +- ++ notebook_path: '/Users/{{workspace_user_name}}/shared' ++ task_key: shared_renamed ++ timeout_seconds: 45 + job_b: +- max_concurrent_runs: 1 ++ max_concurrent_runs: 6 + tasks: + - task_key: simple +@@ -30,5 +30,3 @@ + jobs: + job_a: +- tasks: +- - task_key: shared +- timeout_seconds: 45 ++ tasks: [] + +>>> grep -c max_concurrent_runs: 6 databricks.yml +1 + +>>> grep -c task_key: shared_renamed databricks.yml +1 + +>>> [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 +@@ -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 < last.component+1 { + block, path, err := r.route(change) + if err != nil { + return nil, err + } + return []routeDestination{{block: block, path: path}}, nil + } + + blocks := r.blocksOf(last.element) + if len(blocks) == 0 { + return nil, fmt.Errorf("%w: no source location for the addressed element", errAmbiguousBlock) + } + + destinations := make([]routeDestination, 0, len(blocks)) + for _, block := range blocks { + path, err := r.localize(block, change) + if err != nil { + return nil, err + } + destinations = append(destinations, routeDestination{block: block, path: path}) + } + return destinations, nil +} + // blockFor picks the block a change belongs to. func (r *blockResolver) blockFor(change resolvedChange) (sourceBlock, error) { // A new element has no source of its own; it is placed relative to the diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index 38814f76189..fa3bc6c8bc8 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -247,12 +247,21 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes // A sequence element addressed by the merged view has to be mapped onto // the physical block that defines it, because the merged order and the - // per-block order differ once a sequence is split across blocks. - var block sourceBlock + // per-block order differ once a sequence is split across blocks. An + // element assembled from several blocks has a part in each, so removing + // it yields one destination per block. + destinations := []routeDestination{{path: resolvedPath}} routed := false if blocks != nil && len(resolved.steps) > 0 { var routeErr error - block, resolvedPath, routeErr = blocks.route(resolved) + if configChange.Operation == OperationRemove { + destinations, routeErr = blocks.routeElement(resolved) + } else { + var block sourceBlock + var path *structpath.PatternNode + block, path, routeErr = blocks.route(resolved) + destinations = []routeDestination{{block: block, path: path}} + } if routeErr != nil { if errors.Is(routeErr, errAmbiguousBlock) { // Applying this change would mean guessing a location. @@ -265,110 +274,126 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes routed = true } - // Index bookkeeping is scoped to a block so that shifts caused by - // operations on one block cannot move indices in another. - scope := "" - if routed { - scope = block.prefix + "\x00" + block.file + "\x00" - } - - // 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 := scope + resolvedPath.Parent().String() - indicesToReplaceMap[parentPath] = append(indicesToReplaceMap[parentPath], freeIndex) + 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, } - } - if configChange.Operation == OperationAdd && resolvedPath.BracketStar() { - parentPath := scope + resolvedPath.Parent().String() - indices, ok := indicesToReplaceMap[parentPath] - if ok && len(indices) > 0 { - index := indices[0] - indicesToReplaceMap[parentPath] = indices[1:] - resolvedPath = structpath.NewPatternIndex(resolvedPath.Parent(), index) + // Index bookkeeping is scoped to a block so that shifts caused by + // operations on one block cannot move indices in another. + scope := "" + if routed { + scope = block.prefix + "\x00" + block.file + "\x00" } - } - - resolvedPath = adjustArrayIndex(resolvedPath, scope, indexOperations) - - // Track this operation for future index adjustments (only for array element operations) - if originalIndex, ok := resolvedPath.Index(); ok { - parentPath := scope + resolvedPath.Parent().String() - indexOperations[parentPath] = append(indexOperations[parentPath], struct { - index int - operation OperationType - }{originalIndex, configChange.Operation}) - } - resolvedPathStr := resolvedPath.String() - var candidates []string - if routed { - // The block is known, so there is exactly one path to write. - if block.prefix == "" { - candidates = []string{resolvedPathStr} - } else { - candidates = []string{block.prefix + "." + resolvedPathStr} - } - } else { - candidates = []string{resolvedPathStr} - if targetName != "" { - targetPrefixedPath := "targets." + targetName + "." + resolvedPathStr - candidates = append(candidates, targetPrefixedPath) + // If the element is removed, we can use the index to replace it with added element + // That may improve the diff in cases when the task is recreated because of renaming + if destChange.Operation == OperationRemove { + freeIndex, ok := resolvedPath.Index() + if ok { + parentPath := scope + resolvedPath.Parent().String() + indicesToReplaceMap[parentPath] = append(indicesToReplaceMap[parentPath], freeIndex) + } } - } - // A routed change has a known destination file even when the leaf - // itself is new, but "defined in the config" must still be decided by - // the leaf: a field with no source location is added, not replaced. - filePath := resolved.leaf.Location().File - isDefinedInConfig := filePath != "" - if routed && isDefinedInConfig { - filePath = block.file - } - - if !isDefinedInConfig { - if configChange.Operation == OperationRemove { - // If the field is not defined in the config and the operation is remove, it is more likely a CLI default - // in this case we skip the change - continue + if destChange.Operation == OperationAdd && resolvedPath.BracketStar() { + parentPath := scope + resolvedPath.Parent().String() + indices, ok := indicesToReplaceMap[parentPath] + if ok && len(indices) > 0 { + index := indices[0] + indicesToReplaceMap[parentPath] = indices[1:] + resolvedPath = structpath.NewPatternIndex(resolvedPath.Parent(), index) + } } - 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 = adjustArrayIndex(resolvedPath, scope, indexOperations) + + // Track this operation for future index adjustments (only for array element operations) + if originalIndex, ok := resolvedPath.Index(); ok { + parentPath := scope + resolvedPath.Parent().String() + indexOperations[parentPath] = append(indexOperations[parentPath], struct { + index int + operation OperationType + }{originalIndex, destChange.Operation}) } + resolvedPathStr := resolvedPath.String() + var candidates []string if routed { - // The enclosing element was resolved to a block, so a new field - // on it belongs in that same block. - filePath = block.file + // The block is known, so there is exactly one path to write. + if block.prefix == "" { + candidates = []string{resolvedPathStr} + } else { + candidates = []string{block.prefix + "." + resolvedPathStr} + } } else { - resourceLocation := b.Config.GetLocation(resourceKey) - filePath = resourceLocation.File + candidates = []string{resolvedPathStr} + if targetName != "" { + targetPrefixedPath := "targets." + targetName + "." + resolvedPathStr + candidates = append(candidates, targetPrefixedPath) + } } - if filePath == "" { - return nil, fmt.Errorf("failed to find location for resource %s for a field %s", resourceKey, fieldPath) + + // A routed change has a known destination file even when the leaf + // itself is new, but "defined in the config" must still be decided by + // the leaf: a field with no source location is added, not replaced. + filePath := resolved.leaf.Location().File + isDefinedInConfig := filePath != "" + if routed && isDefinedInConfig { + filePath = block.file } - log.Debugf(ctx, "Field %s has no location, using %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 (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 == OperationReplace { + // If the field is not defined in the config and the operation is replace, it is more likely a CLI default + // in this case we add it explicitly to the resource location. + // The reclassification is also recorded on the shared change so + // the command's output reports what was actually written. + destChange.Operation = OperationAdd + configChange.Operation = OperationAdd + } + + if routed { + // The enclosing element was resolved to a block, so a new field + // on it belongs in that same block. + filePath = block.file + } else { + resourceLocation := b.Config.GetLocation(resourceKey) + filePath = resourceLocation.File + } + if filePath == "" { + return nil, fmt.Errorf("failed to find location for resource %s for a field %s", resourceKey, fieldPath) + } + + log.Debugf(ctx, "Field %s has no location, using %s", fullPath, filePath) } - } - result = append(result, FieldChange{ - FilePath: filePath, - Change: configChange, - FieldCandidates: candidates, - }) + 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: destChange, + FieldCandidates: candidates, + }) + } } }