diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 10a8cdb51b7..2b478007a70 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -17,3 +17,5 @@ This checklist is used to make sure that common guidelines for a pull request ar - [ ] The PR title and description has followed the guideline in [Submitting Pull Requests](https://github.com/Azure/azure-cli/tree/dev/doc/authoring_command_modules#submitting-pull-requests). - [ ] I adhere to the [Command Guidelines](https://github.com/Azure/azure-cli/blob/dev/doc/command_guidelines.md). + +- [ ] I adhere to the [Error Handling Guidelines](https://github.com/Azure/azure-cli/blob/dev/doc/error_handling_guidelines.md). diff --git a/doc/error_handling_guidelines.md b/doc/error_handling_guidelines.md index 394be5964c9..d4a094bdfce 100644 --- a/doc/error_handling_guidelines.md +++ b/doc/error_handling_guidelines.md @@ -17,14 +17,14 @@ For the new commands authoring, here are what need to be done to get onboard. __ The newly designed error types are provided in [azure/cli/core/azclierror.py](https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure/cli/core/azclierror.py), where the error types are defined in three layers with their structures shown below. `AzCLIError` is the base layer for all the newly defined error types. The second layer includes the main categories (`UserFault`, `ClientError`, `ServiceError`), which can be shown to users and also can be used for Telemetry analysis. __The third layer includes the specific error types, which are the ones command group authors can use when raising errors.__ ``` -| -- AzCLIError # [Base Layer]: DO NOT use it in command groups -| | -- UserFault # [Second Layer]: DO NOT use it in command groups -| | | -- CommandNotFoundError # [Third Layer]: Can be used in command groups +| -- AzCLIError # [Base Layer]: DO NOT use it in command groups +| | -- UserFault # [Second Layer]: DO NOT use it in command groups +| | | -- CommandNotFoundError # [Third Layer]: Can be used in command groups | | | -- UnrecognizedArgumentError | | | -- RequiredArgumentMissingError | | | -- MutuallyExclusiveArgumentError | | | -- InvalidArgumentValueError -| | | -- ArgumentParseError # fallback of argument related errors +| | | -- ArgumentUsageError # fallback of argument related errors | | | -- BadRequestError | | | -- UnauthorizedError | | | -- ForbiddenError @@ -35,8 +35,7 @@ The newly designed error types are provided in [azure/cli/core/azclierror.py](ht | | | -- ValidationError # fallback of validation related errors | | | -- FileOperationError | | | -- ManualInterrupt -| | | -- ... More ... -| | | -- UnknownError # fallback of UserFault related errors +| | | -- UnclassifiedUserFault # fallback of UserFault related errors | | -- ClientError | | | -- CLIInternalError | | -- ServiceError @@ -45,8 +44,8 @@ The newly designed error types are provided in [azure/cli/core/azclierror.py](ht To summarize, here is a list of rules for command group authors to select a proper error type. - __DO NOT__ use the error types defined in the base layer and the second layer +- Avoid using the fallback error types unless you can not find a specific one for your case - Consider defining a new error type if you can not find a proper one when it is a general error -- Avoid using the fallback error types unless you can not find a proper one and the error is not general enough for a new error type ### Apply the Error Type @@ -99,6 +98,7 @@ Below are the specific DOs and DON'Ts when writing the error messages. PRs viola __DOs__ - Use the capital letter ahead of an error message. +- Provide actionable message with argument suggestion. (e.g, Instead of using `resource group is missing, please provide a resource group name`, use `resource group is missing, please provide a resource group name by --resource-group`) __DON'Ts__ - Do not control the style of an error message. (e.g, the unnecessary `'\n'` and the colorization.) diff --git a/src/azure-cli-core/azure/cli/core/_profile.py b/src/azure-cli-core/azure/cli/core/_profile.py index 004a720fce8..b45ae4605c8 100644 --- a/src/azure-cli-core/azure/cli/core/_profile.py +++ b/src/azure-cli-core/azure/cli/core/_profile.py @@ -1349,7 +1349,7 @@ def _login_exception_handler(ex): from requests.exceptions import InvalidURL if isinstance(ex, InvalidURL): import traceback - from azure.cli.core.azclierror import UnknownError + from azure.cli.core.azclierror import UnclassifiedUserFault logger.debug('Invalid url when acquiring token\n%s', traceback.format_exc()) - raise UnknownError(error_msg='Invalid url when acquiring token', - recommendation='Please make sure the cloud is registered with valid url') + raise UnclassifiedUserFault(error_msg='Invalid url when acquiring token', + recommendation='Please make sure the cloud is registered with valid url') diff --git a/src/azure-cli-core/azure/cli/core/azclierror.py b/src/azure-cli-core/azure/cli/core/azclierror.py index 8cd8a689375..9fe4d9b3739 100644 --- a/src/azure-cli-core/azure/cli/core/azclierror.py +++ b/src/azure-cli-core/azure/cli/core/azclierror.py @@ -92,6 +92,14 @@ def send_telemetry(self): telemetry.set_failure(self.error_msg) if self.exception_trace: telemetry.set_exception(self.exception_trace, '') + + +class UnknownError(AzCLIError): + """ Unclear errors, could not know who should be responsible for the errors. + DO NOT raise this error class in your codes. """ + def send_telemetry(self): + super().send_telemetry() + telemetry.set_failure(self.error_msg) # endregion @@ -125,8 +133,8 @@ class InvalidArgumentValueError(UserFault): pass -class ArgumentParseError(UserFault): - """ Fallback of the argument parsing related errors. +class ArgumentUsageError(UserFault): + """ Fallback of the argument usage related errors. Avoid using this class unless the error can not be classified into the Argument related specific error types. """ pass @@ -199,7 +207,7 @@ class ValidationError(UserFault): pass -class UnknownError(UserFault): +class UnclassifiedUserFault(UserFault): """ Fallback of the UserFault related error types. Avoid using this class unless the error can not be classified into the UserFault related specific error types. diff --git a/src/azure-cli-core/azure/cli/core/parser.py b/src/azure-cli-core/azure/cli/core/parser.py index dbe07f71bb0..05e138e875d 100644 --- a/src/azure-cli-core/azure/cli/core/parser.py +++ b/src/azure-cli-core/azure/cli/core/parser.py @@ -20,7 +20,7 @@ from azure.cli.core.azclierror import UnrecognizedArgumentError from azure.cli.core.azclierror import RequiredArgumentMissingError from azure.cli.core.azclierror import InvalidArgumentValueError -from azure.cli.core.azclierror import ArgumentParseError +from azure.cli.core.azclierror import ArgumentUsageError from azure.cli.core.azclierror import CommandNotFoundError from azure.cli.core.azclierror import ValidationError @@ -165,7 +165,7 @@ def error(self, message): recommender.set_help_examples(self.get_examples(self.prog)) recommendation = recommender.recommend_a_command() - az_error = ArgumentParseError(message) + az_error = ArgumentUsageError(message) if 'unrecognized arguments' in message: az_error = UnrecognizedArgumentError(message) elif 'arguments are required' in message: diff --git a/src/azure-cli-core/azure/cli/core/tests/test_profile.py b/src/azure-cli-core/azure/cli/core/tests/test_profile.py index c3fc07e6fc2..14f8c3d9074 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_profile.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_profile.py @@ -1050,7 +1050,7 @@ def test_find_subscriptions_thru_username_non_password(self, mock_auth_context): @mock.patch('azure.cli.core._profile._get_authorization_code', autospec=True) def test_find_subscriptions_with_invalid_authority_url(self, _get_authorization_code_mock, mock_auth_context): from requests.exceptions import InvalidURL - from azure.cli.core.azclierror import UnknownError + from azure.cli.core.azclierror import UnclassifiedUserFault def mock_acquire(*args, **kwargs): raise InvalidURL(request='http://some.unknown.endpoints') @@ -1065,11 +1065,11 @@ def mock_acquire(*args, **kwargs): } finder = SubscriptionFinder(cli, lambda _, _1, _2: mock_auth_context, None, lambda _: None) - with self.assertRaisesRegexp(UnknownError, 'Invalid url when acquiring token'): + with self.assertRaisesRegexp(UnclassifiedUserFault, 'Invalid url when acquiring token'): finder.find_from_user_account(self.user1, 'bar', None, 'http://goo-resource') - with self.assertRaisesRegexp(UnknownError, 'Invalid url when acquiring token'): + with self.assertRaisesRegexp(UnclassifiedUserFault, 'Invalid url when acquiring token'): finder.find_through_authorization_code_flow(None, 'https://management.core.windows.net/', 'https:/some_aad_point/common') - with self.assertRaisesRegexp(UnknownError, 'Invalid url when acquiring token'): + with self.assertRaisesRegexp(UnclassifiedUserFault, 'Invalid url when acquiring token'): finder.find_through_interactive_flow(None, 'https://management.core.windows.net/') @mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True) diff --git a/src/azure-cli-core/azure/cli/core/util.py b/src/azure-cli-core/azure/cli/core/util.py index aa15a16d041..6fdebcc7c4a 100644 --- a/src/azure-cli-core/azure/cli/core/util.py +++ b/src/azure-cli-core/azure/cli/core/util.py @@ -96,8 +96,8 @@ def handle_exception(ex): # pylint: disable=too-many-locals, too-many-statement az_error = azclierror.ValidationError(error_msg) elif isinstance(ex, CLIError): - # TODO: Fine-grained analysis here for Unknown error - az_error = azclierror.UnknownError(error_msg) + # TODO: Fine-grained analysis here + az_error = azclierror.UnclassifiedUserFault(error_msg) elif isinstance(ex, AzureError): if extract_common_error_message(ex): diff --git a/src/azure-cli/azure/cli/command_modules/network/custom.py b/src/azure-cli/azure/cli/command_modules/network/custom.py index 544dc95289e..f5a482a6b31 100644 --- a/src/azure-cli/azure/cli/command_modules/network/custom.py +++ b/src/azure-cli/azure/cli/command_modules/network/custom.py @@ -2046,7 +2046,7 @@ def import_zone(cmd, resource_group_name, zone_name, file_name): logger.warning("In the future, zone name will be case insensitive.") RecordSet = cmd.get_models('RecordSet', resource_type=ResourceType.MGMT_NETWORK_DNS) - from azure.cli.core.azclierror import FileOperationError, UnknownError + from azure.cli.core.azclierror import FileOperationError, UnclassifiedUserFault try: file_text = read_file_content(file_name) except FileNotFoundError: @@ -2056,7 +2056,7 @@ def import_zone(cmd, resource_group_name, zone_name, file_name): except PermissionError: raise FileOperationError("Permission denied: " + str(file_name)) except OSError as e: - raise UnknownError(e) + raise UnclassifiedUserFault(e) zone_obj = parse_zone_file(file_text, zone_name) diff --git a/src/azure-cli/azure/cli/command_modules/privatedns/custom.py b/src/azure-cli/azure/cli/command_modules/privatedns/custom.py index 6861ec3f31b..9339f4ccd65 100644 --- a/src/azure-cli/azure/cli/command_modules/privatedns/custom.py +++ b/src/azure-cli/azure/cli/command_modules/privatedns/custom.py @@ -33,7 +33,7 @@ def import_zone(cmd, resource_group_name, private_zone_name, file_name): import sys from azure.mgmt.privatedns.models import RecordSet - from azure.cli.core.azclierror import FileOperationError, UnknownError + from azure.cli.core.azclierror import FileOperationError, UnclassifiedUserFault try: file_text = read_file_content(file_name) except FileNotFoundError: @@ -43,7 +43,7 @@ def import_zone(cmd, resource_group_name, private_zone_name, file_name): except PermissionError: raise FileOperationError("Permission denied: " + str(file_name)) except OSError as e: - raise UnknownError(e) + raise UnclassifiedUserFault(e) zone_obj = parse_zone_file(file_text, private_zone_name) origin = private_zone_name