[ACR] Add managed registries, webhooks, AAD auth, and repository delete #3747

djyou · 2017-06-15T23:29:56Z

Fixes #2445 Azure/acr#33
Includes #3734

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

General Guidelines

The PR has modified HISTORY.rst with an appropriate description of the change (see Modifying change log).

Command Guidelines

Each command and parameter has a meaningful description.
Each new command has a test.

(see Authoring Command Modules)

codecov-io · 2017-06-16T00:35:05Z

Codecov Report

Merging #3747 into master will decrease coverage by 1.14%.
The diff coverage is 45.26%.

@@            Coverage Diff             @@
##           master    #3747      +/-   ##
==========================================
- Coverage   72.12%   70.98%   -1.15%     
==========================================
  Files         420      424       +4     
  Lines       26044    26890     +846     
  Branches     3945     4092     +147     
==========================================
+ Hits        18785    19087     +302     
- Misses       6042     6565     +523     
- Partials     1217     1238      +21

Impacted Files	Coverage Δ
...li-acr/azure/cli/command_modules/acr/credential.py	`70.58% <ø> (-3.1%)`	⬇️
...e-cli-acr/azure/cli/command_modules/acr/_format.py	`27.77% <0%> (-11.12%)`	⬇️
...e-cli-acr/azure/cli/command_modules/acr/_params.py	`100% <100%> (ø)`	⬆️
...-cli-acr/azure/cli/command_modules/acr/_factory.py	`100% <100%> (+11.11%)`	⬆️
...li-acr/azure/cli/command_modules/acr/_constants.py	`100% <100%> (ø)`	⬆️
...ure-cli-acr/azure/cli/command_modules/acr/_help.py	`100% <100%> (ø)`	⬆️
...-cli-acr/azure/cli/command_modules/acr/commands.py	`100% <100%> (ø)`	⬆️
...li-acr/azure/cli/command_modules/acr/repository.py	`17.26% <18.05%> (-34.67%)`	⬇️
src/azure-cli-core/azure/cli/core/_profile.py	`86.52% <20%> (-1.56%)`	⬇️
...acr/azure/cli/command_modules/acr/_docker_utils.py	`22% <22%> (ø)`
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b5e9e3...bddfd33. Read the comment docs.

djyou · 2017-06-16T00:40:16Z

/cc @sajayantony @DavidObando @ankurkhemani

djyou · 2017-06-16T00:42:58Z

/cc @listonb

sajayantony · 2017-06-16T01:55:38Z

LGTM

listonb · 2017-06-16T03:50:52Z

LGTM! Thanks for adding the docker exec check! Excellent additions to the cli!

derekbekoe

Questions about use of Docker in the Docker install of the CLI.

derekbekoe · 2017-06-16T18:10:45Z

Dockerfile

What scenario is this for? The user could run apk add docker if they need docker.
It adds close to 100MB to our Docker image.
https://pkgs.alpinelinux.org/package/v3.6/community/aarch64/docker
356MB -> 433MB

docker is needed for acr login command.

derekbekoe · 2017-06-16T18:27:24Z

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

Does this work inside the Docker install of Azure CLI?
I get this:

# docker login --username username --password "mypassword" Cannot connect to the Docker daemon. Is the docker daemon running on this host? #

I get that error message for almost all Docker commands.

You will need to run the container with

docker run -it -v /var/run/docker.sock:/var/run/docker.sock azuresdk/azure-cli-python

Also apk add docker if docker is not installed in the container.

Okay thanks for removing it from the Dockerfile.

derekbekoe

LGTM

lostintangent · 2017-06-18T01:20:25Z

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

Does this arg pick up the default registry name if it's configured? The help text below doesn't seem to indicate it does, and it would be ideal to have it be defaulted when the ACR default is set.

troydai · 2017-06-19T17:35:28Z

Please rebase your changes and resolve the conflicts.

* expose refresh token retrieving mechanism in profile * added unit tests for refresh token operations

Repository querying operations can use AAD now (#36) Renaming tokenserver user_type to credential_type (#37) Rename credential_type to grant_type (#38) Improve acr login strategy with fallbacks (#39)

troydai

Besides the questions regarding SDK - API version mechanism, I notice the code is formatted following a narrower line length limitation. The current line limitation is 120, you can fully utilize the room on one line to produce more condense code.

troydai · 2017-06-19T23:30:48Z

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



-def get_acr_service_client():
+def get_acr_service_client(api_version='2017-03-01'):


@derekbekoe

troydai · 2017-06-19T23:32:25Z

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

-    if customized_api_version:
-        logger.warning('Customized api-version is used: %s', customized_api_version)
-    return customized_api_version
+    return get_mgmt_service_client(ContainerRegistryManagementClient,


The line length limitation is 120 so you don't have to break the line here.

troydai · 2017-06-19T23:33:54Z

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

 from azure.cli.core.util import CLIError
 from azure.cli.core.commands.parameters import get_resources_in_subscription

+from azure.mgmt.containerregistry.v2017_03_01.models import SkuTier


Why not use

azure-cli/src/azure-cli-core/azure/cli/core/profiles/__init__.py

Line 50 in a6b8fd8

def get_sdk(resource_type, *attr_args, **kwargs):

@derekbekoe

We are using multiple api-version SDK to direct registry creation calls to different versions based on SKU. Basically, Basic SKU will go to 2017-03-01 version and Managed SKUs will go to 2017-06-01-preview. At this point there is no need to expose multiple api-versions to users and the user specified SKU is enough to determine which api-version to go.

@derekbekoe
In this case, when user set CLI to be executed on an older version of API profile, will these commands be hidden from users?

AFAIK, this PR is not related to API profile. We simply direct calls to different api-versions based on SKU selection, but I will let @derekbekoe comment.

Some API versions may be missing in Azure Stack or different cloud. Will the command fail in those environments?

@troydai since @djyou says there is no need to expose multiple api-versions to the users, it's fine to import a specific version. It won't work on Azure Stack but I assume that was not intended anyway.

troydai · 2017-06-20T02:50:28Z

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

 from azure.cli.core.util import CLIError
 from azure.cli.core.commands.parameters import get_resources_in_subscription

+from azure.mgmt.containerregistry.v2017_03_01.models import SkuTier


@derekbekoe
In this case, when user set CLI to be executed on an older version of API profile, will these commands be hidden from users?

troydai · 2017-06-20T02:51:08Z

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

    """
-    arm_resource = _arm_get_resource_by_name(registry_name, ACR_RESOURCE_TYPE)
-    return get_resource_group_name_by_resource_id(arm_resource.id)
+    if resource_group_name is None:


if not resource_group_name

Unless an empty group name is legal, which I don't think should be the case.

troydai · 2017-06-20T02:51:20Z

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

+    :param str storage_account_name: The name of storage account
+    :param str resource_group_name: The name of resource group
+    """
+    if resource_group_name is None:


troydai · 2017-06-20T02:52:40Z

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

+    arm_resource = _arm_get_resource_by_name(registry_name, ACR_RESOURCE_TYPE)
+
+    if arm_resource.sku.tier == SkuTier.basic.value:
+        raise CLIError(message if message else "This operation is not supported for registries in Basic SKU.")


raise CLIError(message or "This operation is not supported for registries in Basic SKU.")

troydai · 2017-06-20T02:56:29Z

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

        )
-    instance.storage_account = storage_account if storage_account_name else None
+
+    if admin_enabled is not None:


if admin_enable. In this case, when admin_enable is empty, it must yield negative result.

admin_enabled has a choice of either true or false and it does yield negative result if it is empty.

ok. but it is actually weird that a bool parameter is not represented by a "switch" option.

If I understand correctly, we will need a pair of switch options support both enable and disable admin user. We used to have it implemented that way (in fact in Powershell cmdlets we still have a pair of switch parameters) but was updated to the current implementation as suggested.

troydai · 2017-06-20T02:57:02Z

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

+    if admin_enabled is not None:
+        instance.admin_user_enabled = admin_enabled == 'true'
+
+    if tags is not None:


Is tag allowed to be an empty string? Otherwise, use if tags

Yes, empty clears tags.

troydai · 2017-06-20T02:57:23Z

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

+    """
+    registry, resource_group_name = get_registry_by_name(registry_name, resource_group_name)
+
+    if parameters.storage_account is not None and registry.sku.name != SkuTier.basic.value:  # pylint: disable=no-member


if parameters.storage_account and ...

Right now, if the specified storage account name is empty, we error out by saying that the storage account (with empty name) couldn't be found, which I think is the right behavior. If we make it the way you suggested, the command will go through but ignore the specified storage account. I personally think empty name is a negative scenario and should error out rather than ignore, what is the guideline here?

The truthiness of an empty string is False. Both None and empty in my opinion should render the same failed result.

troydai · 2017-06-20T02:57:29Z

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

+        logger.warning("'%s' SKU are managed registries. " +
+                       "The specified storage account will be ignored.", registry.sku.name)  # pylint: disable=no-member
+
+    if parameters.storage_account is not None and isinstance(parameters.storage_account, dict):


Same as above.

troydai · 2017-06-20T02:57:55Z

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

+
+    # 1. if username was specified, verify that password was also specified
+    if username:
+        if not password:


if username and not password:

troydai · 2017-06-20T02:59:05Z

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

+
+
+def acr_webhook_list(registry_name,
+                     resource_group_name=None):


troydai · 2017-06-20T03:27:44Z

Most of my comments are about the truthiness in the condition when a string is in place. They're all about whether the empty strings should be treated as the legal values. Other than that the one outstanding question is whether the commands with a hard requirement of api version will be hidden when user switch to an older API profile. That is the case in Azure Stack.

derekbekoe

LGTM

derekbekoe · 2017-06-20T23:02:52Z

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

 from azure.cli.core.util import CLIError
 from azure.cli.core.commands.parameters import get_resources_in_subscription

+from azure.mgmt.containerregistry.v2017_03_01.models import SkuTier


@troydai since @djyou says there is no need to expose multiple api-versions to the users, it's fine to import a specific version. It won't work on Azure Stack but I assume that was not intended anyway.

msftclas added the cla-not-required label Jun 15, 2017

derekbekoe reviewed Jun 16, 2017

View reviewed changes

derekbekoe approved these changes Jun 16, 2017

View reviewed changes

lostintangent reviewed Jun 18, 2017

View reviewed changes

djyou and others added 3 commits June 19, 2017 11:34

Initial acr login command

c80c024

* expose refresh token retrieving mechanism in profile * added unit tests for refresh token operations

[ACR] Improvement to acr login with AAD

5f7cdbc

Repository querying operations can use AAD now (#36) Renaming tokenserver user_type to credential_type (#37) Rename credential_type to grant_type (#38) Improve acr login strategy with fallbacks (#39)

Add managed registries, webhooks, AAD, and repository delete

fa086a3

djyou force-pushed the master branch from 9bad530 to fa086a3 Compare June 19, 2017 18:46

troydai reviewed Jun 19, 2017

View reviewed changes

Fix line breakings

689d525

troydai suggested changes Jun 20, 2017

View reviewed changes

PR review updates

bddfd33

troydai approved these changes Jun 20, 2017

View reviewed changes

derekbekoe approved these changes Jun 20, 2017

View reviewed changes

troydai merged commit 855f123 into Azure:master Jun 20, 2017

derekbekoe mentioned this pull request Sep 9, 2017

Command acr login fails in docker container #4407

Closed

djyou mentioned this pull request Jan 18, 2018

acr says "Please verify if docker daemon is running properly." despite docker daemon running #5075

Closed

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



		def get_acr_service_client():
		def get_acr_service_client(api_version='2017-03-01'):



		def acr_webhook_list(registry_name,
		resource_group_name=None):

[ACR] Add managed registries, webhooks, AAD auth, and repository delete #3747

[ACR] Add managed registries, webhooks, AAD auth, and repository delete #3747

Uh oh!

Conversation

djyou commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General Guidelines

Command Guidelines

Uh oh!

codecov-io commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

djyou commented Jun 16, 2017

Uh oh!

djyou commented Jun 16, 2017

Uh oh!

sajayantony commented Jun 16, 2017

Uh oh!

listonb commented Jun 16, 2017

Uh oh!

derekbekoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekbekoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

troydai commented Jun 19, 2017

Uh oh!

troydai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djyou commented Jun 15, 2017 •

edited

Loading

codecov-io commented Jun 16, 2017 •

edited

Loading