From 3043b3da116d51db7a3a703a78fe621bce0e0cc8 Mon Sep 17 00:00:00 2001 From: Dongjiang You Date: Tue, 11 Oct 2016 16:35:54 -0700 Subject: [PATCH] Updated registry creation experience 1. Removed creating resource group/storage account if user specified ones don't exist. 2. Updated error handling when deployment fails. 3. Added registry name/storage account/resource group validation before submitting template. --- src/command_modules/azure-cli-acr/README.rst | 4 +- .../cli/command_modules/acr/_arm_utils.py | 50 +++++++++++-------- .../azure/cli/command_modules/acr/_help.py | 2 +- .../cli/command_modules/acr/_validators.py | 39 +++++++++++---- .../azure/cli/command_modules/acr/custom.py | 42 ++++++++++------ .../azure/cli/command_modules/acr/storage.py | 24 ++++----- .../acr/template.existing.json | 2 +- .../cli/command_modules/acr/template.new.json | 2 +- 8 files changed, 105 insertions(+), 60 deletions(-) diff --git a/src/command_modules/azure-cli-acr/README.rst b/src/command_modules/azure-cli-acr/README.rst index 09843a0a0f2..5b3cf6369c4 100644 --- a/src/command_modules/azure-cli-acr/README.rst +++ b/src/command_modules/azure-cli-acr/README.rst @@ -33,7 +33,7 @@ Create a container registry --resource-group -g [Required]: Name of resource group. --app-id : The app id of an existing service principal. If provided, no --new-sp or -p should be specified. - --disable-admin : Disable admin user. + --enable-admin : Enable admin user. --new-sp : Create a new service principal. If provided, no --app-id should be specified. Optional: Use -p to specify a password. --password -p : Password used to log into a container registry. @@ -44,7 +44,7 @@ Create a container registry Examples Create a container registry with a new storage account az acr create -n myRegistry -g myResourceGroup -l southus - Create a container registry with a specified new/existing storage account + Create a container registry with an existing storage account az acr create -n myRegistry -g myResourceGroup -l southus -s myStorageAccount Create a container registry with a new service principal az acr create -n myRegistry -g myResourceGroup -l southus --new-sp -p myPassword -r Owner diff --git a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_arm_utils.py b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_arm_utils.py index 5fc7af7b05e..050dcfaa419 100644 --- a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_arm_utils.py +++ b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_arm_utils.py @@ -35,12 +35,13 @@ def arm_get_registries_in_resource_group(resource_group_name): result = get_resources_in_resource_group(resource_group_name, RESOURCE_TYPE) return [Registry(item.id, item.name, item.location, item.tags) for item in result] -def arm_get_registry_by_name(registry_name): - '''Returns the container registry that matches the registry name. - :param str registry_name: The name of container registry +def _arm_get_resource_by_name(resource_name, resource_type): + '''Returns the ARM resource in the current subscription with resource_name. + :param str resource_name: The name of resource + :param str resource_type: The type of resource ''' - registries = arm_get_registries_in_subscription() - elements = [item for item in registries if item.name.lower() == registry_name.lower()] + result = get_resources_in_subscription(resource_type) + elements = [item for item in result if item.name.lower() == resource_name.lower()] if len(elements) == 0: return None @@ -48,7 +49,20 @@ def arm_get_registry_by_name(registry_name): return elements[0] else: raise CLIError( - 'More than one container registries are found with name: {}'.format(registry_name)) + 'More than one resources with type {} are found with name: {}'.format( + resource_type, resource_name)) + +def arm_get_registry_by_name(registry_name): + '''Returns the named container registry. + :param str registry_name: The name of container registry + ''' + return _arm_get_resource_by_name(registry_name, RESOURCE_TYPE) + +def arm_get_storage_account_by_name(storage_account_name): + '''Returns the named storage account. + :param str storage_account_name: The name of storage account + ''' + return _arm_get_resource_by_name(storage_account_name, 'Microsoft.Storage/storageAccounts') def arm_deploy_template(resource_group_name, registry_name, @@ -67,7 +81,7 @@ def arm_deploy_template(resource_group_name, import os parameters = _parameters(registry_name, location, storage_account_name, admin_user_enabled) - storage_account_resource_group, _ = _arm_get_storage_account(storage_account_name) + storage_account_resource_group, _ = _parse_storage_account(storage_account_name) if storage_account_resource_group: file_path = os.path.join(os.path.dirname(__file__), 'template.existing.json') @@ -128,21 +142,17 @@ def _parameters(registry_name, } return parameters -def _arm_get_storage_account(storage_account_name): - '''Returns the dict of tags in the storage account. +def _parse_storage_account(storage_account_name): + '''Returns resource group and tags in the storage account. :param str storage_account_name: The name of storage account ''' - result = get_resources_in_subscription('Microsoft.Storage/storageAccounts') - elements = [item for item in result if item.name.lower() == storage_account_name.lower()] + storage_account = arm_get_storage_account_by_name(storage_account_name) - if len(elements) == 0: - return None, None - elif len(elements) == 1: - storage_account_resource_group = get_resource_group_name_by_resource_id(elements[0].id) - return storage_account_resource_group, elements[0].tags + if storage_account: + storage_account_resource_group = get_resource_group_name_by_resource_id(storage_account.id) + return storage_account_resource_group, storage_account.tags else: - raise CLIError( - 'More than one storage accounts are found with name: {}'.format(storage_account_name)) + return None, None def add_tag_storage_account(storage_account_name, registry_name): '''Add a new tag (key, value) to the storage account. @@ -150,7 +160,7 @@ def add_tag_storage_account(storage_account_name, registry_name): :param str registry_name: The name of container registry ''' from azure.mgmt.storage.models import StorageAccountUpdateParameters - storage_account_resource_group, tags = _arm_get_storage_account(storage_account_name) + storage_account_resource_group, tags = _parse_storage_account(storage_account_name) tags[registry_name.lower()] = 'acr' client = get_storage_service_client().storage_accounts @@ -165,7 +175,7 @@ def delete_tag_storage_account(storage_account_name, registry_name): :param str registry_name: The name of container registry ''' from azure.mgmt.storage.models import StorageAccountUpdateParameters - storage_account_resource_group, tags = _arm_get_storage_account(storage_account_name) + storage_account_resource_group, tags = _parse_storage_account(storage_account_name) registry_name = registry_name.lower() if registry_name in tags and tags[registry_name] == 'acr': diff --git a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_help.py b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_help.py index 4a69ea0e94c..c073d88648d 100644 --- a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_help.py +++ b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_help.py @@ -44,7 +44,7 @@ - name: Create a container registry with a new storage account text: az acr create -n myRegistry -g myResourceGroup -l southus - - name: Create a container registry with a specified new/existing storage account + - name: Create a container registry with an existing storage account text: az acr create -n myRegistry -g myResourceGroup -l southus -s myStorageAccount - name: Create a container registry with a new service principal diff --git a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_validators.py b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_validators.py index bb9bf8a5c5f..95d786feab4 100644 --- a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_validators.py +++ b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_validators.py @@ -6,8 +6,6 @@ import re import uuid -from azure.mgmt.resource.resources.models.resource_group import ResourceGroup - from azure.cli.command_modules.acr.mgmt_acr.models import RegistryNameCheckRequest from azure.cli.core._util import CLIError @@ -21,6 +19,10 @@ get_acr_service_client, get_arm_service_client ) +from ._arm_utils import arm_get_storage_account_by_name + +import azure.cli.core._logging as _logging +logger = _logging.get_az_logger(__name__) def validate_registry_name(namespace): if namespace.registry_name: @@ -50,20 +52,39 @@ def validate_registry_name_create(namespace): def validate_storage_account_name(namespace): client = storage_client_factory().storage_accounts - if namespace.storage_account_name is None: + if namespace.storage_account_name: + storage_account_name = namespace.storage_account_name + if arm_get_storage_account_by_name(storage_account_name) is None: + logger.warning('Command to create a storage account:') + logger.warning( + ' az storage account create ' +\ + '-n -g -l --sku Standard_LRS') + logger.warning( + 'The container registry must be at the same location as the storage account.') + raise CLIError( + 'The storage account {} does not exist in the current subscription.'\ + .format(storage_account_name)) + else: while True: storage_account_name = str(uuid.uuid4()).replace('-', '')[:24] if client.check_name_availability(storage_account_name).name_available is True: #pylint: disable=E1101 namespace.storage_account_name = storage_account_name - break + logger.warning( + 'New storage account with name %s will be created and used.', + storage_account_name) + return def validate_resource_group_name(namespace): - client = get_arm_service_client() - if namespace.resource_group_name: - if not client.resource_groups.check_existence(namespace.resource_group_name): - parameters = ResourceGroup(location=namespace.location) - client.resource_groups.create_or_update(namespace.resource_group_name, parameters) + client = get_arm_service_client() + resource_group_name = namespace.resource_group_name + + if not client.resource_groups.check_existence(resource_group_name): + logger.warning('Command to create a resource group:') + logger.warning(' az resource group create -n -l ') + raise CLIError( + 'The resource group {} does not exist in the current subscription.'\ + .format(resource_group_name)) def validate_password(namespace): if namespace.password and not namespace.new_sp: diff --git a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/custom.py b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/custom.py index bd090a64557..a471a0aa559 100644 --- a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/custom.py +++ b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/custom.py @@ -3,7 +3,10 @@ # Licensed under the MIT License. See License.txt in the project root for license information. #--------------------------------------------------------------------------------------------- -from azure.cli.core.commands import cli_command +from azure.cli.core.commands import ( + cli_command, + LongRunningOperation +) from azure.cli.core._util import CLIError from azure.cli.command_modules.role.custom import _create_role_assignment @@ -53,7 +56,7 @@ def acr_create(registry_name, #pylint: disable=too-many-arguments app_id=None, password=None, role=DEFAULT_ROLE, - disable_admin=False): + enable_admin=False): '''Create a container registry. :param str registry_name: The name of container registry :param str resource_group_name: The name of resource group @@ -63,7 +66,7 @@ def acr_create(registry_name, #pylint: disable=too-many-arguments :param str app_id: The app id of an existing service principal :param str password: The password used to log into the container registry :param str role: The name of role - :param bool disable_admin: Disable admin user + :param bool enable_admin: Enable admin user ''' if new_sp and app_id: raise CLIError('new-service-principal and app-id should not be specified together.') @@ -76,26 +79,37 @@ def acr_create(registry_name, #pylint: disable=too-many-arguments session_key) = create_service_principal(registry_name, password) # Create a container registry - arm_deploy_template(resource_group_name, - registry_name, - location, - storage_account_name, - not disable_admin).wait() # wait for the template deployment to finish + LongRunningOperation()( + arm_deploy_template(resource_group_name, + registry_name, + location, + storage_account_name, + enable_admin) + ) client = get_acr_service_client().registries registry = client.get_properties(resource_group_name, registry_name) add_tag_storage_account(storage_account_name, registry_name) + logger.warning('\nCreate a new service principal and assign access:') + logger.warning( + ' az ad sp create-for-rbac --scopes %s --role Owner --secret ', + registry.id) #pylint: disable=E1101 + logger.warning('\nUse an existing service principal and assign access:') + logger.warning( + ' az role assignment create --scope %s --role Owner --assignee ', + registry.id) #pylint: disable=E1101 + # Create role assignment if app_id: _create_role_assignment(role, app_id, scope=registry.id, #pylint: disable=E1101 ocp_aad_session_key=session_key) - logger.warning("Service principal has been configured.") - logger.warning(" id(client_id): " + app_id) + logger.warning('Service principal has been configured.') + logger.warning(' id(client_id): %s', app_id) if password: - logger.warning(" password(client_secret): " + password) + logger.warning(' password(client_secret): %s', password) return registry @@ -185,10 +199,10 @@ def acr_update(registry_name, #pylint: disable=too-many-arguments app_id, scope=registry.id, #pylint: disable=E1101 ocp_aad_session_key=session_key) - logger.warning("Service principal has been configured.") - logger.warning(" id(client_id): " + app_id) + logger.warning('Service principal has been configured.') + logger.warning(' id(client_id): %s', app_id) if password: - logger.warning(" password(client_secret): " + password) + logger.warning(' password(client_secret): %s', password) # Set admin_user_enabled admin_user_enabled = None diff --git a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/storage.py b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/storage.py index 9546ba01987..746ace9f3c4 100644 --- a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/storage.py +++ b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/storage.py @@ -3,7 +3,10 @@ # Licensed under the MIT License. See License.txt in the project root for license information. #--------------------------------------------------------------------------------------------- -from azure.cli.core.commands import cli_command +from azure.cli.core.commands import ( + cli_command, + LongRunningOperation +) from ._factory import get_acr_service_client from ._arm_utils import ( @@ -16,12 +19,8 @@ get_resource_group_name_by_resource_id, registry_not_found ) - from ._format import output_format -import azure.cli.core._logging as _logging -logger = _logging.get_az_logger(__name__) - def acr_storage_update(registry_name, storage_account_name, resource_group_name=None): @@ -39,13 +38,14 @@ def acr_storage_update(registry_name, old_storage_account_name = registry.properties.storage_account.name - # Create a container registry - arm_deploy_template(resource_group_name, - registry_name, - registry.location, - storage_account_name, - registry.properties.admin_user_enabled).wait() - # wait for the template deployment to finish + # Update a container registry + LongRunningOperation()( + arm_deploy_template(resource_group_name, + registry_name, + registry.location, + storage_account_name, + registry.properties.admin_user_enabled) + ) client = get_acr_service_client().registries registry = client.get_properties(resource_group_name, registry_name) diff --git a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/template.existing.json b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/template.existing.json index aedd0047280..98bd399f1ff 100644 --- a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/template.existing.json +++ b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/template.existing.json @@ -35,7 +35,7 @@ }, "adminUserEnabled": { "type": "bool", - "defaultValue": true, + "defaultValue": false, "metadata": { "description": "Enable admin user" } diff --git a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/template.new.json b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/template.new.json index 60d73438583..a5fc188933c 100644 --- a/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/template.new.json +++ b/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/template.new.json @@ -35,7 +35,7 @@ }, "adminUserEnabled": { "type": "bool", - "defaultValue": true, + "defaultValue": false, "metadata": { "description": "Enable admin user" }