Skip to content

Commit da3374c

Browse files
ilyakuz-dbilyakuz-db
authored
Fix config-remote-sync error when a remote rename is reverted before redeploy (#5251)
## Changes In `convertChangeDesc`, use only the current local config (`cd.New`) to decide whether a path exists "on the config side"; do not include the saved state (`cd.Old`). ## Why When a keyed element (such as a job task) is renamed on the remote workspace, synced into local config via `databricks bundle config-remote-sync --save` *without* an intervening redeploy, and then renamed back to the original key, the next sync would fail with: ``` failed to resolve selectors in path resources.jobs.X.tasks[task_key='Y']: no array element found with task_key='Y' ``` In that state the saved state still held the original key `Y`, the local config held the intermediate key from the first sync, and the remote held `Y` again. The old `hasConfigValue := cd.Old != nil || cd.New != nil` classified the change on the `Y` path as `Replace`, but the YAML no longer contained `Y`, so `resolveSelectors` errored out. With the change, the operation is correctly classified as `Add`, and the existing `indicesToReplaceMap` logic in `ResolveChanges` pairs it with the matching `Remove` to produce a clean rename in the YAML. ## Tests - Adds a table-driven unit test for `convertChangeDesc` covering Add / Remove / Replace / Skip and the regression case. - Adds `acceptance/bundle/config-remote-sync/task_rename_revert` which walks through the full rename-and-revert flow end-to-end.
1 parent caeb2e3 commit da3374c

7 files changed

Lines changed: 227 additions & 1 deletion

File tree

‎acceptance/bundle/config-remote-sync/task_rename_revert/databricks.yml.tmpl‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
bundle:
2+
name: test-bundle-$UNIQUE_NAME
3+
4+
resources:
5+
jobs:
6+
sample_job:
7+
tasks:
8+
- task_key: new_task
9+
notebook_task:
10+
notebook_path: /Users/{{workspace_user_name}}/new_task
11+
new_cluster:
12+
spark_version: $DEFAULT_SPARK_VERSION
13+
node_type_id: $NODE_TYPE_ID
14+
num_workers: 1
15+
16+
targets:
17+
default:
18+
mode: development

‎acceptance/bundle/config-remote-sync/task_rename_revert/out.test.toml‎

Lines changed: 4 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎acceptance/bundle/config-remote-sync/task_rename_revert/output.txt‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files...
2+
Deploying resources...
3+
Updating deployment state...
4+
Deployment complete!
5+
6+
=== Rename task to new_task_2 remotely
7+
8+
=== Sync the rename into config (no redeploy)
9+
Detected changes in 1 resource(s):
10+
11+
Resource: resources.jobs.sample_job
12+
tasks[task_key='new_task']: remove
13+
tasks[task_key='new_task_2']: add
14+
15+
16+
17+
>>> diff.py databricks.yml.backup databricks.yml
18+
--- databricks.yml.backup
19+
+++ databricks.yml
20+
@@ -6,11 +6,11 @@
21+
sample_job:
22+
tasks:
23+
- - task_key: new_task
24+
- notebook_task:
25+
- notebook_path: /Users/{{workspace_user_name}}/new_task
26+
- new_cluster:
27+
- spark_version: 13.3.x-snapshot-scala2.12
28+
+ - new_cluster:
29+
node_type_id: [NODE_TYPE_ID]
30+
num_workers: 1
31+
+ spark_version: 13.3.x-snapshot-scala2.12
32+
+ notebook_task:
33+
+ notebook_path: '/Users/{{workspace_user_name}}/new_task'
34+
+ task_key: new_task_2
35+
36+
targets:
37+
38+
=== Rename task back to new_task remotely
39+
40+
=== Sync the revert into config
41+
Detected changes in 1 resource(s):
42+
43+
Resource: resources.jobs.sample_job
44+
tasks[task_key='new_task']: add
45+
tasks[task_key='new_task_2']: remove
46+
47+
48+
49+
>>> diff.py databricks.yml.backup databricks.yml
50+
--- databricks.yml.backup
51+
+++ databricks.yml
52+
@@ -12,5 +12,5 @@
53+
notebook_task:
54+
notebook_path: '/Users/{{workspace_user_name}}/new_task'
55+
- task_key: new_task_2
56+
+ task_key: new_task
57+
58+
targets:
59+
60+
>>> [CLI] bundle destroy --auto-approve
61+
The following resources will be deleted:
62+
delete resources.jobs.sample_job
63+
64+
All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default
65+
66+
Deleting files...
67+
Destroy complete!

‎acceptance/bundle/config-remote-sync/task_rename_revert/script‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
#!/bin/bash
2+
3+
envsubst < databricks.yml.tmpl > databricks.yml
4+
5+
cleanup() {
6+
trace $CLI bundle destroy --auto-approve
7+
}
8+
trap cleanup EXIT
9+
10+
$CLI bundle deploy
11+
job_id="$(read_id.py sample_job)"
12+
13+
title "Rename task to new_task_2 remotely"
14+
echo
15+
edit_resource.py jobs $job_id <<EOF
16+
for task in r["tasks"]:
17+
if task["task_key"] == "new_task":
18+
task["task_key"] = "new_task_2"
19+
EOF
20+
21+
title "Sync the rename into config (no redeploy)"
22+
echo
23+
cp databricks.yml databricks.yml.backup
24+
$CLI bundle config-remote-sync --save
25+
trace diff.py databricks.yml.backup databricks.yml
26+
rm databricks.yml.backup
27+
28+
title "Rename task back to new_task remotely"
29+
echo
30+
edit_resource.py jobs $job_id <<EOF
31+
for task in r["tasks"]:
32+
if task["task_key"] == "new_task_2":
33+
task["task_key"] = "new_task"
34+
EOF
35+
36+
# Without redeploy, state still holds task_key='new_task' while config holds
37+
# 'new_task_2' from the first sync. Before the fix, sync errored with
38+
# "no array element found with task_key='new_task'" because the change was
39+
# classified as Replace on a key that the YAML no longer contains.
40+
title "Sync the revert into config"
41+
echo
42+
cp databricks.yml databricks.yml.backup
43+
$CLI bundle config-remote-sync --save
44+
trace diff.py databricks.yml.backup databricks.yml
45+
rm databricks.yml.backup

‎acceptance/bundle/config-remote-sync/task_rename_revert/test.toml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
Cloud = true
2+
3+
RecordRequests = false
4+
Ignore = [".databricks", "databricks.yml", "databricks.yml.backup"]
5+
6+
[Env]
7+
DATABRICKS_BUNDLE_ENABLE_EXPERIMENTAL_YAML_SYNC = "true"
8+
9+
[EnvMatrix]
10+
DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"]

‎bundle/configsync/diff.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,12 @@ func filterEntityDefaults(basePath string, value any) any {
8585
}
8686

8787
func convertChangeDesc(path string, cd *deployplan.ChangeDesc) (*ConfigChangeDesc, error) {
88-
hasConfigValue := cd.Old != nil || cd.New != nil
88+
// Use cd.New (current config) to decide whether the field exists "on the config side".
89+
// cd.Old (saved state) must not be considered: when the user has already synced a rename
90+
// locally (cd.New == nil for the old key) but state still holds the prior key, including
91+
// cd.Old in this check would classify the change as Replace and fail later in
92+
// resolveSelectors because the old key no longer exists in the YAML.
93+
hasConfigValue := cd.New != nil
8994
normalizedValue, err := normalizeValue(cd.Remote)
9095
if err != nil {
9196
return nil, fmt.Errorf("failed to normalize remote value: %w", err)

‎bundle/configsync/diff_test.go‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,86 @@ package configsync
33
import (
44
"testing"
55

6+
"github.com/databricks/cli/bundle/deployplan"
67
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
79
)
810

11+
func TestConvertChangeDesc(t *testing.T) {
12+
tests := []struct {
13+
name string
14+
path string
15+
cd *deployplan.ChangeDesc
16+
wantOp OperationType
17+
wantVal any
18+
}{
19+
{
20+
name: "add: new in remote only",
21+
path: "resources.jobs.my_job.description",
22+
cd: &deployplan.ChangeDesc{Old: nil, New: nil, Remote: "remote-desc"},
23+
wantOp: OperationAdd,
24+
wantVal: "remote-desc",
25+
},
26+
{
27+
name: "remove: in config, missing from remote",
28+
path: "resources.jobs.my_job.description",
29+
cd: &deployplan.ChangeDesc{Old: "state-desc", New: "config-desc", Remote: nil},
30+
wantOp: OperationRemove,
31+
wantVal: nil,
32+
},
33+
{
34+
name: "replace: differs between config and remote",
35+
path: "resources.jobs.my_job.description",
36+
cd: &deployplan.ChangeDesc{Old: "state-desc", New: "config-desc", Remote: "remote-desc"},
37+
wantOp: OperationReplace,
38+
wantVal: "remote-desc",
39+
},
40+
{
41+
name: "skip: absent everywhere",
42+
path: "resources.jobs.my_job.description",
43+
cd: &deployplan.ChangeDesc{Old: nil, New: nil, Remote: nil},
44+
wantOp: OperationSkip,
45+
wantVal: nil,
46+
},
47+
// Regression: rename-back-and-forth. State holds the old key (user did not
48+
// redeploy after the first sync), config holds the intermediate key, and
49+
// remote now matches the original. The element at this path is missing from
50+
// config, so the change must be Add — Replace would error in resolveSelectors
51+
// because the keyed element no longer exists in the YAML.
52+
{
53+
name: "add: rename-back path, state has it but config does not",
54+
path: "resources.jobs.my_job.tasks[task_key='new_task']",
55+
cd: &deployplan.ChangeDesc{
56+
Old: map[string]any{"task_key": "new_task"},
57+
New: nil,
58+
Remote: map[string]any{"task_key": "new_task"},
59+
},
60+
wantOp: OperationAdd,
61+
wantVal: map[string]any{"task_key": "new_task"},
62+
},
63+
{
64+
name: "skip: state has it, config and remote do not",
65+
path: "resources.jobs.my_job.tasks[task_key='gone']",
66+
cd: &deployplan.ChangeDesc{
67+
Old: map[string]any{"task_key": "gone"},
68+
New: nil,
69+
Remote: nil,
70+
},
71+
wantOp: OperationSkip,
72+
wantVal: nil,
73+
},
74+
}
75+
76+
for _, tt := range tests {
77+
t.Run(tt.name, func(t *testing.T) {
78+
got, err := convertChangeDesc(tt.path, tt.cd)
79+
require.NoError(t, err)
80+
assert.Equal(t, tt.wantOp, got.Operation)
81+
assert.Equal(t, tt.wantVal, got.Value)
82+
})
83+
}
84+
}
85+
986
func TestStripNamePrefix(t *testing.T) {
1087
tests := []struct {
1188
name string

0 commit comments

Comments
 (0)