Added "az acr" commands #859

djyou · 2016-09-11T04:54:05Z

Added commands to manage Azure container registries (create/delete/show/list/update).
Integrated repository list and show-tags commands to manage repositories.
Added storage command group to manage storage account for container registries.
Added credential command group to manage admin user credentials.
Added mgmt_acr SDK for Python.

This change is

azurecla · 2016-09-11T04:54:14Z

Hi @djyou, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

It looks like you're working at Microsoft (doyou). If you're full-time, we DON'T require a contribution license agreement.

If you are a vendor, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;
#ByDesign

djyou · 2016-09-12T19:33:54Z

/cc @sajayantony @sivagms #ByDesign

tjprescott · 2016-09-12T20:16:47Z

..._modules/azure-cli-acr/azure/cli/command_modules/acr/containerregistry/container_registry.py

+from . import models
+
+
+class ContainerRegistryConfiguration(Configuration):


ontainerRegistryConfiguratio [](start = 7, length = 28)

Is this not part of the compute SDK? #Resolved

Azure container registry is a different service from compute or ACS. ACS is one consumer of the registry and container registry can be used without Azure VMs or websites. We have deployed from this registry into other cloud providers and local network machines, and we regularly use it as part of our development process via the CI containers. #Resolved

sajayantony · 2016-09-12T21:01:21Z

/cc @mayurid #ByDesign

mayurid · 2016-09-12T22:51:13Z

@djyou : Do you guys have a Swagger spec and generated SDKS? #Resolved

djyou · 2016-09-12T22:56:43Z

Yes, we do have a swagger spec and the generated SDK is under /src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/containerregistry/, which will eventually go to SDK for Python repo, but for now we haven't separated it from the CLI. #Resolved

derekbekoe · 2016-09-13T17:09:25Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_utils.py

+    '''
+    if len(registry_name) < 5 or len(registry_name) > 60:
+        logger.error('The registry name must be between 5 and 60 characters.')
+        raise SystemExit(1)


Instead of logging to error and then raising SystemExit, raise CLIError.
i.e. raise CLIError('The registry name must be between 5 and 60 characters.')

For an example, see https://github.com/Azure/azure-cli/blob/master/src/command_modules/azure-cli-network/azure/cli/command_modules/network/_validators.py#L69 #Resolved

tjprescott · 2016-09-14T17:00:58Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_params.py

+    help='Key of storage account.'
+)
+
+register_cli_argument('acr', 'registry_name', arg_type=registry_name_type)


registry_name_type [](start = 55, length = 18)

You can delete the "CliArgumentType" definition since you aren't reusing it and simply put the parameters inline. This line would become:

register_cli_argument('acr', 'registry_name', options_list=('--name', '-n'), help='Name of container registry', validator=validate_registry_name)

This will apply to storage account name/key as well (any place you don't need to reuse a base definition). #Resolved

tjprescott · 2016-09-19T15:29:18Z

LGTM

sajayantony · 2016-09-20T16:52:16Z

/cc @DavidObando

sajayantony · 2016-10-18T23:50:07Z

/cc @JasonRShaver

derekbekoe

Added some comments.

derekbekoe · 2016-10-19T00:13:00Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_utils.py

+
+def registry_not_found(registry_name):
+    raise CLIError(
+        'ERROR: Registry {} cannot be found in the current subscription.'\


We don't put 'ERROR:' before CLIErrors because they are logged to logging.error() anyway.
Otherwise, if colored logging is disabled or we are logging to a file, you'll see "ERROR: ERROR: Registry not found"

derekbekoe · 2016-10-19T00:14:39Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_validators.py

+            )
+        )
+
+        if not result.name_available: #pylint: disable=E1101


We use the pylint error names instead of codes so it's clear when reading the code what is being disabled. So this would be # pylint: disable=no-member

derekbekoe · 2016-10-19T00:17:34Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_validators.py

+
+def validate_role(namespace):
+    if namespace.role and not namespace.role.lower() in ALLOWED_ROLES:
+        raise CLIError('The role {} is not allowed. Allowed roles (Owner, Contributor, Reader).'\


Suggest instead of hard-coding the string '(Owner, Contributor, Reader)', could do ', '.join(ALLOWED_ROLES)

derekbekoe · 2016-10-19T00:20:28Z

src/command_modules/azure-cli-acr/setup.py

+]
+
+DEPENDENCIES = [
+    'azure==2.0.0rc6',


Do you depend on the SDK, if so, you should use the specific components you need e.g. azure-mgmt-* instead.
Looks like that's why the build is failing with Keyvault errors as you're including an incompatible version of keyvault.

derekbekoe

LGTM @tjprescott should also take a look since he provided feedback on the earlier iterations.

tjprescott

A few random comments, but the biggest issue is that it looks like the changes we discussed after last week's meeting have not been made (regarding new/existing storage account and taking out the ability to create a SP as part of the registry create command).

tjprescott · 2016-10-19T21:36:45Z

src/command_modules/azure-cli-acr/README.rst

+        az acr create: Create a container registry.
+
+    Arguments
+        --location -l       [Required]: Location.


During the meeting we talked about location being optional and inheriting from the resource group if not provided.

We are only supporting one location currently (but will increase) and it is very unlikely that the location for the resource group would be the one that we support. I would keep it as required for now.

tjprescott · 2016-10-19T21:38:23Z

src/command_modules/azure-cli-acr/README.rst

+        --app-id                      : The app id of an existing service principal. If provided, no
+                                        --new-sp or -p should be specified.
+        --enable-admin                : Enable admin user.
+        --new-sp                      : Create a new service principal. If provided, no --app-id should


During the meeting we talked about removing this entirely and displaying the commands to create the service principal in the examples section.

The create_for_rbac PR is still pending, we will remove the code when it is merged.

This PR has been merged.

tjprescott · 2016-10-19T21:39:29Z

src/command_modules/azure-cli-acr/README.rst

+        --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.
+        --role -r                     : Name of role. Allowed roles: owner, contributor, reader.


I believe this is another parameter that we discussed removing since the SP creation wouldn't be part of the registry create command.

Will be removed once create_for_rbac PR is merged.

tjprescott · 2016-10-19T21:40:35Z

src/command_modules/azure-cli-acr/README.rst

+        --password -p                 : Password used to log into a container registry.
+        --role -r                     : Name of role. Allowed roles: owner, contributor, reader.
+                                        Default: reader.
+        --storage-account-name -s     : Name of new or existing storage account. If not provided, a


This isn't consistent with what we discussed during the meeting. We talked about either creating a new SA (if omitted) or treating it as an exiting SA if provided and throwing an error if the SA didn't actually exist in the subscription.

Description issue, will fix. The code is doing the right thing.

tjprescott · 2016-10-19T21:41:35Z

src/command_modules/azure-cli-acr/README.rst

+            az acr create -n myRegistry -g myResourceGroup -l southus
+        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


We discussed breaking this scenario into the three CLI commands (SP create for RBAC, role assignment create, registry create)

Will be removed once create_for_rbac PR is merged.

tjprescott · 2016-10-19T21:52:48Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_params.py

+
+register_cli_argument('acr', 'role',
+                      options_list=('--role', '-r'),
+                      help='Name of role. Allowed roles: {}'.format(', '.join(ALLOWED_ROLES)),


Assuming --role remains a valid parameter, you should use **enum_choice_list(ALLOWED_ROLES) as this will make your choice list case insensitive. While it will work with a list of strings, even better would be to use the autorest generated enum object, if one exists. It will be less maintenance burden for you.

tjprescott · 2016-10-19T21:54:01Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_utils.py

+    return resource_id[resource_id.index(resource_group_keyword) + len(resource_group_keyword):
+                       resource_id.index('/providers/')]
+
+def create_service_principal(registry_name, password=None):


During the meeting we discussed omitting this implementation and making use of the existing commands to create a service principal.

tjprescott · 2016-10-19T21:57:13Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_validators.py

+        client = get_arm_service_client()
+        resource_group_name = namespace.resource_group_name
+
+        if not client.resource_groups.check_existence(resource_group_name):


Nowhere in the CLI do we make checking for resource_group existence part of client-side validation. You're welcome to of course, but it would be additional maintenance burden for you. Most services return a pretty clear error if the specified resource group doesn't exist.

The code has a step to validate template deployment for a resource group. It will also return false if the resource group doesn't exist, so we can't differentiate between the resource group doesn't exist and the deployment is invalid. Can we keep it here and if it turns out to be a burden, we will remove it. #ByDesign

tjprescott · 2016-10-19T21:58:12Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_validators.py

+                'The resource group {} does not exist in the current subscription.'\
+                .format(resource_group_name))
+
+def validate_password(namespace):


For security reasons, I would recommend allowing the user to omit password from the command line and submit it interactively like we do for az login. @johanste do you agree?

We will be removing this password here for SP. The only case the user needs to provide a password is in repository command group and when the user has to use an SP to list repositories (since we cannot retrieve SP password from anywhere). I tend to think that this is OK, at least for now, since if the user does docker login using the same credential, the username/password will also be stored by the OS. Even for az login, the user can also do az login -u myUsername -p myPassword. If this is a general concern, we definitely want to have a work item and track this. @sajayantony

tjprescott · 2016-10-19T21:59:22Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_validators.py

+    if namespace.password and not namespace.new_sp:
+        raise CLIError('--password has to be used with --new-sp.')
+
+def validate_role(namespace):


This is not necessary and will never be executed. Because allowed roles is in a choice list, the fact that the supplied value is not in the list will be caught earlier by argparse and thus it will never reach here.

djyou · 2016-10-20T19:28:29Z

@tjprescott I believe we have addressed these issues as much as we can. Thanks for your feedback!

sajayantony · 2016-10-20T20:58:31Z

@tjprescott if there is anything blocking this from merging, please let us know.

1. Added commands to manage Azure container registries (create/delete/show/list/update). 2. Integrated repository list and show-tags commands to manage repositories. 3. Added storage command group to manage storage account for container registries. 4. Added credential command group to manage admin user credentials. 5. Added mgmt_acr SDK for Python.

azurecla added the cla-not-required label Sep 11, 2016

djyou force-pushed the master branch from 8a6a17d to 9f3adfc Compare September 12, 2016 18:37

tjprescott reviewed Sep 12, 2016
View reviewed changes

derekbekoe reviewed Sep 13, 2016
View reviewed changes

tjprescott reviewed Sep 14, 2016
View reviewed changes

djyou force-pushed the master branch from 7425d9d to e515e63 Compare September 16, 2016 23:27

djyou force-pushed the master branch from ab92b41 to a18b1f4 Compare September 19, 2016 17:48

tjprescott changed the title ~~Added "az acr" commands~~ [DO NOT MERGE] Added "az acr" commands Sep 20, 2016

djyou force-pushed the master branch from a18b1f4 to a28ae87 Compare October 10, 2016 19:25

djyou force-pushed the master branch from 5a252a7 to 89ac486 Compare October 18, 2016 23:24

djyou changed the title ~~[DO NOT MERGE] Added "az acr" commands~~ Added "az acr" commands Oct 18, 2016

derekbekoe reviewed Oct 19, 2016

View reviewed changes

derekbekoe approved these changes Oct 19, 2016

View reviewed changes

tjprescott suggested changes Oct 19, 2016

View reviewed changes

djyou force-pushed the master branch 4 times, most recently from c511d86 to 9e071be Compare October 20, 2016 18:44

tjprescott approved these changes Oct 20, 2016

View reviewed changes

djyou force-pushed the master branch from 9e071be to e3aec29 Compare October 20, 2016 21:44

JasonRShaver merged commit 4ee4d11 into Azure:master Oct 20, 2016

derekbekoe mentioned this pull request Oct 20, 2016

acr module should use their mgmt SDK instead of including code in module #1107

Closed

haroldrandom added the cla-not-required label Oct 25, 2019

		from . import models


		class ContainerRegistryConfiguration(Configuration):

Added "az acr" commands #859

Added "az acr" commands #859

Conversation

djyou commented Sep 11, 2016 • edited by johanste Loading

azurecla commented Sep 11, 2016 • edited by tjprescott Loading

djyou commented Sep 12, 2016 • edited by tjprescott Loading

tjprescott Sep 12, 2016 • edited Loading

Choose a reason for hiding this comment

djyou Sep 12, 2016 • edited by tjprescott Loading

Choose a reason for hiding this comment

sajayantony commented Sep 12, 2016 • edited by tjprescott Loading

mayurid commented Sep 12, 2016 • edited by tjprescott Loading

djyou commented Sep 12, 2016 • edited by tjprescott Loading

derekbekoe Sep 13, 2016 • edited by tjprescott Loading

Choose a reason for hiding this comment

tjprescott Sep 14, 2016 • edited Loading

Choose a reason for hiding this comment

tjprescott commented Sep 19, 2016

sajayantony commented Sep 20, 2016

sajayantony commented Oct 18, 2016

derekbekoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbekoe left a comment

Choose a reason for hiding this comment

tjprescott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djyou commented Oct 20, 2016

sajayantony commented Oct 20, 2016

djyou commented Sep 11, 2016 •

edited by johanste

Loading

azurecla commented Sep 11, 2016 •

edited by tjprescott

Loading

djyou commented Sep 12, 2016 •

edited by tjprescott

Loading

tjprescott Sep 12, 2016 •

edited

Loading

djyou Sep 12, 2016 •

edited by tjprescott

Loading

sajayantony commented Sep 12, 2016 •

edited by tjprescott

Loading

mayurid commented Sep 12, 2016 •

edited by tjprescott

Loading

djyou commented Sep 12, 2016 •

edited by tjprescott

Loading

derekbekoe Sep 13, 2016 •

edited by tjprescott

Loading

tjprescott Sep 14, 2016 •

edited

Loading