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

{Core} Remove the try-catch logic of CloudError in get_default_location_from_resource_group() #20739

Merged
merged 3 commits into from
Dec 17, 2021

Conversation

zhoxing-ms
Copy link
Contributor

@zhoxing-ms zhoxing-ms commented Dec 15, 2021

Fix: #20732

Since the resource module has migrated to Track 2 #17783, CloudError should be replaced by HttpResponseError in Track 2, so submit this PR to modify it

Description

Testing Guide

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change.
[Component Name 2] az command b: Add some customer-facing feature.


This checklist is used to make sure that common guidelines for a pull request are followed.

from knack.util import CLIError

resource_client = get_mgmt_service_client(cmd.cli_ctx, ResourceType.MGMT_RESOURCE_RESOURCES)
try:
rg = resource_client.resource_groups.get(namespace.resource_group_name)
except CloudError as ex:
except HttpResponseError as ex:
Copy link
Member

@jiasli jiasli Dec 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we don't use try catch here to let azure.cli.core.parser.AzCliCommandParser.validation_error handle exceptions, such as azure.core.exceptions.ResourceNotFoundError?

And it works pretty well, just like what we are having right now.

def get_default_location_from_resource_group(cmd, namespace):
    if not namespace.location:
        from azure.cli.core.commands.client_factory import get_mgmt_service_client
        resource_client = get_mgmt_service_client(cmd.cli_ctx, ResourceType.MGMT_RESOURCE_RESOURCES)

        # We don't use try catch here to let azure.cli.core.parser.AzCliCommandParser.validation_error
        # handle exceptions, such as azure.core.exceptions.ResourceNotFoundError
        rg = resource_client.resource_groups.get(namespace.resource_group_name)
        namespace.location = rg.location  # pylint: disable=no-member
        logger.debug("using location '%s' from resource group '%s'", namespace.location, rg.name)

But the side effect is that the exit code would be 2:

If the ResourceNotFoundError is caught, the exit code would be 1:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, makes sense~
Here raise the original specific error seems to be a better choice.

Copy link
Contributor Author

@zhoxing-ms zhoxing-ms Dec 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed offline, we first maintain the latest exit code and error throwing ways, because it seems to work well at present. So I remove the try-catch logic now.

If additional information is needed to help locate related problem in the future, we can consider adding error log

@zhoxing-ms zhoxing-ms changed the title {Core} Migrate CloudError for resource Track 2 SDK {Core} Remove the try-catch logic of CloudError in get_default_location_from_resource_group Dec 15, 2021
@zhoxing-ms zhoxing-ms changed the title {Core} Remove the try-catch logic of CloudError in get_default_location_from_resource_group {Core} Remove the try-catch logic of CloudError in get_default_location_from_resource_group() Dec 15, 2021
@zhoxing-ms zhoxing-ms merged commit 8a6ff86 into Azure:dev Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing dependency of msrestazure from azure-cli-core
2 participants