From e1c0382f977dee1084ce5eaa79e730e5cb1ca55a Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 1 Dec 2025 14:40:14 +0100 Subject: [PATCH 1/8] Add test for modifying a map remotely --- .../jobs/remote_add_tag/databricks.yml | 14 ++++ .../out.plan_post_update.direct.json | 68 +++++++++++++++++++ .../out.plan_post_update.direct.txt | 1 + .../out.plan_post_update.terraform.json | 7 ++ .../out.plan_post_update.terraform.txt | 3 + .../jobs/remote_add_tag/out.test.toml | 5 ++ .../resources/jobs/remote_add_tag/output.txt | 51 ++++++++++++++ .../resources/jobs/remote_add_tag/script | 12 ++++ .../resources/jobs/remote_add_tag/test.toml | 2 + 9 files changed, 163 insertions(+) create mode 100644 acceptance/bundle/resources/jobs/remote_add_tag/databricks.yml create mode 100644 acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.json create mode 100644 acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.txt create mode 100644 acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.terraform.json create mode 100644 acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.terraform.txt create mode 100644 acceptance/bundle/resources/jobs/remote_add_tag/out.test.toml create mode 100644 acceptance/bundle/resources/jobs/remote_add_tag/output.txt create mode 100644 acceptance/bundle/resources/jobs/remote_add_tag/script create mode 100644 acceptance/bundle/resources/jobs/remote_add_tag/test.toml diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/databricks.yml b/acceptance/bundle/resources/jobs/remote_add_tag/databricks.yml new file mode 100644 index 0000000000..f2f0d49687 --- /dev/null +++ b/acceptance/bundle/resources/jobs/remote_add_tag/databricks.yml @@ -0,0 +1,14 @@ +resources: + jobs: + foo: + name: "My Wheel Job" + tags: + tag1: tag value + tasks: + - task_key: TestTask + python_wheel_task: + package_name: "my_test_code" + entry_point: "run" + environment_key: test_env + libraries: + - whl: hello.whl diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.json b/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.json new file mode 100644 index 0000000000..bc685779ab --- /dev/null +++ b/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.json @@ -0,0 +1,68 @@ +{ + "plan": { + "resources.jobs.foo": { + "action": "skip", + "remote_state": { + "created_time": [UNIX_TIME_MILLIS], + "creator_user_name": "[USERNAME]", + "job_id": [JOB_ID], + "run_as_user_name": "[USERNAME]", + "settings": { + "deployment": { + "kind": "BUNDLE", + "metadata_file_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/metadata.json" + }, + "edit_mode": "UI_LOCKED", + "email_notifications": {}, + "format": "MULTI_TASK", + "max_concurrent_runs": 1, + "name": "My Wheel Job", + "queue": { + "enabled": true + }, + "tags": { + "new_tag": "new_value", + "tag1": "tag value" + }, + "tasks": [ + { + "environment_key": "test_env", + "libraries": [ + { + "whl": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/artifacts/.internal/hello.whl" + } + ], + "python_wheel_task": { + "entry_point": "run", + "package_name": "my_test_code" + }, + "task_key": "TestTask" + } + ], + "timeout_seconds": 0, + "webhook_notifications": {} + } + }, + "changes": { + "remote": { + "email_notifications": { + "action": "skip", + "reason": "server_side_default" + }, + "tags.new_tag": { + "action": "skip", + "reason": "server_side_default" + }, + "timeout_seconds": { + "action": "skip", + "reason": "server_side_default" + }, + "webhook_notifications": { + "action": "skip", + "reason": "server_side_default" + } + } + } + } + } +} diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.txt b/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.txt new file mode 100644 index 0000000000..c54c9d511c --- /dev/null +++ b/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.txt @@ -0,0 +1 @@ +Plan: 0 to add, 0 to change, 0 to delete, 1 unchanged diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.terraform.json b/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.terraform.json new file mode 100644 index 0000000000..970b7d5539 --- /dev/null +++ b/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.terraform.json @@ -0,0 +1,7 @@ +{ + "plan": { + "resources.jobs.foo": { + "action": "update" + } + } +} diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.terraform.txt b/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.terraform.txt new file mode 100644 index 0000000000..75b4895c34 --- /dev/null +++ b/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.terraform.txt @@ -0,0 +1,3 @@ +update jobs.foo + +Plan: 0 to add, 1 to change, 0 to delete, 0 unchanged diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/out.test.toml b/acceptance/bundle/resources/jobs/remote_add_tag/out.test.toml new file mode 100644 index 0000000000..d560f1de04 --- /dev/null +++ b/acceptance/bundle/resources/jobs/remote_add_tag/out.test.toml @@ -0,0 +1,5 @@ +Local = true +Cloud = false + +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/output.txt b/acceptance/bundle/resources/jobs/remote_add_tag/output.txt new file mode 100644 index 0000000000..53f3116919 --- /dev/null +++ b/acceptance/bundle/resources/jobs/remote_add_tag/output.txt @@ -0,0 +1,51 @@ +create jobs.foo + +Plan: 1 to add, 0 to change, 0 to delete, 0 unchanged +Uploading hello.whl... +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! ++ [CLI] jobs get [JOB_ID] +{ + "created_time":[UNIX_TIME_MILLIS], + "creator_user_name":"[USERNAME]", + "job_id":[JOB_ID], + "run_as_user_name":"[USERNAME]", + "settings": { + "deployment": { + "kind":"BUNDLE", + "metadata_file_path":"/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/metadata.json" + }, + "edit_mode":"UI_LOCKED", + "email_notifications": {}, + "format":"MULTI_TASK", + "max_concurrent_runs":1, + "name":"My Wheel Job", + "queue": { + "enabled":true + }, + "tags": { + "tag1":"tag value" + }, + "tasks": [ + { + "environment_key":"test_env", + "libraries": [ + { + "whl":"/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/artifacts/.internal/hello.whl" + } + ], + "python_wheel_task": { + "entry_point":"run", + "package_name":"my_test_code" + }, + "task_key":"TestTask" + } + ], + "timeout_seconds":0, + "webhook_notifications": {} + } +} + ++ [CLI] jobs reset [JOB_ID] --json '{"job_id": "[JOB_ID]", "new_settings": {"deployment": {"kind": "BUNDLE", "metadata_file_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/metadata.json"}, "edit_mode": "UI_LOCKED", "email_notifications": {}, "format": "MULTI_TASK", "max_concurrent_runs": 1, "name": "My Wheel Job", "queue": {"enabled": true}, "tags": {"tag1": "tag value", "new_tag": "new_value"}, "tasks": [{"environment_key": "test_env", "libraries": [{"whl": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/artifacts/.internal/hello.whl"}], "python_wheel_task": {"entry_point": "run", "package_name": "my_test_code"}, "task_key": "TestTask"}], "timeout_seconds": 0, "webhook_notifications": {}}}' diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/script b/acceptance/bundle/resources/jobs/remote_add_tag/script new file mode 100644 index 0000000000..d5c39d4e71 --- /dev/null +++ b/acceptance/bundle/resources/jobs/remote_add_tag/script @@ -0,0 +1,12 @@ +touch hello.whl +$CLI bundle plan +$CLI bundle deploy +job_id="$(read_id.py jobs foo)" +echo "$job_id:JOB_ID" >> ACC_REPLS + +edit_resource.py -v jobs $job_id < out.plan_post_update.$DATABRICKS_BUNDLE_ENGINE.txt +$CLI bundle plan -o json > out.plan_post_update.$DATABRICKS_BUNDLE_ENGINE.json diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/test.toml b/acceptance/bundle/resources/jobs/remote_add_tag/test.toml new file mode 100644 index 0000000000..d3525bda91 --- /dev/null +++ b/acceptance/bundle/resources/jobs/remote_add_tag/test.toml @@ -0,0 +1,2 @@ +RecordRequests = false +Ignore = [".databricks", "hello.whl"] From 139307272d11a86dadef7c8ead02a10cdc447608 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 1 Dec 2025 14:46:26 +0100 Subject: [PATCH 2/8] add edit_resource.py and util.py --- acceptance/bin/edit_resource.py | 54 +++++++++++++++++++++++++++++++++ acceptance/bin/util.py | 34 +++++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100755 acceptance/bin/edit_resource.py create mode 100644 acceptance/bin/util.py diff --git a/acceptance/bin/edit_resource.py b/acceptance/bin/edit_resource.py new file mode 100755 index 0000000000..15c2c9769b --- /dev/null +++ b/acceptance/bin/edit_resource.py @@ -0,0 +1,54 @@ +#!/usr/bin/env python3 +import sys +import os +import subprocess +import argparse +import json +import pprint + +sys.path.insert(0, os.path.dirname(__file__)) +import util +from util import run_json, run + + +CLI = os.environ["CLI"] + + +class jobs: + + def get(self, job_id): + return run_json([CLI, "jobs", "get", job_id])["settings"] + + def set(self, job_id, value): + payload = {"job_id": job_id, "new_settings": value} + return run([CLI, "jobs", "reset", job_id, "--json", json.dumps(payload)]) + + +def main(): + parser = argparse.ArgumentParser() + parser.add_argument("type") + parser.add_argument("id") + parser.add_argument("-v", "--verbose", action="store_true") + args = parser.parse_args() + + util.VERBOSE = args.verbose + + script = sys.stdin.read() + + klass = globals()[args.type] + instance = klass() + + data = instance.get(args.id) + my_locals = {"r": data} + + try: + exec(script, locals=my_locals) + except Exception: + pprint.pprint(my_locals) + raise + + instance.set(args.id, my_locals["r"]) + + +if __name__ == "__main__": + main() diff --git a/acceptance/bin/util.py b/acceptance/bin/util.py new file mode 100644 index 0000000000..1bd73af084 --- /dev/null +++ b/acceptance/bin/util.py @@ -0,0 +1,34 @@ +import sys +import subprocess +import json +import shlex + + +VERBOSE = False + + +class RunError(Exception): + pass + + +def run_json(cmd): + if VERBOSE: + print("+ " + " ".join([shlex.quote(x) for x in cmd]), file=sys.stderr, flush=True) + result = subprocess.run(cmd, stdout=subprocess.PIPE, encoding="utf-8") + if VERBOSE and result.stdout: + print(result.stdout, flush=True) + if result.returncode != 0: + raise RunError(f"{cmd} failed with code {result.returncode}\n{result.stdout}".strip()) + try: + return json.loads(result.stdout) + except Exception as ex: + raise RunError(f"{cmd} returned non-json: {ex}\n{result.stdout}") + + +def run(cmd): + if VERBOSE: + print("+ " + " ".join([shlex.quote(x) for x in cmd]), file=sys.stderr, flush=True) + result = subprocess.run(cmd) + if result.returncode != 0: + raise RunError(f"{cmd} failed with code {result.returncode}") + return result From 10881a2f7c43df1b485ec175f721d9b6809777c3 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 1 Dec 2025 14:50:53 +0100 Subject: [PATCH 3/8] format --- acceptance/bin/edit_resource.py | 1 - 1 file changed, 1 deletion(-) diff --git a/acceptance/bin/edit_resource.py b/acceptance/bin/edit_resource.py index 15c2c9769b..9a957fa72b 100755 --- a/acceptance/bin/edit_resource.py +++ b/acceptance/bin/edit_resource.py @@ -15,7 +15,6 @@ class jobs: - def get(self, job_id): return run_json([CLI, "jobs", "get", job_id])["settings"] From 43a9bdf2f7ce1cfde3d33305e6e9348c30110ce1 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 1 Dec 2025 15:36:32 +0100 Subject: [PATCH 4/8] clean up --- acceptance/bundle/resources/jobs/remote_add_tag/databricks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/databricks.yml b/acceptance/bundle/resources/jobs/remote_add_tag/databricks.yml index f2f0d49687..878160bf4b 100644 --- a/acceptance/bundle/resources/jobs/remote_add_tag/databricks.yml +++ b/acceptance/bundle/resources/jobs/remote_add_tag/databricks.yml @@ -1,4 +1,4 @@ -resources: +resources: jobs: foo: name: "My Wheel Job" From 20cf2dc5affd5755d8c05be6ede50477fb47908c Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 1 Dec 2025 16:23:58 +0100 Subject: [PATCH 5/8] Record map keys --- .../out.plan_post_update.direct.json | 40 +++++++++- .../out.plan_post_update.direct.txt | 1 - .../out.plan_post_update.terraform.txt | 3 - .../resources/jobs/remote_add_tag/output.txt | 44 +---------- .../resources/jobs/remote_add_tag/script | 4 +- bundle/direct/bundle_plan.go | 5 +- libs/structs/structdiff/diff.go | 4 +- libs/structs/structdiff/diff_test.go | 14 ++-- libs/structs/structpath/path.go | 78 ++++++++++++++----- libs/structs/structwalk/walk.go | 4 +- libs/structs/structwalk/walk_test.go | 4 +- libs/structs/structwalk/walktype.go | 2 +- 12 files changed, 116 insertions(+), 87 deletions(-) delete mode 100644 acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.txt delete mode 100644 acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.terraform.txt diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.json b/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.json index bc685779ab..af69b5e087 100644 --- a/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.json +++ b/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.json @@ -1,7 +1,40 @@ { "plan": { "resources.jobs.foo": { - "action": "skip", + "action": "update", + "new_state": { + "value": { + "deployment": { + "kind": "BUNDLE", + "metadata_file_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/metadata.json" + }, + "edit_mode": "UI_LOCKED", + "format": "MULTI_TASK", + "max_concurrent_runs": 1, + "name": "My Wheel Job", + "queue": { + "enabled": true + }, + "tags": { + "tag1": "tag value" + }, + "tasks": [ + { + "environment_key": "test_env", + "libraries": [ + { + "whl": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/artifacts/.internal/hello.whl" + } + ], + "python_wheel_task": { + "entry_point": "run", + "package_name": "my_test_code" + }, + "task_key": "TestTask" + } + ] + } + }, "remote_state": { "created_time": [UNIX_TIME_MILLIS], "creator_user_name": "[USERNAME]", @@ -49,9 +82,8 @@ "action": "skip", "reason": "server_side_default" }, - "tags.new_tag": { - "action": "skip", - "reason": "server_side_default" + "tags['new_tag']": { + "action": "update" }, "timeout_seconds": { "action": "skip", diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.txt b/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.txt deleted file mode 100644 index c54c9d511c..0000000000 --- a/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.direct.txt +++ /dev/null @@ -1 +0,0 @@ -Plan: 0 to add, 0 to change, 0 to delete, 1 unchanged diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.terraform.txt b/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.terraform.txt deleted file mode 100644 index 75b4895c34..0000000000 --- a/acceptance/bundle/resources/jobs/remote_add_tag/out.plan_post_update.terraform.txt +++ /dev/null @@ -1,3 +0,0 @@ -update jobs.foo - -Plan: 0 to add, 1 to change, 0 to delete, 0 unchanged diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/output.txt b/acceptance/bundle/resources/jobs/remote_add_tag/output.txt index 53f3116919..545c3e6de3 100644 --- a/acceptance/bundle/resources/jobs/remote_add_tag/output.txt +++ b/acceptance/bundle/resources/jobs/remote_add_tag/output.txt @@ -6,46 +6,6 @@ Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/defaul Deploying resources... Updating deployment state... Deployment complete! -+ [CLI] jobs get [JOB_ID] -{ - "created_time":[UNIX_TIME_MILLIS], - "creator_user_name":"[USERNAME]", - "job_id":[JOB_ID], - "run_as_user_name":"[USERNAME]", - "settings": { - "deployment": { - "kind":"BUNDLE", - "metadata_file_path":"/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/metadata.json" - }, - "edit_mode":"UI_LOCKED", - "email_notifications": {}, - "format":"MULTI_TASK", - "max_concurrent_runs":1, - "name":"My Wheel Job", - "queue": { - "enabled":true - }, - "tags": { - "tag1":"tag value" - }, - "tasks": [ - { - "environment_key":"test_env", - "libraries": [ - { - "whl":"/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/artifacts/.internal/hello.whl" - } - ], - "python_wheel_task": { - "entry_point":"run", - "package_name":"my_test_code" - }, - "task_key":"TestTask" - } - ], - "timeout_seconds":0, - "webhook_notifications": {} - } -} +update jobs.foo -+ [CLI] jobs reset [JOB_ID] --json '{"job_id": "[JOB_ID]", "new_settings": {"deployment": {"kind": "BUNDLE", "metadata_file_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/metadata.json"}, "edit_mode": "UI_LOCKED", "email_notifications": {}, "format": "MULTI_TASK", "max_concurrent_runs": 1, "name": "My Wheel Job", "queue": {"enabled": true}, "tags": {"tag1": "tag value", "new_tag": "new_value"}, "tasks": [{"environment_key": "test_env", "libraries": [{"whl": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/artifacts/.internal/hello.whl"}], "python_wheel_task": {"entry_point": "run", "package_name": "my_test_code"}, "task_key": "TestTask"}], "timeout_seconds": 0, "webhook_notifications": {}}}' +Plan: 0 to add, 1 to change, 0 to delete, 0 unchanged diff --git a/acceptance/bundle/resources/jobs/remote_add_tag/script b/acceptance/bundle/resources/jobs/remote_add_tag/script index d5c39d4e71..dfdb7d472e 100644 --- a/acceptance/bundle/resources/jobs/remote_add_tag/script +++ b/acceptance/bundle/resources/jobs/remote_add_tag/script @@ -4,9 +4,9 @@ $CLI bundle deploy job_id="$(read_id.py jobs foo)" echo "$job_id:JOB_ID" >> ACC_REPLS -edit_resource.py -v jobs $job_id < out.plan_post_update.$DATABRICKS_BUNDLE_ENGINE.txt +$CLI bundle plan $CLI bundle plan -o json > out.plan_post_update.$DATABRICKS_BUNDLE_ENGINE.json diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index d5966bd0b5..08669bd29e 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -260,12 +260,11 @@ func interpretOldStateVsRemoteState(ctx context.Context, adapter *dresources.Ada m := make(map[string]deployplan.Trigger) for _, ch := range diff { - if ch.Old == nil && ch.Path.IsStringKey() { + if ch.Old == nil && ch.Path.IsDotString() { // The field was not set by us, but comes from the remote state. // This could either be server-side default or a policy. // In any case, this is not a change we should react to. - // Note, we only consider StringKeys here, because indexes and key-value pairs refer to slices and we want to react to new element in slices. - // Note, IsStringKey is also too broad - it currently covers struct fields and map keys, we don't want to include map keys here. + // Note, we only consider struct fields here. Adding/removing elements to/from maps and slices should trigger updates. m[ch.Path.String()] = deployplan.Trigger{ Action: deployplan.ActionTypeSkipString, Reason: "server_side_default", diff --git a/libs/structs/structdiff/diff.go b/libs/structs/structdiff/diff.go index 160f14b649..402e44dda7 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -205,7 +205,7 @@ func diffStruct(ctx *diffContext, path *structpath.PathNode, s1, s2 reflect.Valu if fieldName == "" { fieldName = sf.Name } - node := structpath.NewStringKey(path, fieldName) + node := structpath.NewDotString(path, fieldName) v1Field := s1.Field(i) v2Field := s2.Field(i) @@ -257,7 +257,7 @@ func diffMapStringKey(ctx *diffContext, path *structpath.PathNode, m1, m2 reflec k := keySet[ks] v1 := m1.MapIndex(k) v2 := m2.MapIndex(k) - node := structpath.NewStringKey(path, ks) + node := structpath.NewBracketString(path, ks) if err := diffValues(ctx, node, v1, v2, changes); err != nil { return err } diff --git a/libs/structs/structdiff/diff_test.go b/libs/structs/structdiff/diff_test.go index 9a216e0c4f..c4b87ae871 100644 --- a/libs/structs/structdiff/diff_test.go +++ b/libs/structs/structdiff/diff_test.go @@ -147,7 +147,7 @@ func TestGetStructDiff(t *testing.T) { name: "map diff", a: A{M: map[string]int{"a": 1}}, b: A{M: map[string]int{"a": 2}}, - want: []ResolvedChange{{Field: "m.a", Old: 1, New: 2}}, + want: []ResolvedChange{{Field: "m['a']", Old: 1, New: 2}}, }, { name: "slice diff", @@ -258,9 +258,9 @@ func TestGetStructDiff(t *testing.T) { a: map[string]C{"key1": {Title: "title", ForceSendFields: []string{"Name", "IsEnabled", "Title"}}}, b: map[string]C{"key1": {Title: "title", ForceSendFields: []string{"Age"}}}, want: []ResolvedChange{ - {Field: "key1.name", Old: "", New: nil}, - {Field: "key1.age", Old: nil, New: 0}, - {Field: "key1.is_enabled", Old: false, New: nil}, + {Field: "['key1'].name", Old: "", New: nil}, + {Field: "['key1'].age", Old: nil, New: 0}, + {Field: "['key1'].is_enabled", Old: false, New: nil}, }, }, @@ -270,9 +270,9 @@ func TestGetStructDiff(t *testing.T) { a: map[string]*C{"key1": {Title: "title", ForceSendFields: []string{"Name", "IsEnabled", "Title"}}}, b: map[string]*C{"key1": {Title: "title", ForceSendFields: []string{"Age"}}}, want: []ResolvedChange{ - {Field: "key1.name", Old: "", New: nil}, - {Field: "key1.age", Old: nil, New: 0}, - {Field: "key1.is_enabled", Old: false, New: nil}, + {Field: "['key1'].name", Old: "", New: nil}, + {Field: "['key1'].age", Old: nil, New: 0}, + {Field: "['key1'].is_enabled", Old: false, New: nil}, }, }, diff --git a/libs/structs/structpath/path.go b/libs/structs/structpath/path.go index 8c20be814d..1ec0da4ba9 100644 --- a/libs/structs/structpath/path.go +++ b/libs/structs/structpath/path.go @@ -11,9 +11,6 @@ import ( ) const ( - // Encodes string key, which is encoded as .field or as ['spark.conf'] - tagStringKey = -1 - // Encodes wildcard after a dot: foo.* tagDotStar = -2 @@ -22,6 +19,13 @@ const ( // Encodes key/value index, which is encoded as [key='value'] or [key="value"] tagKeyValue = -4 + + // Encodes .field syntax + // Note, most users should use StringKey() method which represents both DotString and BracketString + tagDotString = -5 + + // Encodes ["field"] syntax + tagBracketString = -6 ) // PathNode represents a node in a path for struct diffing. @@ -73,8 +77,28 @@ func (p *PathNode) KeyValue() (key, value string, ok bool) { return "", "", false } -func (p *PathNode) IsStringKey() bool { - return p != nil && p.index == tagStringKey +func (p *PathNode) IsDotString() bool { + return p != nil && p.index == tagDotString +} + +func (p *PathNode) DotString() (string, bool) { + if p == nil { + return "", false + } + if p.index == tagDotString { + return p.key, true + } + return "", false +} + +func (p *PathNode) BracketString() (string, bool) { + if p == nil { + return "", false + } + if p.index == tagBracketString { + return p.key, true + } + return "", false } // StringKey returns either Field() or MapKey() if either is available @@ -82,7 +106,7 @@ func (p *PathNode) StringKey() (string, bool) { if p == nil { return "", false } - if p.index == tagStringKey { + if p.index == tagDotString || p.index == tagBracketString { return p.key, true } return "", false @@ -122,14 +146,32 @@ func NewIndex(prev *PathNode, index int) *PathNode { } } -// NewStringKey creates either StructField or MapKey -// The fieldName should be the resolved field name (e.g., from JSON tag or Go field name). -func NewStringKey(prev *PathNode, fieldName string) *PathNode { +// NewDotString creates a PathNode for dot notation (.field). +func NewDotString(prev *PathNode, fieldName string) *PathNode { return &PathNode{ prev: prev, key: fieldName, - index: tagStringKey, + index: tagDotString, + } +} + +// NewBracketString creates a PathNode for bracket notation (["field"]). +func NewBracketString(prev *PathNode, fieldName string) *PathNode { + return &PathNode{ + prev: prev, + key: fieldName, + index: tagBracketString, + } +} + +// NewStringKey creates a PathNode, choosing dot notation if the field name is valid, +// otherwise bracket notation. This maintains backward compatibility while automatically +// selecting the appropriate representation. +func NewStringKey(prev *PathNode, fieldName string) *PathNode { + if isValidField(fieldName) { + return NewDotString(prev, fieldName) } + return NewBracketString(prev, fieldName) } func NewDotStar(prev *PathNode) *PathNode { @@ -193,14 +235,14 @@ func (p *PathNode) String() string { result.WriteString("=") result.WriteString(EncodeMapKey(node.value)) result.WriteString("]") - } else if isValidField(node.key) { - // Valid field name + } else if node.index == tagDotString { + // Dot notation: .field if i != 0 { result.WriteString(".") } result.WriteString(node.key) - } else { - // Map key with single quotes + } else if node.index == tagBracketString { + // Bracket notation: ['field'] result.WriteString("[") result.WriteString(EncodeMapKey(node.key)) result.WriteString("]") @@ -309,11 +351,11 @@ func Parse(s string) (*PathNode, error) { case stateField: if ch == '.' { - result = NewStringKey(result, currentToken.String()) + result = NewDotString(result, currentToken.String()) currentToken.Reset() state = stateFieldStart } else if ch == '[' { - result = NewStringKey(result, currentToken.String()) + result = NewDotString(result, currentToken.String()) currentToken.Reset() state = stateBracketOpen } else if !isReservedFieldChar(ch) { @@ -380,7 +422,7 @@ func Parse(s string) (*PathNode, error) { state = stateMapKey case ']': // End of map key - result = NewStringKey(result, currentToken.String()) + result = NewBracketString(result, currentToken.String()) currentToken.Reset() state = stateExpectDotOrEnd default: @@ -461,7 +503,7 @@ func Parse(s string) (*PathNode, error) { case stateStart: return result, nil // Empty path, result is nil case stateField: - result = NewStringKey(result, currentToken.String()) + result = NewDotString(result, currentToken.String()) return result, nil case stateDotStar: result = NewDotStar(result) diff --git a/libs/structs/structwalk/walk.go b/libs/structs/structwalk/walk.go index e3eff3bc7c..791077cfc6 100644 --- a/libs/structs/structwalk/walk.go +++ b/libs/structs/structwalk/walk.go @@ -92,7 +92,7 @@ func walkValue(path *structpath.PathNode, val reflect.Value, field *reflect.Stru sort.Strings(keys) for _, ks := range keys { v := val.MapIndex(reflect.ValueOf(ks)) - node := structpath.NewStringKey(path, ks) + node := structpath.NewBracketString(path, ks) walkValue(node, v, nil, visit) } @@ -131,7 +131,7 @@ func walkStruct(path *structpath.PathNode, s reflect.Value, visit VisitFunc) { if fieldName == "" { fieldName = sf.Name } - node := structpath.NewStringKey(path, fieldName) + node := structpath.NewDotString(path, fieldName) fieldVal := s.Field(i) // Skip zero values with omitempty unless field is explicitly forced. diff --git a/libs/structs/structwalk/walk_test.go b/libs/structs/structwalk/walk_test.go index c5c729eabf..4cbf0d3a1c 100644 --- a/libs/structs/structwalk/walk_test.go +++ b/libs/structs/structwalk/walk_test.go @@ -87,8 +87,8 @@ func TestValueJobSettings(t *testing.T) { } assert.Equal(t, map[string]any{ - `tags.env`: "test", - `tags.team`: "data", + `tags['env']`: "test", + `tags['team']`: "data", "name": "test-job", "max_concurrent_runs": 5, "timeout_seconds": 3600, diff --git a/libs/structs/structwalk/walktype.go b/libs/structs/structwalk/walktype.go index 4d572e792f..56b60a92f2 100644 --- a/libs/structs/structwalk/walktype.go +++ b/libs/structs/structwalk/walktype.go @@ -127,7 +127,7 @@ func walkTypeStruct(path *structpath.PathNode, st reflect.Type, visit VisitTypeF if fieldName == "" { fieldName = sf.Name } - node := structpath.NewStringKey(path, fieldName) + node := structpath.NewDotString(path, fieldName) walkTypeValue(node, sf.Type, &sf, visit, visitedCount) } } From af2b493751f8556bb716bbea92919ae59b32a593 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 1 Dec 2025 16:33:57 +0100 Subject: [PATCH 6/8] update tests --- acceptance/bundle/migrate/basic/out.plan_update.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acceptance/bundle/migrate/basic/out.plan_update.json b/acceptance/bundle/migrate/basic/out.plan_update.json index 46f5f0e219..f062cefae2 100644 --- a/acceptance/bundle/migrate/basic/out.plan_update.json +++ b/acceptance/bundle/migrate/basic/out.plan_update.json @@ -175,7 +175,7 @@ "name": { "action": "update" }, - "tags.myjob_name": { + "tags['myjob_name']": { "action": "update" } }, From f6711760e58381c88ae824b2047e3e02e2348f34 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 1 Dec 2025 20:49:26 +0100 Subject: [PATCH 7/8] add comments --- acceptance/bin/edit_resource.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/acceptance/bin/edit_resource.py b/acceptance/bin/edit_resource.py index 9a957fa72b..6270666794 100755 --- a/acceptance/bin/edit_resource.py +++ b/acceptance/bin/edit_resource.py @@ -1,4 +1,10 @@ #!/usr/bin/env python3 +""" +Implements get / update / set for a given resource. + +Fetches a given resource by id from the backend, executes Python code read from stdin and updates the resource. +""" + import sys import os import subprocess @@ -14,6 +20,9 @@ CLI = os.environ["CLI"] +# Each class should be named after CLI command group and implement get(id) and set(id, value) methods: + + class jobs: def get(self, job_id): return run_json([CLI, "jobs", "get", job_id])["settings"] From 5204a5c3f53d82ec2f5d25d4afe74a3b5b38bd89 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 1 Dec 2025 21:06:34 +0100 Subject: [PATCH 8/8] add TestValueNonEmptyMap to structwalk; update structpath tests to use NewDotString/NewBracketString directly in some cases --- libs/structs/structpath/path.go | 5 ++--- libs/structs/structpath/path_test.go | 12 ++++++------ libs/structs/structwalk/walk_test.go | 4 ++++ 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/libs/structs/structpath/path.go b/libs/structs/structpath/path.go index 1ec0da4ba9..977af60554 100644 --- a/libs/structs/structpath/path.go +++ b/libs/structs/structpath/path.go @@ -164,9 +164,8 @@ func NewBracketString(prev *PathNode, fieldName string) *PathNode { } } -// NewStringKey creates a PathNode, choosing dot notation if the field name is valid, -// otherwise bracket notation. This maintains backward compatibility while automatically -// selecting the appropriate representation. +// NewStringKey creates a PathNode, choosing dot notation if the fieldName is a valid field name, +// otherwise bracket notation. func NewStringKey(prev *PathNode, fieldName string) *PathNode { if isValidField(fieldName) { return NewDotString(prev, fieldName) diff --git a/libs/structs/structpath/path_test.go b/libs/structs/structpath/path_test.go index c4e427666d..585990aa32 100644 --- a/libs/structs/structpath/path_test.go +++ b/libs/structs/structpath/path_test.go @@ -33,7 +33,7 @@ func TestPathNode(t *testing.T) { }, { name: "map key", - node: NewStringKey(nil, "mykey"), + node: NewDotString(nil, "mykey"), String: `mykey`, StringKey: "mykey", }, @@ -59,31 +59,31 @@ func TestPathNode(t *testing.T) { // Two node tests { name: "struct field -> array index", - node: NewIndex(NewStringKey(nil, "items"), 3), + node: NewIndex(NewDotString(nil, "items"), 3), String: "items[3]", Index: 3, }, { name: "struct field -> map key", - node: NewStringKey(NewStringKey(nil, "config"), "database.name"), + node: NewBracketString(NewDotString(nil, "config"), "database.name"), String: `config['database.name']`, StringKey: "database.name", }, { name: "struct field -> struct field", - node: NewStringKey(NewStringKey(nil, "user"), "name"), + node: NewDotString(NewDotString(nil, "user"), "name"), String: "user.name", StringKey: "name", }, { name: "map key -> array index", - node: NewIndex(NewStringKey(nil, "servers list"), 0), + node: NewIndex(NewBracketString(nil, "servers list"), 0), String: `['servers list'][0]`, Index: 0, }, { name: "array index -> struct field", - node: NewStringKey(NewIndex(nil, 2), "id"), + node: NewDotString(NewIndex(nil, 2), "id"), String: "[2].id", StringKey: "id", }, diff --git a/libs/structs/structwalk/walk_test.go b/libs/structs/structwalk/walk_test.go index 4cbf0d3a1c..302679eaff 100644 --- a/libs/structs/structwalk/walk_test.go +++ b/libs/structs/structwalk/walk_test.go @@ -37,6 +37,10 @@ func TestValueEmptyMap(t *testing.T) { assert.Empty(t, flatten(t, make(map[string]int))) } +func TestValueNonEmptyMap(t *testing.T) { + assert.Equal(t, map[string]any{"['hello']": 5}, flatten(t, map[string]int{"hello": 5})) +} + func TestValueEmptySlice(t *testing.T) { assert.Empty(t, flatten(t, []string{})) }