Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 215
Represent struct fields and map keys; fix ss_default to ignore map keys#4041
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
e1c0382139307210881a243a9bdf20cf2dcaf2b493f6711765204a5cFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| #!/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 | ||
| import argparse | ||
| import json | ||
| import pprint | ||
| sys.path.insert(0, os.path.dirname(__file__)) | ||
| import util | ||
| from util import run_json, run | ||
denik marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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"] | ||
| 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() | ||
denik marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import sys | ||
| import subprocess | ||
| import json | ||
| import shlex | ||
| VERBOSE = False | ||
denik marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -175,7 +175,7 @@ | ||
| "name": { | ||
| "action": "update" | ||
| }, | ||
| "tags.myjob_name": { | ||
| "tags['myjob_name']": { | ||
denik marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| "action": "update" | ||
| } | ||
| }, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious, why the wheel if the test is about tags? ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No reason, I just copied job config from another test. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can be removed to keep the test tight. Since this is a local only test, the whole tasks block can be omitted. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| { | ||
| "plan": { | ||
| "resources.jobs.foo": { | ||
| "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]", | ||
| "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": "update" | ||
| }, | ||
| "timeout_seconds": { | ||
| "action": "skip", | ||
| "reason": "server_side_default" | ||
| }, | ||
| "webhook_notifications": { | ||
| "action": "skip", | ||
| "reason": "server_side_default" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "plan": { | ||
| "resources.jobs.foo": { | ||
| "action": "update" | ||
| } | ||
| } | ||
| } |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| 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! | ||
| update jobs.foo | ||
| Plan: 0 to add, 1 to change, 0 to delete, 0 unchanged |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 jobs $job_id <<EOF | ||
| r["tags"]["new_tag"] = "new_value" | ||
| EOF | ||
| $CLI bundle plan | ||
| $CLI bundle plan -o json > out.plan_post_update.$DATABRICKS_BUNDLE_ENGINE.json |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| RecordRequests = false | ||
| Ignore = [".databricks", "hello.whl"] | ||
denik marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: Maps can have server-side defaults as well. Like tags for example. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth noting as a comment. ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate? How does TF handle those? Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For example, sql warehouses have a owner tag by default: Looks like Terraform does a suppress diff for these, i.e. ignoring remote changes: https://github.com/databricks/terraform-provider-databricks/blob/f5f476292a7e2198dcf4009d4c0c1411161637f6/sql/resource_sql_endpoint.go#L80 ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks - good to know. If it's a fixed key we might be able to handle it more precisely than ignoring all custom_tags changes, but ignore | ||
| m[ch.Path.String()] = deployplan.Trigger{ | ||
| Action: deployplan.ActionTypeSkipString, | ||
| Reason: "server_side_default", | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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}}, | ||
denik marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| }, | ||
| { | ||
| 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}, | ||
| }, | ||
| }, | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.