Fixed requiring positional arguments for API URL parameters #878
Conversation
There was a problem hiding this comment.
Looking at the generated code, we now always load JSON into the request payload if it is specified, but also always require the positional argument(s) to be set. Is that correct? What if a parameter is both set as positional argument as well as in the JSON body?
|
@pietern this is intentional and actually correct, positional arguments are required because they are part of API URL and if they are set in the JSON file as well they will be ignored and positional arguments takes precedence |
| @@ -162,7 +161,7 @@ func new{{.PascalName}}() *cobra.Command { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| }{{end}}{{if $useJsonForAllFields }} else { | |||
| }{{end}}{{if .CanSetRequiredFieldsFromJson }} else { | |||
There was a problem hiding this comment.
What threw me off here when reading this is, is that this check really pertains to the previous block. I.e. if . CanSetRequiredFieldsFromJson and cmd.Flags().Changed("json"), then we don't want to bother 1) prompting for an ID, or 2) checking the # of args.
I know this is a rename so it is no different from what it was, but please add a comment describing why we need this statement + else branch. It's rather daunting at the moment.
| } | ||
| updateReq.Id = args[0] |
There was a problem hiding this comment.
When I look at this command, I think we actually don't need to force the positional arg. The ID field is part of the request structure and can be provided through JSON.
There was a problem hiding this comment.
It is possible this is caused by aliasing at the OpenAPI level where the same name is used for both the ID in the request body as well as the ID in the URL path.
There was a problem hiding this comment.
That's right. @andrewnester and I discussed this in a one-on-one last week. The question is whether the JSON body corresponds to the Go structure or to the request body. If the former, then positional parameters are only needed when not specified in the body; if the latter, we may want to reject with an error message if specified in the JSON to make usage obvious to users.
CLI: * Never load authentication configuration from bundle for sync command ([#889](#889)). * Fixed requiring positional arguments for API URL parameters ([#878](#878)). Bundles: * Add support for validating CLI version when loading a jsonschema object ([#883](#883)). * Do not emit wheel wrapper error when python_wheel_wrapper setting is true ([#894](#894)). * Resolve configuration before performing verification ([#890](#890)). * Fix wheel task not working with with 13.x clusters ([#898](#898)). Internal: * Skip prompt on completion hook ([#888](#888)). * New YAML loader to support configuration location ([#828](#828)). Dependency updates: * Bump github.com/mattn/go-isatty from 0.0.19 to 0.0.20 ([#896](#896)).
CLI: * Never load authentication configuration from bundle for sync command ([#889](#889)). * Fixed requiring positional arguments for API URL parameters ([#878](#878)). Bundles: * Add support for validating CLI version when loading a jsonschema object ([#883](#883)). * Do not emit wheel wrapper error when python_wheel_wrapper setting is true ([#894](#894)). * Resolve configuration before performing verification ([#890](#890)). * Fix wheel task not working with with 13.x clusters ([#898](#898)). Internal: * Skip prompt on completion hook ([#888](#888)). * New YAML loader to support configuration location ([#828](#828)). Dependency updates: * Bump github.com/mattn/go-isatty from 0.0.19 to 0.0.20 ([#896](#896)).
Changes
Some commands such as update commands have an argument in their url, for example in pipeline we have
PUT pipelines/<id>to update the pipeline.Such parameters must be required and respected even if
--jsonflag with the payload passed.Note: this depends on these PRs in Go SDK:
databricks/databricks-sdk-go#660
databricks/databricks-sdk-go#661
Tests
Manually running
databricks pipelines update