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

AzCLIError should take kwarg original_exception #16348

Open
jiasli opened this issue Dec 23, 2020 · 2 comments
Open

AzCLIError should take kwarg original_exception #16348

jiasli opened this issue Dec 23, 2020 · 2 comments

Comments

@jiasli
Copy link
Member

jiasli commented Dec 23, 2020

Is your feature request related to a problem? Please describe.

When an exception is caught and wrapped by AzCLIError, the analysis of the root cause of the exception may not be correct, thus the recommendation can also be misleading.

For example, #15776 assumes InvalidURL is caused by invalid AAD endpoint in cloud registration, but the exception is actually caused by invalid proxy URL. This PR is later reverted by #15962.

In the current design, the original exception is discarded and it is very difficult to identify what the original issue is.

Describe the solution you'd like

AzCLIError should take optional kwarg original_exception=None and when any exception is wrapped/replaced by AzCLIError, original_exception should be set as the original exception so that azure.cli.core.util.handle_exception can log callstack and error message of original_exception to

  • --debug log
  • telemetry (if allowed)
@ghost ghost added the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Dec 23, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Dec 23, 2020

AzCLIError

@ghost ghost removed the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Dec 23, 2020
@jiasli
Copy link
Member Author

jiasli commented Jan 26, 2021

Another or possibly better solution is to take advantage of Python's built-in Exception Chaining:

The latest Pylint also has this check with W0707:

> pylint --help-msg W0707
:raise-missing-from (W0707): *Consider explicitly re-raising using the 'from' keyword*
  Python 3's exception chaining means it shows the traceback of the current
  exception, but also the original exception. Not using `raise from` makes the
  traceback inaccurate, because the message implies there is a bug in the
  exception-handling code itself, which is a separate situation than wrapping
  an exception. This message belongs to the exceptions checker.

Knack has adopted this approach: microsoft/knack#232.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants