Skip to content
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

Deserialize path & query parameters when passing the --json flag #1127

Closed
wants to merge 1 commit into from

Conversation

mgyucht
Copy link
Contributor

@mgyucht mgyucht commented Jan 17, 2024

Changes

Depends on databricks/databricks-sdk-go#772.

The CLI allows users to pass a --json flag to provide all parameters needed for executing the request. When a request includes fields annotated with json:"-", those fields are not deserialized. As a result, requests like

databricks account workspace-assignment update --json "{\"principal_id\":123, \"permissions\": [\"USER\"],\"workspace_id \":456}"

fail because the request made is

PUT /api/2.0/accounts/<account-id>/workspaces/0/permissionassignments/principals/0

Custom unmarshalling support is added to the Go SDK in databricks/databricks-sdk-go#772, allowing the CLI to unmarshal a JSON object containing fields which are not normally unmarshalled because they don't belong to the request body but are part of the request definition.

Tests

  • Unit test.
  • Manual test:
$ ./databricks --profile <> account workspace-assignment update --json '{"principal_id":<>, "workspace_id": <>, "permissions":["ADMIN"]}' --debug
10:49:29  INFO start pid=70628 version=0.0.0-dev+3d359319dfa2 args="./databricks, --profile, <>, account, workspace-assignment, update, --json, {\"principal_id\":<>, \"workspace_id\": <>, \"permissions\":[\"ADMIN\"]}, --debug"
10:49:29 DEBUG Loading <> profile from /Users/miles/.databrickscfg pid=70628 sdk=true
10:49:29  INFO Ignoring pat auth, because databricks-cli is preferred pid=70628 sdk=true
10:49:29  INFO Ignoring basic auth, because databricks-cli is preferred pid=70628 sdk=true
10:49:29  INFO Ignoring oauth-m2m auth, because databricks-cli is preferred pid=70628 sdk=true
10:49:29  INFO Refreshed OAuth token from Databricks CLI, expires on 2024-01-17 11:46:04.025043 +0100 CET pid=70628 sdk=true
10:49:29 DEBUG Using Databricks CLI authentication with Databricks OAuth tokens pid=70628 sdk=true
10:49:29  INFO Refreshed OAuth token from Databricks CLI, expires on 2024-01-17 11:46:04.025043 +0100 CET pid=70628 sdk=true
10:49:32 DEBUG PUT /api/2.0/accounts/<>/workspaces/<>/permissionassignments/principals/<>
> {
>   "permissions": [
>     "ADMIN"
>   ]
> }
< HTTP/2.0 200 OK
< {
<   "permissions": [
<     "ADMIN"
<   ],
<   "principal": {
<     "display_name": "<>",
<     "principal_id": <>,
<     "user_name": "<>"
<   }
< } pid=70628 sdk=true
10:49:32  INFO completed execution pid=70628 exit_code=0

@mgyucht mgyucht requested a review from andrewnester January 17, 2024 09:54
Copy link
Contributor

@andrewnester andrewnester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on my understanding path fields should be marked as required positional arguments, see for example this method

https://github.com/databricks/databricks-sdk-go/blob/1fb6a9d657833170f9ad250bc138400ccddf5e4e/openapi/code/entity.go#L300

What happens right now is that for commands which MustUseJson like assignment command we ignore positional arguments and they never set.
https://github.com/databricks/cli/blob/main/.codegen/service.go.tmpl#L128

So we need to change this generational logic (again) to handle this case with required path arguments.

cc @pietern

@andrewnester
Copy link
Contributor

The fix is in this PR: #1129

@pietern pietern deleted the deserialize-ignored-fields-with-json-flag branch January 17, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants