-
Notifications
You must be signed in to change notification settings - Fork 3k
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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. A couple comments and questions.
|
||
|
||
# pylint: disable=line-too-long | ||
def get_account(client, resource_group_name, account_name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)