On-board Location Based Services #5716
Conversation
- Add the Preview Terms agreement requirement - Remove location, custom_header, and raw parameters for account operations.
[Fix] --key-type, -t are now --key
[Refactor] get_account command now specifically throws error only if it's a 404 response.
[Output] Added recordings
|
View a preview at https://prompt.ws/r/Azure/azure-cli/5716 |
|
PR being submitted for code review only. |
|
|
||
|
|
||
| # pylint: disable=line-too-long | ||
| def get_account(client, resource_group_name, account_name): |
There was a problem hiding this comment.
This can be simply reflected from the SDK.
| :raises: | ||
| :class:`ErrorException<azure.mgmt.locationbasedservices.models.ErrorException>` | ||
| or :class:`ErrorException<knack.util.CLIError>` | ||
| """ |
There was a problem hiding this comment.
There are a few ways of supplying help text to the CLI. The docstring for a custom method is the only method that is actively discouraged. It would make more sense to put this in the help.py file so it is easier for people to find if they need to edit it (a docs feature) and because that system has more capabilities. If you just want to reuse help text across commands, then use the help kwarg in params.py.
| if out.response.status_code == 404: | ||
| raise CLIError("The resource 'Microsoft.LocationBasedServices/accounts/" + account_name + | ||
| "' under resource group '" + resource_group_name + "' was not found.") | ||
| return out.output |
There was a problem hiding this comment.
If you did need to have a custom command for this, it should return:
return client.get(resource_group_name, account_name)| if not agree: # ... in order to pass ScenarioTest | ||
| response = prompt_y_n('I confirm that I have read and agree to the Microsoft Azure Preview Terms.') | ||
| if not response: | ||
| raise CLIError('You must agree to the Microsoft Azure Preview Terms to create an account.') |
There was a problem hiding this comment.
This is generally not used anywhere else for create commands. Is there a particular reason LBS would be different?
| # Retrieve ClientRawResponse from get call | ||
| out = client.get(resource_group_name, account_name, raw=True) | ||
| # Handle 404 error | ||
| if out.response.status_code == 404: |
There was a problem hiding this comment.
Many show commands use error_handler=empty_on_404 kwarg in their command registrations specifically to avoid printing an error message.
This would be a very non-standard way of handling this error. My suggestions:
- follow suit with other commands and use empty_on_404
- Do nothing and see if the default error does what you want.
- Only if the error in 2 is inadequate or results in a stack trace, what a small error handler that accepts the incoming exception, processes it, and raise a CLIError.
Note on custom get_accountOur default behavior does not return anything (like empty_on_404). All three behaviors are due to the way how RestAPI specs (Swagger) file is defined. My team suggested that we show 404 errors. Note Public Preview Terms promptOur service is currently on a Public Preview, but our actual PR will occur on our General Availability. |
|
Hi @jp94 on the 404, here are some options:
Officially the recommendation is to go with option 1 as it allows you to distinguish between "the command succeeded and the thing isn't there (which you can see by the output)" and "the command failed". |
|
Cool. I will use |
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Command Guidelines
(see Authoring Command Modules)