diff --git a/acceptance/bundle/config-remote-sync/task_split_target_index/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/task_split_target_index/databricks.yml.tmpl new file mode 100644 index 00000000000..9581be2a521 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_index/databricks.yml.tmpl @@ -0,0 +1,34 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +targets: + default: + mode: development + resources: + jobs: + my_job: + tasks: + - task_key: aaa_task + run_if: ALL_SUCCESS + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/aaa + +resources: + jobs: + my_job: + tasks: + - task_key: bbb_task + run_if: ALL_SUCCESS + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/bbb + new_cluster: + spark_version: $DEFAULT_SPARK_VERSION + node_type_id: $NODE_TYPE_ID + num_workers: 1 + - task_key: ccc_task + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/ccc + new_cluster: + spark_version: $DEFAULT_SPARK_VERSION + node_type_id: $NODE_TYPE_ID + num_workers: 1 diff --git a/acceptance/bundle/config-remote-sync/task_split_target_index/out.test.toml b/acceptance/bundle/config-remote-sync/task_split_target_index/out.test.toml new file mode 100644 index 00000000000..579b1e4a3c9 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_index/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = true +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/task_split_target_index/output.txt b/acceptance/bundle/config-remote-sync/task_split_target_index/output.txt new file mode 100644 index 00000000000..04b725289ce --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_index/output.txt @@ -0,0 +1,36 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Set run_if on bbb_task remotely (merged task order: aaa,bbb,ccc) + +=== Detect and save changes +Detected changes in 1 resource(s): + +Resource: resources.jobs.my_job + tasks[task_key='bbb_task'].run_if: replace + + + +=== Configuration changes + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -19,5 +19,5 @@ + tasks: + - task_key: bbb_task +- run_if: ALL_SUCCESS ++ run_if: ALL_DONE + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/bbb + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.jobs.my_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/task_split_target_index/script b/acceptance/bundle/config-remote-sync/task_split_target_index/script new file mode 100644 index 00000000000..8ab908ffde1 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_index/script @@ -0,0 +1,37 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve +} +trap cleanup EXIT + +$CLI bundle deploy +job_id="$(read_id.py my_job)" + +# my_job's tasks are split across two config blocks in the SAME file: the +# top-level resources block (bbb_task, ccc_task) and the targets.default +# override block (aaa_task). After MergeJobTasks sorts by task_key the merged +# order is aaa,bbb,ccc. yamlFileIndex must be block-aware: bbb_task's remote +# run_if edit resolves to index 0 of the top-level block (bbb_task), not to the +# whole-file position 1 (ccc_task). +title "Set run_if on bbb_task remotely (merged task order: aaa,bbb,ccc)" +echo +edit_resource.py jobs $job_id <>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -24,5 +24,5 @@ + tasks: + - task_key: aaa_tgt +- run_if: ALL_SUCCESS ++ run_if: ALL_DONE + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/aaa + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.jobs.my_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/task_split_target_local/script b/acceptance/bundle/config-remote-sync/task_split_target_local/script new file mode 100644 index 00000000000..3946293f135 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/task_split_target_local/script @@ -0,0 +1,33 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve +} +trap cleanup EXIT + +$CLI bundle deploy +job_id="$(read_id.py my_job)" + +# Merged+sorted task order is aaa_tgt, zzz_top. aaa_tgt is defined in the +# targets block (target-local index 0); zzz_top in the top-level block +# (top-level index 0) and also has run_if. Editing aaa_tgt's run_if must patch +# the targets block, not zzz_top in the top-level block. +title "Set run_if on aaa_tgt remotely (target-block task)" +echo +edit_resource.py jobs $job_id < "resources.jobs.foo.tasks[1].name" // Returns a PatternNode because for Add operations, [*] may be used as a placeholder for new elements. -func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) (*structpath.PatternNode, dyn.Location, error) { +// +// The returned bool reports whether the resolved element was contributed by a +// target override block. Its numeric index is then local to that block, so only +// the targets..-prefixed candidate is valid (see ResolveChanges). +func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) (*structpath.PatternNode, dyn.Location, bool, error) { node, err := structpath.ParsePath(pathStr) if err != nil { - return nil, dyn.Location{}, fmt.Errorf("failed to parse path %s: %w", pathStr, err) + return nil, dyn.Location{}, false, fmt.Errorf("failed to parse path %s: %w", pathStr, err) } nodes := node.AsSlice() var result *structpath.PatternNode + inOverrideBlock := false currentValue := b.Config.Value() for _, n := range nodes { @@ -57,10 +62,11 @@ func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) // Check for key-value selector: [key='value'] if key, value, ok := n.KeyValue(); ok { if !currentValue.IsValid() || currentValue.Kind() != dyn.KindSequence { - return nil, dyn.Location{}, fmt.Errorf("cannot apply [%s='%s'] selector to non-array value in path %s", key, value, pathStr) + return nil, dyn.Location{}, false, fmt.Errorf("cannot apply [%s='%s'] selector to non-array value in path %s", key, value, pathStr) } seq, _ := currentValue.AsSequence() + seqLocations := currentValue.Locations() foundIndex := -1 for i, elem := range seq { @@ -82,45 +88,88 @@ func resolveSelectors(pathStr string, b *bundle.Bundle, operation OperationType) currentValue = dyn.Value{} continue } - return nil, dyn.Location{}, fmt.Errorf("no array element found with %s='%s' in path %s", key, value, pathStr) + return nil, dyn.Location{}, false, fmt.Errorf("no array element found with %s='%s' in path %s", key, value, pathStr) } // Mutators may reorder sequence elements (e.g., tasks sorted by task_key). // Use location information to determine the original YAML file position. - yamlIndex := yamlFileIndex(seq, foundIndex) + yamlIndex := yamlFileIndex(seq, foundIndex, seqLocations) result = structpath.NewPatternIndex(result, yamlIndex) + // yamlIndex is local to the element's own block; latch so a nested + // single-block selector further down doesn't clear a target match. + inOverrideBlock = inOverrideBlock || elementInOverrideBlock(seq[foundIndex].Location(), seqLocations) currentValue = seq[foundIndex] continue } } - return result, currentValue.Location(), nil + return result, currentValue.Location(), inOverrideBlock, nil +} + +// elementInOverrideBlock reports whether a merged sequence element was contributed +// by a target override block rather than the top-level resources block. Target +// overrides are merged with the top-level block as the merge reference, and merge +// keeps the reference locations first, so seqLocations[0] is always the top-level +// anchor regardless of where each block appears in the file. An element whose +// nearest block anchor is not that top-level anchor came from a target block, so +// its block-local index is only valid under the targets.. prefix. +func elementInOverrideBlock(elemLoc dyn.Location, seqLocations []dyn.Location) bool { + if len(seqLocations) < 2 || elemLoc.File == "" { + return false + } + topAnchor := seqLocations[0] + return elemLoc.File != topAnchor.File || blockAnchor(elemLoc, seqLocations) != topAnchor.Line } // yamlFileIndex determines the original YAML file position of a sequence element. // Mutators may reorder sequence elements (e.g., tasks sorted by task_key), so the -// in-memory index may not match the position in the YAML file. This function uses -// location information to count how many elements from the same file appear before -// the target element, giving the correct index for YAML patching. -func yamlFileIndex(seq []dyn.Value, sortedIndex int) int { +// in-memory index may not match the position in the YAML file. +// +// A single field can hold sequences from more than one config block — e.g. a job's +// tasks defined partly under top-level `resources.jobs..tasks` and partly under +// `targets..resources.jobs..tasks`. MergeJobTasks concatenates and sorts them +// into one in-memory sequence, but each block is patched independently in the YAML. +// seqLocations holds one location per contributing block (the location of each block's +// sequence node); an element belongs to the block whose anchor is the closest one at or +// before it in the same file. Counting only same-block elements with a smaller line +// gives the index within that block, which is what the YAML patch targets. When the +// sequence comes from a single block this reduces to a plain same-file count. +func yamlFileIndex(seq []dyn.Value, sortedIndex int, seqLocations []dyn.Location) int { matchLocation := seq[sortedIndex].Location() if matchLocation.File == "" { return sortedIndex } + matchAnchor := blockAnchor(matchLocation, seqLocations) + yamlIndex := 0 for i, elem := range seq { if i == sortedIndex { continue } loc := elem.Location() - if loc.File == matchLocation.File && loc.Line < matchLocation.Line { + if loc.File == matchLocation.File && loc.Line < matchLocation.Line && blockAnchor(loc, seqLocations) == matchAnchor { yamlIndex++ } } return yamlIndex } +// blockAnchor returns the config block a sequence element belongs to, identified by the +// line of that block's sequence node. Blocks never interleave within a file, so the +// element's block is the anchor with the greatest line at or before the element in the +// same file. Returns 0 when no anchor matches (single unlocated block), which keeps all +// elements in one group. +func blockAnchor(loc dyn.Location, seqLocations []dyn.Location) int { + anchor := 0 + for _, l := range seqLocations { + if l.File == loc.File && l.Line <= loc.Line && l.Line > anchor { + anchor = l.Line + } + } + return anchor +} + func pathDepth(pathStr string) int { node, err := structpath.ParsePath(pathStr) if err != nil { @@ -215,7 +264,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes configChange := resourceChanges[fieldPath] fullPath := resourceKey + "." + fieldPath - resolvedPath, resolvedLocation, err := resolveSelectors(fullPath, b, configChange.Operation) + resolvedPath, resolvedLocation, inOverrideBlock, err := resolveSelectors(fullPath, b, configChange.Operation) if err != nil { return nil, fmt.Errorf("failed to resolve selectors in path %s: %w", fullPath, err) } @@ -252,10 +301,19 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes } resolvedPathStr := resolvedPath.String() - candidates := []string{resolvedPathStr} - if targetName != "" { - targetPrefixedPath := "targets." + targetName + "." + resolvedPathStr - candidates = append(candidates, targetPrefixedPath) + var candidates []string + targetPrefixedPath := "targets." + targetName + "." + resolvedPathStr + switch { + case inOverrideBlock: + // The index is local to the target override block, so only the + // target-prefixed candidate points at the right element. Emitting the + // unprefixed candidate too would let applyChange write the same index + // into the top-level block and silently patch the wrong element. + candidates = []string{targetPrefixedPath} + case targetName != "": + candidates = []string{resolvedPathStr, targetPrefixedPath} + default: + candidates = []string{resolvedPathStr} } filePath := resolvedLocation.File diff --git a/bundle/configsync/resolve_test.go b/bundle/configsync/resolve_test.go index 9264ad7f5dc..e39174abd00 100644 --- a/bundle/configsync/resolve_test.go +++ b/bundle/configsync/resolve_test.go @@ -32,7 +32,7 @@ func TestResolveSelectors_NoSelectors(t *testing.T) { mutator.DefaultMutators(ctx, b) - result, _, err := resolveSelectors("resources.jobs.test_job.name", b, OperationReplace) + result, _, _, err := resolveSelectors("resources.jobs.test_job.name", b, OperationReplace) require.NoError(t, err) assert.Equal(t, "resources.jobs.test_job.name", result.String()) } @@ -57,11 +57,11 @@ func TestResolveSelectors_NumericIndices(t *testing.T) { mutator.DefaultMutators(ctx, b) - result, _, err := resolveSelectors("resources.jobs.test_job.tasks[0].task_key", b, OperationReplace) + result, _, _, err := resolveSelectors("resources.jobs.test_job.tasks[0].task_key", b, OperationReplace) require.NoError(t, err) assert.Equal(t, "resources.jobs.test_job.tasks[0].task_key", result.String()) - result, _, err = resolveSelectors("resources.jobs.test_job.tasks[1].task_key", b, OperationReplace) + result, _, _, err = resolveSelectors("resources.jobs.test_job.tasks[1].task_key", b, OperationReplace) require.NoError(t, err) assert.Equal(t, "resources.jobs.test_job.tasks[1].task_key", result.String()) } @@ -90,11 +90,11 @@ func TestResolveSelectors_KeyValueSelector(t *testing.T) { mutator.DefaultMutators(ctx, b) - result, _, err := resolveSelectors("resources.jobs.test_job.tasks[task_key='main'].notebook_task.notebook_path", b, OperationReplace) + result, _, _, err := resolveSelectors("resources.jobs.test_job.tasks[task_key='main'].notebook_task.notebook_path", b, OperationReplace) require.NoError(t, err) assert.Equal(t, "resources.jobs.test_job.tasks[1].notebook_task.notebook_path", result.String()) - result, _, err = resolveSelectors("resources.jobs.test_job.tasks[task_key='setup'].notebook_task.notebook_path", b, OperationReplace) + result, _, _, err = resolveSelectors("resources.jobs.test_job.tasks[task_key='setup'].notebook_task.notebook_path", b, OperationReplace) require.NoError(t, err) assert.Equal(t, "resources.jobs.test_job.tasks[0].notebook_task.notebook_path", result.String()) } @@ -120,7 +120,7 @@ func TestResolveSelectors_SelectorNotFound(t *testing.T) { mutator.DefaultMutators(ctx, b) - _, _, err = resolveSelectors("resources.jobs.test_job.tasks[task_key='nonexistent'].notebook_task.notebook_path", b, OperationReplace) + _, _, _, err = resolveSelectors("resources.jobs.test_job.tasks[task_key='nonexistent'].notebook_task.notebook_path", b, OperationReplace) require.Error(t, err) assert.Contains(t, err.Error(), "no array element found with task_key='nonexistent'") } @@ -143,7 +143,7 @@ func TestResolveSelectors_SelectorOnNonArray(t *testing.T) { mutator.DefaultMutators(ctx, b) - _, _, err = resolveSelectors("resources.jobs.test_job[task_key='main'].name", b, OperationReplace) + _, _, _, err = resolveSelectors("resources.jobs.test_job[task_key='main'].name", b, OperationReplace) require.Error(t, err) assert.Contains(t, err.Error(), "cannot apply [task_key='main'] selector to non-array value") } @@ -174,7 +174,7 @@ func TestResolveSelectors_NestedSelectors(t *testing.T) { mutator.DefaultMutators(ctx, b) - result, _, err := resolveSelectors("resources.jobs.test_job.tasks[task_key='main'].libraries[0].pypi.package", b, OperationReplace) + result, _, _, err := resolveSelectors("resources.jobs.test_job.tasks[task_key='main'].libraries[0].pypi.package", b, OperationReplace) require.NoError(t, err) assert.Equal(t, "resources.jobs.test_job.tasks[1].libraries[0].pypi.package", result.String()) } @@ -200,7 +200,7 @@ func TestResolveSelectors_WildcardNotSupported(t *testing.T) { mutator.DefaultMutators(ctx, b) - _, _, err = resolveSelectors("resources.jobs.test_job.tasks.*.task_key", b, OperationReplace) + _, _, _, err = resolveSelectors("resources.jobs.test_job.tasks.*.task_key", b, OperationReplace) require.Error(t, err) assert.Contains(t, err.Error(), "wildcards not allowed in path") } @@ -215,11 +215,13 @@ func TestYamlFileIndex(t *testing.T) { dyn.NewValue(nil, []dyn.Location{{File: "a.yml", Line: 30}}), // pipeline_task dyn.NewValue(nil, []dyn.Location{{File: "a.yml", Line: 20}}), // python_wheel_task } + // Single block: the sequence node is anchored at the block's line in the file. + seqLocations := []dyn.Location{{File: "a.yml", Line: 5}} - assert.Equal(t, 3, yamlFileIndex(seq, 0)) // extra: 3 elements before it in YAML - assert.Equal(t, 0, yamlFileIndex(seq, 1)) // notebook_task: first in YAML - assert.Equal(t, 2, yamlFileIndex(seq, 2)) // pipeline_task: 2 elements before it - assert.Equal(t, 1, yamlFileIndex(seq, 3)) // python_wheel_task: 1 element before it + assert.Equal(t, 3, yamlFileIndex(seq, 0, seqLocations)) // extra: 3 elements before it in YAML + assert.Equal(t, 0, yamlFileIndex(seq, 1, seqLocations)) // notebook_task: first in YAML + assert.Equal(t, 2, yamlFileIndex(seq, 2, seqLocations)) // pipeline_task: 2 elements before it + assert.Equal(t, 1, yamlFileIndex(seq, 3, seqLocations)) // python_wheel_task: 1 element before it } func TestYamlFileIndex_MultipleFiles(t *testing.T) { @@ -233,12 +235,14 @@ func TestYamlFileIndex_MultipleFiles(t *testing.T) { dyn.NewValue(nil, []dyn.Location{{File: "b.yml", Line: 5}}), // task_c dyn.NewValue(nil, []dyn.Location{{File: "b.yml", Line: 15}}), // task_d } + // One anchor per file, each above its file's first element. + seqLocations := []dyn.Location{{File: "a.yml", Line: 3}, {File: "b.yml", Line: 2}} // Indices are relative to each file - assert.Equal(t, 0, yamlFileIndex(seq, 0)) // task_a: first in file A - assert.Equal(t, 1, yamlFileIndex(seq, 1)) // task_b: second in file A - assert.Equal(t, 0, yamlFileIndex(seq, 2)) // task_c: first in file B - assert.Equal(t, 1, yamlFileIndex(seq, 3)) // task_d: second in file B + assert.Equal(t, 0, yamlFileIndex(seq, 0, seqLocations)) // task_a: first in file A + assert.Equal(t, 1, yamlFileIndex(seq, 1, seqLocations)) // task_b: second in file A + assert.Equal(t, 0, yamlFileIndex(seq, 2, seqLocations)) // task_c: first in file B + assert.Equal(t, 1, yamlFileIndex(seq, 3, seqLocations)) // task_d: second in file B } func TestYamlFileIndex_NoLocation(t *testing.T) { @@ -246,6 +250,50 @@ func TestYamlFileIndex_NoLocation(t *testing.T) { dyn.NewValue(nil, nil), dyn.NewValue(nil, nil), } - assert.Equal(t, 0, yamlFileIndex(seq, 0)) - assert.Equal(t, 1, yamlFileIndex(seq, 1)) + assert.Equal(t, 0, yamlFileIndex(seq, 0, nil)) + assert.Equal(t, 1, yamlFileIndex(seq, 1, nil)) +} + +func TestYamlFileIndex_SplitBlocksSameFile(t *testing.T) { + // A job's tasks split across two blocks in the SAME file: a targets override + // block (aaa, near the top) and the top-level resources block (bbb, ccc, lower + // down). MergeJobTasks concatenates and sorts by task_key -> [aaa, bbb, ccc]. + // The tasks sequence node carries one anchor per block: top-level tasks at line + // 20, target tasks at line 11 (order matches the real merge, which keeps the + // top-level location first). Each element's index must be relative to its own + // block, not the whole file. + seq := []dyn.Value{ + dyn.NewValue(nil, []dyn.Location{{File: "databricks.yml", Line: 11}}), // aaa (target block) + dyn.NewValue(nil, []dyn.Location{{File: "databricks.yml", Line: 20}}), // bbb (top-level block) + dyn.NewValue(nil, []dyn.Location{{File: "databricks.yml", Line: 28}}), // ccc (top-level block) + } + seqLocations := []dyn.Location{{File: "databricks.yml", Line: 20}, {File: "databricks.yml", Line: 11}} + + assert.Equal(t, 0, yamlFileIndex(seq, 0, seqLocations)) // aaa: first in target block + assert.Equal(t, 0, yamlFileIndex(seq, 1, seqLocations)) // bbb: first in top-level block (NOT 1) + assert.Equal(t, 1, yamlFileIndex(seq, 2, seqLocations)) // ccc: second in top-level block +} + +func TestElementInOverrideBlock(t *testing.T) { + // seqLocations[0] is always the top-level anchor (merge keeps the reference + // block first); seqLocations[1] is the target override anchor. This holds + // regardless of which block appears first in the file. + topFirst := []dyn.Location{{File: "databricks.yml", Line: 6}, {File: "databricks.yml", Line: 20}} + assert.True(t, elementInOverrideBlock(dyn.Location{File: "databricks.yml", Line: 22}, topFirst)) // target block + assert.False(t, elementInOverrideBlock(dyn.Location{File: "databricks.yml", Line: 8}, topFirst)) // top-level block + + // Target block precedes resources in the file: top-level anchor still first. + targetFirst := []dyn.Location{{File: "databricks.yml", Line: 20}, {File: "databricks.yml", Line: 6}} + assert.True(t, elementInOverrideBlock(dyn.Location{File: "databricks.yml", Line: 8}, targetFirst)) // target block + assert.False(t, elementInOverrideBlock(dyn.Location{File: "databricks.yml", Line: 22}, targetFirst)) // top-level block + + // Single block (no override contribution) is never in an override block. + single := []dyn.Location{{File: "databricks.yml", Line: 6}} + assert.False(t, elementInOverrideBlock(dyn.Location{File: "databricks.yml", Line: 8}, single)) + assert.False(t, elementInOverrideBlock(dyn.Location{}, topFirst)) + + // Multi-file split: a top-level block in a.yml and a target block in b.yml. + multiFile := []dyn.Location{{File: "a.yml", Line: 6}, {File: "b.yml", Line: 4}} + assert.True(t, elementInOverrideBlock(dyn.Location{File: "b.yml", Line: 6}, multiFile)) // target block, other file + assert.False(t, elementInOverrideBlock(dyn.Location{File: "a.yml", Line: 8}, multiFile)) // top-level block }