Skip to content

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

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.

Sign up for GitHub

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

Merged
zhoxing-ms merged 3 commits into from
Dec 17, 2021
Merged

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc4c265
Migrate cloud error for resource Track 2
zhoxing-ms Dec 15, 2021
75a087c
Remove error catch for get_default_location_from_resource_group
zhoxing-ms Dec 15, 2021
c283eb0
Update src/azure-cli-core/azure/cli/core/commands/validators.py
zhoxing-ms Dec 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Migrate cloud error for resource Track 2
  • Loading branch information
@zhoxing-ms
zhoxing-ms committed Dec 15, 2021
commit dc4c26513e2e2752eaff22cfd501645d80e896b3
4 changes: 2 additions & 2 deletions src/azure-cli-core/azure/cli/core/commands/validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,13 @@ def generate_deployment_name(namespace):
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
from msrestazure.azure_exceptions import CloudError
from azure.core.exceptions import HttpResponseError
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 •
edited
Loading

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:

azure-cli/src/azure-cli-core/azure/cli/core/parser.py

Line 153 in 110f7b4

self.exit(2)

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

azure-cli/src/azure-cli-core/azure/cli/core/util.py

Line 71 in 110f7b4

exit_code = 1

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.

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 •
edited
Loading

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

raise CLIError('error retrieving default location: {}'.format(ex.message))
namespace.location = rg.location # pylint: disable=no-member
logger.debug("using location '%s' from resource group '%s'", namespace.location, rg.name)
Expand Down