Skip to content

Commit d9a76d3

Browse files
authored
Fix missing locations in job clusters item (#4683)
## Changes <!-- Brief summary of your changes that is easy to understand --> Preserve correct location for sequence items, e.g. `my_job.job_clusters[0]` ## Why In config-remote-sync when when we attempt to update `my_job.job_clusters[0].job_cluster_key` it produces unexpected results because location is not preserved even though the field is in the config ## Tests 1. Unit test for the dyn helper 2. Add this case to the acceptance tests of the config-remote-sync <!-- If your PR needs to be included in the release notes for next release, add a separate entry in NEXT_CHANGELOG.md as part of your PR. -->
1 parent fd9f50e commit d9a76d3

File tree

5 files changed

+68
-3
lines changed

5 files changed

+68
-3
lines changed

acceptance/bundle/config-remote-sync/job_fields/databricks.yml.tmpl

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,18 @@ resources:
2222
environment_version: "3"
2323
dependencies:
2424
- ./*.whl
25+
job_clusters:
26+
- job_cluster_key: test_cluster
27+
new_cluster:
28+
spark_version: $DEFAULT_SPARK_VERSION
29+
node_type_id: $NODE_TYPE_ID
30+
num_workers: 1
2531
tasks:
2632
- task_key: main
33+
notebook_task:
34+
notebook_path: /Users/{{workspace_user_name}}/notebook
35+
job_cluster_key: test_cluster
36+
- task_key: inline_cluster_task
2737
notebook_task:
2838
notebook_path: /Users/{{workspace_user_name}}/notebook
2939
new_cluster:

acceptance/bundle/config-remote-sync/job_fields/output.txt

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,11 @@ Detected changes in 1 resource(s):
1212
Resource: resources.jobs.my_job
1313
email_notifications.no_alert_for_skipped_runs: add
1414
email_notifications.on_failure: add
15+
job_clusters[0].job_cluster_key: replace
1516
parameters: replace
1617
tags['team']: add
18+
tasks[task_key='inline_cluster_task'].new_cluster.num_workers: replace
19+
tasks[task_key='main'].job_cluster_key: replace
1720
trigger.pause_status: add
1821
trigger.periodic: remove
1922
trigger.table_update: add
@@ -52,9 +55,25 @@ Resource: resources.jobs.my_job
5255
+ - samples.nyctaxi.trips
5356
environments:
5457
- environment_key: default
55-
@@ -31,4 +37,6 @@
58+
@@ -24,5 +30,5 @@
59+
- ./*.whl
60+
job_clusters:
61+
- - job_cluster_key: test_cluster
62+
+ - job_cluster_key: test_cluster_renamed
63+
new_cluster:
64+
spark_version: [[DEFAULT_SPARK_VERSION]]
65+
@@ -33,5 +39,5 @@
66+
notebook_task:
67+
notebook_path: /Users/{{workspace_user_name}}/notebook
68+
- job_cluster_key: test_cluster
69+
+ job_cluster_key: test_cluster_renamed
70+
- task_key: inline_cluster_task
71+
notebook_task:
72+
@@ -40,5 +46,7 @@
73+
spark_version: [[DEFAULT_SPARK_VERSION]]
5674
node_type_id: [NODE_TYPE_ID]
57-
num_workers: 1
75+
- num_workers: 1
76+
+ num_workers: 2
5877
+ tags:
5978
+ team: data
6079

acceptance/bundle/config-remote-sync/job_fields/script

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
#!/bin/bash
22

33
envsubst < databricks.yml.tmpl > databricks.yml
4+
add_repl.py "$DEFAULT_SPARK_VERSION" "[DEFAULT_SPARK_VERSION]"
5+
add_repl.py "$NODE_TYPE_ID" "[NODE_TYPE_ID]"
46

57
cleanup() {
68
trace $CLI bundle destroy --auto-approve
@@ -20,6 +22,16 @@ r["email_notifications"]["no_alert_for_skipped_runs"] = True
2022
r["parameters"].append({"name": "region", "default": "us-east-1"})
2123
r["trigger"] = {"pause_status": "UNPAUSED", "table_update": {"table_names": ["samples.nyctaxi.trips"]}}
2224
25+
for cluster in r.get("job_clusters", []):
26+
if cluster.get("job_cluster_key") == "test_cluster":
27+
cluster["job_cluster_key"] = "test_cluster_renamed"
28+
for task in r.get("tasks", []):
29+
if task.get("task_key") == "inline_cluster_task" and "new_cluster" in task:
30+
task["new_cluster"]["num_workers"] = 2
31+
for task in r.get("tasks", []):
32+
if task.get("job_cluster_key") == "test_cluster":
33+
task["job_cluster_key"] = "test_cluster_renamed"
34+
2335
if "tags" not in r:
2436
r["tags"] = {}
2537
r["tags"]["team"] = "data"

libs/dyn/merge/elements_by_key.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,10 @@ func (e elementsByKey) doMap(_ dyn.Path, v dyn.Value, mergeFunc func(a, b dyn.Va
5454
// Gather resulting elements in natural order.
5555
out := make([]dyn.Value, 0, len(keys))
5656
for _, key := range keys {
57-
nv, err := dyn.Set(seen[key], e.key, dyn.V(key))
57+
// Preserve the location from the original key value so that
58+
// downstream code can tell this field was defined in the YAML.
59+
keyLoc := seen[key].Get(e.key).Locations()
60+
nv, err := dyn.Set(seen[key], e.key, dyn.NewValue(key, keyLoc))
5861
if err != nil {
5962
return dyn.InvalidValue, err
6063
}

libs/dyn/merge/elements_by_key_test.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,27 @@ func TestElementByKey(t *testing.T) {
4949
)
5050
}
5151

52+
func TestElementByKeyPreservesLocations(t *testing.T) {
53+
loc := dyn.Location{File: "config.yml", Line: 10, Column: 5}
54+
vin := dyn.V([]dyn.Value{
55+
dyn.V(map[string]dyn.Value{
56+
"key": dyn.NewValue("foo", []dyn.Location{loc}),
57+
"value": dyn.V(42),
58+
}),
59+
})
60+
61+
keyFunc := func(v dyn.Value) string {
62+
return v.MustString()
63+
}
64+
65+
vout, err := dyn.MapByPath(vin, dyn.EmptyPath, ElementsByKey("key", keyFunc))
66+
require.NoError(t, err)
67+
68+
// Verify the key field retains its original location.
69+
keyValue := vout.Index(0).Get("key")
70+
assert.Equal(t, loc, keyValue.Location())
71+
}
72+
5273
func TestElementByKeyWithOverride(t *testing.T) {
5374
vin := dyn.V([]dyn.Value{
5475
dyn.V(map[string]dyn.Value{

0 commit comments

Comments
 (0)