diff --git a/acceptance/bundle/config-remote-sync/null_parent/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/null_parent/databricks.yml.tmpl new file mode 100644 index 00000000000..0d083a722be --- /dev/null +++ b/acceptance/bundle/config-remote-sync/null_parent/databricks.yml.tmpl @@ -0,0 +1,45 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +resources: + jobs: + my_job: + name: test-job-$UNIQUE_NAME + job_clusters: + - job_cluster_key: shared + new_cluster: + spark_version: $DEFAULT_SPARK_VERSION + node_type_id: $NODE_TYPE_ID + custom_tags: # Keep this comment when adding tags. + spark_env_vars: null # Keep the environment comment. + init_scripts: ~ + autoscale: ~ # Keep the autoscale comment. + - job_cluster_key: nested + new_cluster: null # Keep the cluster comment through both tag additions. + tasks: + - task_key: main + job_cluster_key: shared + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/notebook + +targets: + default: + resources: + jobs: + my_job: + job_clusters: + - job_cluster_key: shared + new_cluster: + custom_tags: + existing: preserved + spark_env_vars: + EXISTING: preserved + autoscale: + min_workers: 1 + - job_cluster_key: nested + new_cluster: + spark_version: $DEFAULT_SPARK_VERSION + node_type_id: $NODE_TYPE_ID + num_workers: 1 + custom_tags: + existing: preserved diff --git a/acceptance/bundle/config-remote-sync/null_parent/out.test.toml b/acceptance/bundle/config-remote-sync/null_parent/out.test.toml new file mode 100644 index 00000000000..8c2cb31de53 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/null_parent/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +GOOS.windows = false +EnvMatrix.DMS = ["", "true"] diff --git a/acceptance/bundle/config-remote-sync/null_parent/output.txt b/acceptance/bundle/config-remote-sync/null_parent/output.txt new file mode 100644 index 00000000000..a4c0786e1a1 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/null_parent/output.txt @@ -0,0 +1,59 @@ + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Created jobs.my_job +Files: 2 uploaded, 0 deleted +Resources: 1 created, 0 changed, 0 deleted, 0 unchanged + +>>> [CLI] bundle config-remote-sync --save +Detected changes in 1 resource(s): + +Resource: resources.jobs.my_job + job_clusters[job_cluster_key='nested'].new_cluster.custom_tags['123']: add + job_clusters[job_cluster_key='nested'].new_cluster.custom_tags['team']: add + job_clusters[job_cluster_key='shared'].new_cluster.autoscale.max_workers: add + job_clusters[job_cluster_key='shared'].new_cluster.custom_tags['123']: add + job_clusters[job_cluster_key='shared'].new_cluster.custom_tags['team']: add + job_clusters[job_cluster_key='shared'].new_cluster.init_scripts: replace + job_clusters[job_cluster_key='shared'].new_cluster.spark_env_vars['ENV']: add + + + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -12,9 +12,18 @@ + node_type_id: [NODE_TYPE_ID] + custom_tags: # Keep this comment when adding tags. +- spark_env_vars: null # Keep the environment comment. +- init_scripts: ~ +- autoscale: ~ # Keep the autoscale comment. ++ "123": numeric key ++ team: analytics ++ spark_env_vars: # Keep the environment comment. ++ ENV: dev ++ init_scripts: ++ - workspace: ++ destination: /Workspace/init.sh ++ autoscale: # Keep the autoscale comment. ++ max_workers: 2 + - job_cluster_key: nested +- new_cluster: null # Keep the cluster comment through both tag additions. ++ new_cluster: # Keep the cluster comment through both tag additions. ++ custom_tags: ++ "123": numeric key ++ team: analytics + tasks: + - task_key: main + +>>> [CLI] bundle config-remote-sync --save +No changes detected. + + +>>> [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 + +Destroy: 1 deleted diff --git a/acceptance/bundle/config-remote-sync/null_parent/script b/acceptance/bundle/config-remote-sync/null_parent/script new file mode 100644 index 00000000000..b5fd36fb4c5 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/null_parent/script @@ -0,0 +1,27 @@ +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve +} +trap cleanup EXIT + +trace $CLI bundle deploy +job_id=$(read_id.py my_job) + +# Target overrides supply existing keys; new keys route to the null parents in the base config. +edit_resource.py jobs "$job_id" <<'EOF' +for jc in r["job_clusters"]: + cluster = jc["new_cluster"] + cluster["custom_tags"].update({"team": "analytics", "123": "numeric key"}) + if jc["job_cluster_key"] == "shared": + cluster["spark_env_vars"]["ENV"] = "dev" + cluster["init_scripts"] = [{"workspace": {"destination": "/Workspace/init.sh"}}] + cluster["autoscale"]["max_workers"] = 2 +EOF + +cp databricks.yml databricks.yml.backup +trace $CLI bundle config-remote-sync --save +trace diff.py databricks.yml.backup databricks.yml +rm databricks.yml.backup + +trace $CLI bundle config-remote-sync --save diff --git a/acceptance/bundle/config-remote-sync/null_parent/test.toml b/acceptance/bundle/config-remote-sync/null_parent/test.toml new file mode 100644 index 00000000000..6455d92ed8d --- /dev/null +++ b/acceptance/bundle/config-remote-sync/null_parent/test.toml @@ -0,0 +1 @@ +Ignore = [".databricks", "databricks.yml", "databricks.yml.backup"] diff --git a/acceptance/bundle/config-remote-sync/variable_reference_parent/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/variable_reference_parent/databricks.yml.tmpl index 778f4dc71a8..dd53264ad95 100644 --- a/acceptance/bundle/config-remote-sync/variable_reference_parent/databricks.yml.tmpl +++ b/acceptance/bundle/config-remote-sync/variable_reference_parent/databricks.yml.tmpl @@ -7,6 +7,21 @@ variables: default: spark.speculation: "true" + init_scripts: + type: complex + default: + - workspace: + destination: /Workspace/init.sh + new_cluster: + type: complex + default: + spark_version: $DEFAULT_SPARK_VERSION + node_type_id: $NODE_TYPE_ID + num_workers: 1 + custom_tags: + changed: original + removed: original + targets: default: mode: development @@ -22,6 +37,9 @@ resources: node_type_id: $NODE_TYPE_ID num_workers: 1 spark_conf: ${var.spark_conf} + init_scripts: ${var.init_scripts} + - job_cluster_key: variable_cluster + new_cluster: ${var.new_cluster} tasks: - task_key: main job_cluster_key: shared diff --git a/acceptance/bundle/config-remote-sync/variable_reference_parent/output.txt b/acceptance/bundle/config-remote-sync/variable_reference_parent/output.txt index a4eac5ef990..263bb5235b4 100644 --- a/acceptance/bundle/config-remote-sync/variable_reference_parent/output.txt +++ b/acceptance/bundle/config-remote-sync/variable_reference_parent/output.txt @@ -3,15 +3,18 @@ Created jobs.my_job Files: 2 uploaded, 0 deleted Resources: 1 created, 0 changed, 0 deleted, 0 unchanged -=== Inject a spark_conf key under a field defined as a complex variable -spark_conf is set to a variable reference, so a remotely-added key cannot be -written into that scalar node; the change is skipped while unrelated edits still sync. +=== Edit fields under complex variables === Detect and save all changes Detected changes in 1 resource(s): Resource: resources.jobs.my_job + job_clusters[job_cluster_key='shared'].new_cluster.init_scripts[0].workspace.destination: skip job_clusters[job_cluster_key='shared'].new_cluster.spark_conf['spark.executor.memory']: skip + job_clusters[job_cluster_key='variable_cluster'].new_cluster.custom_tags['added']: skip + job_clusters[job_cluster_key='variable_cluster'].new_cluster.custom_tags['changed']: skip + job_clusters[job_cluster_key='variable_cluster'].new_cluster.custom_tags['removed']: skip + job_clusters[job_cluster_key='variable_cluster'].new_cluster.spark_env_vars: skip max_concurrent_runs: replace @@ -21,7 +24,7 @@ Resource: resources.jobs.my_job >>> diff.py databricks.yml.backup databricks.yml --- databricks.yml.backup +++ databricks.yml -@@ -15,5 +15,5 @@ +@@ -30,5 +30,5 @@ jobs: my_job: - max_concurrent_runs: 1 @@ -34,20 +37,22 @@ Resource: resources.jobs.my_job >>> cat [OUTPUT_DIR]/out.requests.txt { "save": true, - "changes_total": 2, - "add_count": 1, - "replace_count": 1, + "changes_total": 7, + "add_count": 3, + "replace_count": 3, + "remove_count": 1, "resource_changes": [ { "resource_type": "jobs", - "changes_count": 2, - "add_count": 1, - "replace_count": 1 + "changes_count": 7, + "add_count": 3, + "replace_count": 3, + "remove_count": 1 } ], "files_changed_count": 1, "files_written_count": 1, - "skipped_changes_count": 1, + "skipped_changes_count": 6, "state_serial": 1, "state_lineage": "[UUID]", "state_source": "local", diff --git a/acceptance/bundle/config-remote-sync/variable_reference_parent/script b/acceptance/bundle/config-remote-sync/variable_reference_parent/script index 00347b093f6..c49921acfe5 100644 --- a/acceptance/bundle/config-remote-sync/variable_reference_parent/script +++ b/acceptance/bundle/config-remote-sync/variable_reference_parent/script @@ -12,15 +12,20 @@ trap cleanup EXIT $CLI bundle deploy job_id="$(read_id.py my_job)" -title "Inject a spark_conf key under a field defined as a complex variable" +title "Edit fields under complex variables" echo -echo "spark_conf is set to a variable reference, so a remotely-added key cannot be" -echo "written into that scalar node; the change is skipped while unrelated edits still sync." edit_resource.py jobs $job_id <<'EOF' r["max_concurrent_runs"] = 5 -for jc in r.get("job_clusters", []): - jc["new_cluster"]["spark_conf"]["spark.executor.memory"] = "4g" +cluster = r["job_clusters"][0]["new_cluster"] +cluster["spark_conf"]["spark.executor.memory"] = "4g" +cluster["init_scripts"][0]["workspace"]["destination"] = "/Workspace/updated-init.sh" + +cluster = r["job_clusters"][1]["new_cluster"] +cluster["spark_env_vars"] = {"ADDED": "value"} +cluster["custom_tags"]["added"] = "new" +cluster["custom_tags"]["changed"] = "updated" +del cluster["custom_tags"]["removed"] EOF title "Detect and save all changes" @@ -36,5 +41,5 @@ rm databricks.yml.backup title "Telemetry" echo # The engine name is dropped because both EnvMatrix variants share this output. -# skipped_changes_count reflects the spark_conf change skipped under a variable reference. +# skipped_changes_count includes changes under both immediate and distant variable ancestors. trace cat "$OUT_REQUESTS" | jq 'select(has("path") and .path == "/telemetry-ext") | .body.protoLogs[] | fromjson | .entry.databricks_cli_log.bundle_config_remote_sync_event | select(. != null) | del(.engine)' diff --git a/bundle/configsync/patch.go b/bundle/configsync/patch.go index 573b5d2c1ba..a350686e1f4 100644 --- a/bundle/configsync/patch.go +++ b/bundle/configsync/patch.go @@ -115,6 +115,16 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( Value: fieldChange.Change.Value, }}) + if patchErr != nil { + patched, err := addThroughNullAncestor(content, fieldPathCandidate, fieldChange.Change.Value) + if err != nil { + return nil, err + } + if patched != nil { + modifiedContent, patchErr = patched, nil + } + } + // Collect parent path errors for later retry if patchErr != nil && isParentPathError(patchErr) { if missingPath, extractErr := extractMissingPath(patchErr); extractErr == nil { @@ -174,6 +184,67 @@ func applyChange(ctx context.Context, content []byte, fieldChange FieldChange) ( return content, nil } +// addThroughNullAncestor applies an add by materializing its null ancestor. +// It returns nil when no null ancestor can be replaced safely. +func addThroughNullAncestor(content []byte, fieldPath string, value any) ([]byte, error) { + var doc yaml.Node + if err := yaml.Unmarshal(content, &doc); err != nil { + return nil, err + } + if len(doc.Content) != 1 { + return nil, nil + } + pattern, err := structpath.ParsePattern(fieldPath) + if err != nil { + return nil, err + } + current := doc.Content[0] + var keyNode *yaml.Node + for depth, part := range pattern.AsSlice() { + // Replacing anchored content could alter aliases or leave dangling references. + if current.Anchor != "" { + return nil, nil + } + if current.Kind == yaml.ScalarNode && current.ShortTag() == "!!null" { + for node := pattern; node.Len() > depth; node = node.Parent() { + key, isKey := node.StringKey() + index, isIndex := node.Index() + switch { + case isKey: + value = map[string]any{key: value} + case node.BracketStar(), isIndex && index == 0: + value = []any{value} + default: + // Only the first element can be added to an empty sequence. + return nil, nil + } + } + var replacement yaml.Node + if err := replacement.Encode(value); err != nil { + return nil, err + } + replacement.HeadComment = current.HeadComment + replacement.FootComment = current.FootComment + // Block containers cannot carry the scalar's inline comment themselves. + if current.LineComment != "" { + if keyNode != nil { + keyNode.LineComment = strings.TrimSpace(keyNode.LineComment + "\n" + current.LineComment) + } else { + replacement.HeadComment = strings.TrimSpace(replacement.HeadComment + "\n" + current.LineComment) + } + } + *current = replacement + return encodeYAML(&doc) + } + + current, keyNode = yamlChild(current, (*structpath.PathNode)(part)) + if current == nil { + return nil, nil + } + } + return nil, nil +} + // isParentPathError checks if error indicates missing parent path. func isParentPathError(err error) bool { if err == nil { @@ -289,15 +360,46 @@ func clearAddedFlowStyle(content []byte, fieldChanges []FieldChange) ([]byte, er clearFlowStyleAlongPath(&doc, candidate) } } + return encodeYAML(&doc) +} + +// encodeYAML uses the same indentation as the YAML patcher. +func encodeYAML(doc *yaml.Node) ([]byte, error) { var buf bytes.Buffer enc := yaml.NewEncoder(&buf) enc.SetIndent(2) - if err := enc.Encode(&doc); err != nil { + if err := enc.Encode(doc); err != nil { return nil, err } return buf.Bytes(), enc.Close() } +// yamlChild returns the child and its mapping key so callers can preserve comments. +// A nil child means the path component cannot be followed. +func yamlChild(current *yaml.Node, part *structpath.PathNode) (child, keyNode *yaml.Node) { + switch current.Kind { + case yaml.MappingNode: + key, ok := part.StringKey() + if !ok { + return nil, nil + } + // current.Content: [key1, val1, key2, val2, ...] + for i := 0; i+1 < len(current.Content); i += 2 { + if current.Content[i].Value == key { + return current.Content[i+1], current.Content[i] + } + } + case yaml.SequenceNode: + index, ok := part.Index() + if ok && index >= 0 && index < len(current.Content) { + return current.Content[index], nil + } + default: + return nil, nil + } + return nil, nil +} + // clearFlowStyleAlongPath navigates the YAML tree along the given structpath, // clearing FlowStyle on every node from root to leaf (inclusive). func clearFlowStyleAlongPath(doc *yaml.Node, pathStr string) { @@ -313,35 +415,10 @@ func clearFlowStyleAlongPath(doc *yaml.Node, pathStr string) { for _, n := range node.AsSlice() { current.Style &^= yaml.FlowStyle - - if key, ok := n.StringKey(); ok { - if current.Kind != yaml.MappingNode { - return - } - found := false - // current.Content: [key1, val1, key2, val2, ...] - for i := 0; i+1 < len(current.Content); i += 2 { - if current.Content[i].Value == key { - current = current.Content[i+1] - found = true - break - } - } - if !found { - return - } - continue + current, _ = yamlChild(current, n) + if current == nil { + return } - - if idx, ok := n.Index(); ok { - if current.Kind != yaml.SequenceNode || idx < 0 || idx >= len(current.Content) { - return - } - current = current.Content[idx] - continue - } - - return } clearFlowStyleNodes(current) diff --git a/bundle/configsync/patch_test.go b/bundle/configsync/patch_test.go index 959a4d37195..080f912fca3 100644 --- a/bundle/configsync/patch_test.go +++ b/bundle/configsync/patch_test.go @@ -438,3 +438,60 @@ func TestBuildNestedMaps(t *testing.T) { } assert.Equal(t, expected, result) } + +func TestApplyChangeScalarParents(t *testing.T) { + for _, tc := range []struct { + name string + content string + path string + want string + }{ + {"string", "parent: text\n", "parent.key", ""}, + {"quoted null", "parent: \"null\"\n", "parent.key", ""}, + {"empty string", "parent: \"\"\n", "parent.key", ""}, + {"boolean", "parent: false\n", "parent.key", ""}, + {"anchored null", "parent: &shared null\nother: *shared\n", "parent.key", ""}, + {"aliased null", "other: &shared null\nparent: *shared\n", "parent.key", ""}, + {"anchored container", "parent: &shared {tags: null}\nother: *shared\n", "parent.tags.key", ""}, + {"nonzero index", "parent: null\n", "parent[1].key", ""}, + {"append", "parent: null\n", "parent[*]", "parent: [value]"}, + {"nested sequence", "parent: ~\n", "parent[0].key", "parent: [{key: value}]"}, + } { + t.Run(tc.name, func(t *testing.T) { + result, err := applyChange(t.Context(), []byte(tc.content), FieldChange{ + WritePath: tc.path, + Change: &ConfigChangeDesc{Operation: OperationAdd, Value: "value"}, + }) + if tc.want == "" { + assert.ErrorContains(t, err, "scalar can not be a container") + return + } + require.NoError(t, err) + assert.YAMLEq(t, tc.want, string(result)) + }) + } +} + +func TestApplyChangeNullParentComments(t *testing.T) { + for _, tc := range []struct { + name string + content string + path string + want string + }{ + {"root", "# head\nnull # inline\n# foot\n", "key", "key: value"}, + {"sequence item", "parent:\n # head\n - null # inline\n # foot\n", "parent[0].key", "parent: [{key: value}]"}, + } { + t.Run(tc.name, func(t *testing.T) { + result, err := applyChange(t.Context(), []byte(tc.content), FieldChange{ + WritePath: tc.path, + Change: &ConfigChangeDesc{Operation: OperationAdd, Value: "value"}, + }) + require.NoError(t, err) + assert.YAMLEq(t, tc.want, string(result)) + for _, comment := range []string{"# head", "# inline", "# foot"} { + assert.Equal(t, 1, strings.Count(string(result), comment), "%s in %s", comment, result) + } + }) + } +} diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index 2dc64b275ef..ddf8482c81b 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -193,13 +193,13 @@ func adjustArrayIndex(path *structpath.PatternNode, scope string, operations map // ResolveChanges resolves selectors and computes field path candidates for each change. // -// A change that cannot be attributed to a single source location, or whose parent +// A change that cannot be attributed to a single source location, or whose ancestor // in the config is written as a variable reference, is left unapplied rather than // written to a guessed or non-existent location. The count of those is returned so // the caller can record it: the command runs unattended, so a change that silently // disappears looks identical to one that was written. preResolved is the merged // config with ${...} references still literal, used to detect variable-reference -// parents. +// ancestors. func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes, preResolved dyn.Value) ([]FieldChange, int, error) { var result []FieldChange skipped := 0 @@ -283,8 +283,8 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes // to place the key or index under. Leave it unapplied, and record the skip on // the shared change so the output reports "skip" rather than a write that never // happens (matching the reclassification below). - if parentIsVariableReference(preResolved, mergedIndexPath.String()) { - log.Debugf(ctx, "config-remote-sync: skipping %s: parent is a variable reference", fullPath) + if ancestorIsVariableReference(preResolved, mergedIndexPath.String()) { + log.Debugf(ctx, "config-remote-sync: skipping %s: ancestor is a variable reference", fullPath) configChange.Operation = OperationSkip skipped++ continue diff --git a/bundle/configsync/variables.go b/bundle/configsync/variables.go index 0701bd217b0..b40fbafbe86 100644 --- a/bundle/configsync/variables.go +++ b/bundle/configsync/variables.go @@ -571,26 +571,25 @@ func preResolvedValueAt(preResolved dyn.Value, fieldPath string) (dyn.Value, boo return v, true } -// parentIsVariableReference reports whether the parent of fieldPath resolves to a +// ancestorIsVariableReference reports whether an ancestor of fieldPath resolves to a // scalar variable reference in the pre-resolved config (e.g. spark_conf: // ${var.spark_conf}). A nested key or index cannot be written into such a scalar, // so config-remote-sync skips the change. fieldPath is a merged-index path, the // same space as FieldChange.originalPath. -func parentIsVariableReference(preResolved dyn.Value, fieldPath string) bool { +func ancestorIsVariableReference(preResolved dyn.Value, fieldPath string) bool { node, err := structpath.ParsePattern(fieldPath) if err != nil { return false } - parent := node.Parent() - if parent == nil { - return false - } - v, ok := preResolvedValueAt(preResolved, parent.String()) - if !ok { - return false + for parent := node.Parent(); parent != nil; parent = parent.Parent() { + v, ok := preResolvedValueAt(preResolved, parent.String()) + if !ok { + continue + } + s, ok := v.AsString() + return ok && dynvar.ContainsVariableReference(s) } - s, ok := v.AsString() - return ok && dynvar.ContainsVariableReference(s) + return false } // sequenceSiblings returns the sibling elements of the parent sequence when