-
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
{Log} Adapt az_command_data_logger
to Knack 0.8.0
#17324
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -69,89 +69,88 @@ def handle_exception(ex): # pylint: disable=too-many-locals, too-many-statement | |||||||||
# Print the traceback and exception message | ||||||||||
logger.debug(traceback.format_exc()) | ||||||||||
|
||||||||||
with CommandLoggerContext(logger): | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
azure-cli/src/azure-cli-core/azure/cli/core/azclierror.py Lines 62 to 65 in 4560e96
Calling it here is redundant. Besides, no log is written here, so these lines shouldn't be surrounded by |
||||||||||
error_msg = getattr(ex, 'message', str(ex)) | ||||||||||
exit_code = 1 | ||||||||||
|
||||||||||
if isinstance(ex, azclierror.AzCLIError): | ||||||||||
az_error = ex | ||||||||||
|
||||||||||
elif isinstance(ex, JMESPathError): | ||||||||||
error_msg = "Invalid jmespath query supplied for `--query`: {}".format(error_msg) | ||||||||||
az_error = azclierror.InvalidArgumentValueError(error_msg) | ||||||||||
az_error.set_recommendation(QUERY_REFERENCE) | ||||||||||
error_msg = getattr(ex, 'message', str(ex)) | ||||||||||
exit_code = 1 | ||||||||||
|
||||||||||
if isinstance(ex, azclierror.AzCLIError): | ||||||||||
az_error = ex | ||||||||||
|
||||||||||
elif isinstance(ex, JMESPathError): | ||||||||||
error_msg = "Invalid jmespath query supplied for `--query`: {}".format(error_msg) | ||||||||||
az_error = azclierror.InvalidArgumentValueError(error_msg) | ||||||||||
az_error.set_recommendation(QUERY_REFERENCE) | ||||||||||
|
||||||||||
elif isinstance(ex, SSLError): | ||||||||||
az_error = azclierror.AzureConnectionError(error_msg) | ||||||||||
az_error.set_recommendation(SSLERROR_TEMPLATE) | ||||||||||
|
||||||||||
elif isinstance(ex, CloudError): | ||||||||||
if extract_common_error_message(ex): | ||||||||||
error_msg = extract_common_error_message(ex) | ||||||||||
status_code = str(getattr(ex, 'status_code', 'Unknown Code')) | ||||||||||
AzCLIErrorType = get_error_type_by_status_code(status_code) | ||||||||||
az_error = AzCLIErrorType(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, ValidationError): | ||||||||||
az_error = azclierror.ValidationError(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, CLIError): | ||||||||||
# TODO: Fine-grained analysis here | ||||||||||
az_error = azclierror.UnclassifiedUserFault(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, AzureError): | ||||||||||
if extract_common_error_message(ex): | ||||||||||
error_msg = extract_common_error_message(ex) | ||||||||||
AzCLIErrorType = get_error_type_by_azure_error(ex) | ||||||||||
az_error = AzCLIErrorType(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, AzureException): | ||||||||||
if is_azure_connection_error(error_msg): | ||||||||||
az_error = azclierror.AzureConnectionError(error_msg) | ||||||||||
else: | ||||||||||
# TODO: Fine-grained analysis here for Unknown error | ||||||||||
az_error = azclierror.UnknownError(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, SSLError): | ||||||||||
elif isinstance(ex, ClientRequestError): | ||||||||||
if is_azure_connection_error(error_msg): | ||||||||||
az_error = azclierror.AzureConnectionError(error_msg) | ||||||||||
elif isinstance(ex.inner_exception, SSLError): | ||||||||||
# When msrest encounters SSLError, msrest wraps SSLError in ClientRequestError | ||||||||||
az_error = azclierror.AzureConnectionError(error_msg) | ||||||||||
az_error.set_recommendation(SSLERROR_TEMPLATE) | ||||||||||
|
||||||||||
elif isinstance(ex, CloudError): | ||||||||||
if extract_common_error_message(ex): | ||||||||||
error_msg = extract_common_error_message(ex) | ||||||||||
status_code = str(getattr(ex, 'status_code', 'Unknown Code')) | ||||||||||
AzCLIErrorType = get_error_type_by_status_code(status_code) | ||||||||||
az_error = AzCLIErrorType(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, ValidationError): | ||||||||||
az_error = azclierror.ValidationError(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, CLIError): | ||||||||||
# TODO: Fine-grained analysis here | ||||||||||
az_error = azclierror.UnclassifiedUserFault(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, AzureError): | ||||||||||
if extract_common_error_message(ex): | ||||||||||
error_msg = extract_common_error_message(ex) | ||||||||||
AzCLIErrorType = get_error_type_by_azure_error(ex) | ||||||||||
az_error = AzCLIErrorType(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, AzureException): | ||||||||||
if is_azure_connection_error(error_msg): | ||||||||||
az_error = azclierror.AzureConnectionError(error_msg) | ||||||||||
else: | ||||||||||
# TODO: Fine-grained analysis here for Unknown error | ||||||||||
az_error = azclierror.UnknownError(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, ClientRequestError): | ||||||||||
if is_azure_connection_error(error_msg): | ||||||||||
az_error = azclierror.AzureConnectionError(error_msg) | ||||||||||
elif isinstance(ex.inner_exception, SSLError): | ||||||||||
# When msrest encounters SSLError, msrest wraps SSLError in ClientRequestError | ||||||||||
az_error = azclierror.AzureConnectionError(error_msg) | ||||||||||
az_error.set_recommendation(SSLERROR_TEMPLATE) | ||||||||||
else: | ||||||||||
az_error = azclierror.ClientRequestError(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, HttpOperationError): | ||||||||||
message, _ = extract_http_operation_error(ex) | ||||||||||
if message: | ||||||||||
error_msg = message | ||||||||||
status_code = str(getattr(ex.response, 'status_code', 'Unknown Code')) | ||||||||||
AzCLIErrorType = get_error_type_by_status_code(status_code) | ||||||||||
az_error = AzCLIErrorType(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, HTTPError): | ||||||||||
status_code = str(getattr(ex.response, 'status_code', 'Unknown Code')) | ||||||||||
AzCLIErrorType = get_error_type_by_status_code(status_code) | ||||||||||
az_error = AzCLIErrorType(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, KeyboardInterrupt): | ||||||||||
error_msg = 'Keyboard interrupt is captured.' | ||||||||||
az_error = azclierror.ManualInterrupt(error_msg) | ||||||||||
|
||||||||||
else: | ||||||||||
error_msg = "The command failed with an unexpected error. Here is the traceback:" | ||||||||||
az_error = azclierror.CLIInternalError(error_msg) | ||||||||||
az_error.set_exception_trace(ex) | ||||||||||
az_error.set_recommendation("To open an issue, please run: 'az feedback'") | ||||||||||
|
||||||||||
if isinstance(az_error, azclierror.ResourceNotFoundError): | ||||||||||
exit_code = 3 | ||||||||||
|
||||||||||
az_error.print_error() | ||||||||||
az_error.send_telemetry() | ||||||||||
|
||||||||||
return exit_code | ||||||||||
az_error = azclierror.ClientRequestError(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, HttpOperationError): | ||||||||||
message, _ = extract_http_operation_error(ex) | ||||||||||
if message: | ||||||||||
error_msg = message | ||||||||||
status_code = str(getattr(ex.response, 'status_code', 'Unknown Code')) | ||||||||||
AzCLIErrorType = get_error_type_by_status_code(status_code) | ||||||||||
az_error = AzCLIErrorType(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, HTTPError): | ||||||||||
status_code = str(getattr(ex.response, 'status_code', 'Unknown Code')) | ||||||||||
AzCLIErrorType = get_error_type_by_status_code(status_code) | ||||||||||
az_error = AzCLIErrorType(error_msg) | ||||||||||
|
||||||||||
elif isinstance(ex, KeyboardInterrupt): | ||||||||||
error_msg = 'Keyboard interrupt is captured.' | ||||||||||
az_error = azclierror.ManualInterrupt(error_msg) | ||||||||||
|
||||||||||
else: | ||||||||||
error_msg = "The command failed with an unexpected error. Here is the traceback:" | ||||||||||
az_error = azclierror.CLIInternalError(error_msg) | ||||||||||
az_error.set_exception_trace(ex) | ||||||||||
az_error.set_recommendation("To open an issue, please run: 'az feedback'") | ||||||||||
|
||||||||||
if isinstance(az_error, azclierror.ResourceNotFoundError): | ||||||||||
exit_code = 3 | ||||||||||
|
||||||||||
az_error.print_error() | ||||||||||
az_error.send_telemetry() | ||||||||||
|
||||||||||
return exit_code | ||||||||||
|
||||||||||
|
||||||||||
def extract_common_error_message(ex): | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ | |
import os | ||
import time | ||
import unittest | ||
|
||
from unittest import mock | ||
from knack.events import EVENT_CLI_POST_EXECUTE | ||
|
||
from azure.cli.core.azlogging import CommandLoggerContext | ||
|
@@ -138,7 +138,7 @@ def _helper_test_log_contents_correct(self): | |
# check failed cli command: | ||
data_dict = command_log_files[0].command_data_dict | ||
self.assertTrue(data_dict["success"] is False) | ||
self.assertEqual("There was an error during execution.", data_dict["errors"][0].strip()) | ||
self.assertEqual("The extension alias is not installed.", data_dict["errors"][0].strip()) | ||
self.assertEqual(data_dict["command_args"], "extension remove -n {}") | ||
|
||
# check successful cli command | ||
|
@@ -192,22 +192,18 @@ def _run_cmd(self, command, checks=None, expect_failure=False): | |
cli_ctx = get_dummy_cli() | ||
cli_ctx.logging.command_log_dir = self.temp_command_log_dir | ||
|
||
# hotfix here for untouch feedback's code to avoid introducing possible break change. | ||
# unregister the event for auto closing CLI's file logging after execute() is done | ||
cli_ctx.unregister_event(EVENT_CLI_POST_EXECUTE, cli_ctx.logging.deinit_cmd_metadata_logging) | ||
|
||
# manually handle error logging as azure.cli.core.util's handle_exception function is mocked out in testsdk / patches | ||
# this logged error tests that we can properly parse errors from command log file. | ||
with CommandLoggerContext(logger): | ||
result = execute(cli_ctx, command, expect_failure=expect_failure).assert_with_checks(checks) | ||
# azure.cli.core.util.handle_exception is mocked by azure.cli.testsdk.patches.patch_main_exception_handler | ||
# Patch it again so that errors are properly written to command log file. | ||
from azure.cli.core.util import handle_exception | ||
original_handle_exception = handle_exception | ||
|
||
if result.exit_code != 0: | ||
logger.error("There was an error during execution.") | ||
|
||
# close it manually because we unregister the deinit_cmd_metadata_logging | ||
# callback from EVENT_CLI_POST_EXECUTE event | ||
cli_ctx.logging.end_cmd_metadata_logging(result.exit_code) | ||
def _handle_exception_with_log(ex, *args, **kwargs): | ||
with CommandLoggerContext(logger): | ||
logger.error(ex) | ||
original_handle_exception(*args, **kwargs) | ||
|
||
with mock.patch('azure.cli.core.util.handle_exception', _handle_exception_with_log): | ||
result = execute(cli_ctx, command, expect_failure=expect_failure).assert_with_checks(checks) | ||
Comment on lines
+205
to
+206
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old code registers |
||
return result | ||
|
||
|
||
|
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.
No log is written here. The scope of
CommandLoggerContext
should be as small as possible.