Image import handles more scenarios by NatalieGong · Pull Request #152 · AzureCR/azure-cli

NatalieGong · 2018-05-15T03:13:57Z

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

The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.

Merge changes from upstream

* Add support for image import * Enable image import commands * Enable Image Import Commands * Error handling * Rewrite acr_image_import + Help messages * Image import without support of docker hub and cross-subscription registires * Listify tags and repository arguments for image import * Support import image cross subscription * Help message for arguments

djyou · 2018-05-15T04:17:41Z

        c.argument('source_image', help='A fully qualified image identifier.')
-        c.argument('tags', nargs='+', options_list=['--image', '-t'], help="Space-separated list of images in the 'repository:tag' format where tag is optional.")
-        c.argument('repository', nargs='+', help='Space-separared list of image repository without tag to do a manifest only copy.')
+        c.argument('target', options_list=['--image', '-t'], help="The image repository and optionally a tag in the 'repository:tag' format where tag is optional.", action='append')


You can just use target_tags from the SDK since there is an options_list to override it.

The help doesn't read right. Can you reuse the same help from build-task --image?

djyou · 2018-05-15T04:19:20Z


 IMPORT_NOT_SUPPORTED = "Image imports are only supported for managed registries."
-INVALID_SOURCE_IMAGE = "Please specify source image in the form of 'regsitry.azurecr.io/repository[:tag]'."
+INVALID_SOURCE_IMAGE = "Please specify source image in the form of 'registry.azurecr.io/repository[:tag]"


Missing an ending single quote.

djyou · 2018-05-15T04:19:59Z

 IMPORT_NOT_SUPPORTED = "Image imports are only supported for managed registries."
-INVALID_SOURCE_IMAGE = "Please specify source image in the form of 'regsitry.azurecr.io/repository[:tag]'."
+INVALID_SOURCE_IMAGE = "Please specify source image in the form of 'registry.azurecr.io/repository[:tag]"
+SOURCE_REGISTRY_NOT_FOUND = "Source registry cannot be found in the current subscription. Please specify the full resource id for it:"


resource ID

NIT: add a space after :.

djyou · 2018-05-15T04:20:47Z

 def acr_image_import(cmd,
                     client,
-                     registry_name,
+                     registry_name,                     


trailing whitespace.

djyou · 2018-05-15T04:20:58Z

+        try:
+            resource_id = get_resource_id_by_registry_name(cmd.cli_ctx, source_registry)
+        except CLIError:
+            resource_id = input(SOURCE_REGISTRY_NOT_FOUND)


Handle non-tty

djyou · 2018-05-15T04:22:02Z

-        resource_id = get_resource_id_by_registry_name(cmd.cli_ctx, source_registry)
+        try:
+            resource_id = get_resource_id_by_registry_name(cmd.cli_ctx, source_registry)
+        except CLIError:


I guess you only want to catch when registry couldn't be found, but throw other errors out.

djyou · 2018-05-15T20:56:32Z

-        c.argument('tags', nargs='+', options_list=['--image', '-t'], help="Space-separated list of images in the 'repository:tag' format where tag is optional.")
-        c.argument('repository', nargs='+', help='Space-separared list of image repository without tag to do a manifest only copy.')
+        c.argument('target_tags', options_list=['--image', '-t'], help="The image repository and optionally a tag in the 'repository:tag' format.", action='append')
+        c.argument('repository', help='The repository names to do a manifest only copy.', action='append')


The repository name?

djyou · 2018-05-15T20:56:43Z

                    result = list(result)

-                result = todict(result, AzCliCommandInvoker.remove_additional_prop_layer)
+                result = todict(result)


Why do you need to modify core?

Revert this change, rebase from dev branch, create a new venv, run dev setup again, this problem should go away.

djyou · 2018-05-15T21:01:31Z

          text: >
-            az acr image import -n yugongTest --resource-id /subscriptions/dfb63c8c-7c89-4ef8-af13-75c1d873c895/resourceGroups/yugong/providers/Microsoft.ContainerRegistry/registries/yugongeast --source-image yugongeast.azurecr.io/builder:latest -t test:test .
+            az acr image import -n targetRegistry --resource-id /subscriptions/dfb63c8c-7c89-4ef8-af13-75c1d873c895/resourceGroups/yugong/providers/Microsoft.ContainerRegistry/registries/yugongeast --source-image yugongeast.azurecr.io/builder:latest
+            -t test:test


Replace your names with some common strings, also the sub id with empty guid, see other examples.

djyou · 2018-05-15T21:02:52Z

-        resource_id = get_resource_id_by_registry_name(cmd.cli_ctx, source_registry)
-    source_image = source_image[slash + 1 :]
+        resource_id = get_resource_id_by_registry_name(cmd.cli_ctx, source_registry, SOURCE_REGISTRY_NOT_FOUND)
+    registry_name_from_resource_id = get_registry_name_by_resource_id(


Don't we already have registry name from source_registry?

djyou · 2018-05-15T21:03:29Z

-    if tags is None and repository is None:
-        tags = [source_image]
+    image_source = ImportSource(
+        resource_id=resource_id, source_image=source_image)


Revert this line break. Line length max is 120.

djyou · 2018-05-15T21:05:25Z

+        resource_id = get_resource_id_by_registry_name(cmd.cli_ctx, source_registry, SOURCE_REGISTRY_NOT_FOUND)
+    registry_name_from_resource_id = get_registry_name_by_resource_id(
+        resource_id)
+    if source_registry != registry_name_from_resource_id:


Not getting this condition. resource_id is obtained from source_registry, registry_name_from_resource_id is obtained from resource_id, shouldn't registry_name_from_resource_id always match source_registry?

djyou · 2018-05-15T21:09:38Z

+
+    if not elements or len(elements) != 1:
+        try:
+            resource_id = prompt(msg)


This method would be confusing here, at least it shouldn't be in _utils as it won't be used by any other commands due to this special handling.

I would suggest that you do a get_resource_id_by_registry_name but instead of throwing, returning an empty result so you know the registry doesn't exist in the subscription.

djyou · 2018-05-15T23:47:34Z

+    """Returns the resource ID for the container registry.
+    :param str login_server: The login server of the container registry.
+    """
+    from .custom import acr_list


No need to import. Call client.list directly here.

djyou · 2018-05-15T23:48:06Z

+    """
+    from .custom import acr_list
+    registry_list = acr_list(client)
+    elements = [item for item in registry_list if item.login_server.lower() == login_server.lower()]


Create a var = login_server.lower() and reuse it, rather than calling lower() every time.

djyou · 2018-05-15T23:48:26Z

+    elements = [item for item in registry_list if item.login_server.lower() == login_server.lower()]
+
+    if not elements or len(elements) != 1:
+        return ""


edit: rename this method to get_registry_by_login_server and return the Registry object if it exists in the current sub, None otherwise, so that this method can be generic enough to be used by others.

djyou · 2018-05-15T23:53:40Z

@@ -1,54 +1,69 @@
 from knack.util import CLIError
+from knack.prompting import prompt, NoTTYException


Move this line to where they are used for better perf.

djyou · 2018-05-15T23:53:48Z


 from azure.mgmt.containerregistry.v2017_10_01.models import SkuName, Sku

+#from .custom import acr_list


djyou · 2018-05-15T23:55:55Z

+
+    if resource_id and resource_id_from_login_server and resource_id_from_login_server != resource_id:
+        raise CLIError("Registry mismatch. Please check either source-image or resource ID " \
+                       "to make sure that they are referring to the same registry and give another try.")


NIT: and try again.

* Enable Image Import Commands (#148) * Add support for image import * Enable image import commands * Enable Image Import Commands * Error handling * Rewrite acr_image_import + Help messages * Image import without support of docker hub and cross-subscription registires * Listify tags and repository arguments for image import * Support import image cross subscription * Help message for arguments * Enable Image Import Commands with Prompt for resource_id * Add help message * . * Move tty to _utils * Enable Image Import Commands with Prompt for resource_id * Add help message * . * Move tty to _utils * Move prompt out of _util * . * Validate using login server * . * .

djyou and others added 2 commits May 10, 2018 10:47

Merge pull request #146 from Azure/dev

515b71d

Merge changes from upstream

NatalieGong requested a review from djyou as a code owner May 15, 2018 03:13

Enable Image Import Commands with Prompt for resource_id

c409f36

NatalieGong force-pushed the natalie/image-import-advanced branch from ee791f7 to c409f36 Compare May 15, 2018 03:26

nataliegong added 2 commits May 14, 2018 20:41

Add help message

53c0a9f

.

c42a637

djyou suggested changes May 15, 2018

View reviewed changes

Move tty to _utils

2a4656b

djyou force-pushed the dev branch from 7fda03b to 5497d88 Compare May 15, 2018 20:59

djyou suggested changes May 15, 2018

View reviewed changes

nataliegong added 9 commits May 15, 2018 14:12

Merge branch 'dev' of https://github.com/AzureCR/azure-cli into dev

e9304a3

Enable Image Import Commands with Prompt for resource_id

ac93d37

Add help message

1ee5a36

.

712ef6d

Move tty to _utils

d65c698

Move prompt out of _util

080d074

Rebase master

5fa4bec

.

5e71406

Validate using login server

501a319

djyou reviewed May 15, 2018

View reviewed changes

.

239b851

djyou approved these changes May 16, 2018

View reviewed changes

.

734b453

NatalieGong merged commit 9309805 into dev May 16, 2018

NatalieGong deleted the natalie/image-import-advanced branch May 16, 2018 03:15

		@@ -1,54 +1,69 @@
		from knack.util import CLIError
		from knack.prompting import prompt, NoTTYException


		from azure.mgmt.containerregistry.v2017_10_01.models import SkuName, Sku

		#from .custom import acr_list

Conversation

NatalieGong commented May 15, 2018

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 May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

djyou May 15, 2018 •

edited

Loading

djyou May 15, 2018 •

edited

Loading