Add ACR Image Import Commands by NatalieGong · Pull Request #6375 · Azure/azure-cli

NatalieGong · 2018-05-17T17:30:12Z

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.

promptws · 2018-05-17T17:30:13Z

View a preview at https://prompt.ws/r/Azure/azure-cli/6375
This is an experimental preview for @microsoft users.

codecov-io · 2018-05-17T19:10:25Z

Codecov Report

Merging #6375 into dev will not change coverage.
The diff coverage is n/a.

@@         Coverage Diff         @@
##           dev   #6375   +/-   ##
===================================
  Coverage    0%      0%           
===================================
  Files       11      11           
  Lines      133     133           
  Branches     9       9           
===================================
  Misses     133     133

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 2cd0ed3...fbd1a60. Read the comment docs.

djyou

Just some minor comments.

djyou · 2018-05-17T20:37:03Z

NIT: command

djyou · 2018-05-17T20:38:15Z

Can you also add this Case 3 to help -> examples?

djyou · 2018-05-17T20:40:54Z

These two lines can be combined to elif not resource_id?

djyou · 2018-05-17T21:03:27Z

Check if this API works with Classic registry, remove this validation if so.

tjprescott · 2018-05-18T16:38:15Z

Will there be additional commands within the acr image group? If not, this should be acr import-image or potentially acr import. If there will be additional commands in this group (even if its a future PR) then an image group is appropriate.

How is this different from the repository group which says "Manage repositories (image names) for Azure Container Registries"?

ping @NatalieGong

tjprescott · 2018-05-18T16:40:54Z

Is this basically copying from one registry to another or is it from any source to an ACR? It sounds like "copy" might be the more appropriate verb than "import". Import usually implies converting a file into resource, so there's a transformation. This sounds like just copying an image into the registry. But I could be misinterpreting.

Import images from registries is the start. I guess we will add import from other places as docker hub etc.

tjprescott · 2018-05-18T16:42:56Z

I'm a little confused by this argument. Why --image and not --tags?

Changed to target-tags

tjprescott · 2018-05-18T16:43:10Z

"manifest-only"

tjprescott · 2018-05-18T16:52:33Z

Is the container registry referred to here always another ACR resource? If so, this could just be called 'registry' or 'source_registry'. It should also support names or IDs. https://github.com/Azure/azure-cli/blob/dev/doc/authoring_command_modules/authoring_commands.md#supporting-name-or-id-parameters

The resource id is used for talking with ARM when doing cross subscription importing. I believe customers have access to it.

It still needs to support the name-or-id paradigm of the CLI. If it's a generic resource it's a bit more annoying than a strongly-typed resource like another ACR.

It needs to enable our client side CLI to lookup resources cross subscription.

To summarize the discussion we had with @djyou
The goal is to make --source a required parameter which can be a partial image name and have -r as a parameter which will be used to derive the source which can be a name or dns or fully fully qualified resource id.

tjprescott · 2018-05-18T16:53:25Z

Why is this? Are there ever more than 1? If so, why return nothing in that case?

Good catch. Fixed it.

tjprescott · 2018-05-18T16:54:13Z

I believe prompt already handles the "no TTY" case.

Nope it does not.

It does in that it throws the NoTTYException, which you handle here.

tjprescott · 2018-05-18T16:54:50Z

The CLI will already wrap this in a LRO. You can just return client.import_image(....)

tjprescott · 2018-05-18T16:56:19Z

Since you are adding the first relevant item for this release, you can remove "Minor fix" above. Also, recommend:

* Added `acr image import` command.

troydai

Aside from minor comments, looks good to me.

troydai · 2018-05-21T16:55:10Z

2.0.25 ++++++ * Minor Fix * Add 'acr import' command.

troydai · 2018-05-21T16:56:57Z

Unnecessary variable.

troydai · 2018-05-21T16:58:21Z

Should this be a warning instead of a debug? Debug level logging is usually buried deeply. If this is a message impacting customer's decision process, it should be more visible.

troydai · 2018-05-21T17:00:03Z

Unnecessary else since the raise will terminate the function.

sajayantony · 2018-05-25T16:55:43Z

can you just make it consistent to -n registry for all examples

sajayantony · 2018-05-25T16:58:27Z

@djyou why are we using _ for parameter name seperators shouldn't this be --target-tags
Either way does not align with --image which is what build uses and I think we need to have the same convention here.

* 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 (#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 * . * .

* Make util method more generic, update help * Improve examples

* 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 * Test cases for image import * Test cases for image import * Naming

djyou

Just a few minor comments.

djyou · 2018-05-30T23:34:56Z

+    examples:
+        - name: Import an image to the target registry and inherits repository:tag from source.
+          text: >
+            az acr import -n MyRegistry --source SourceRegistry.azurecr.io/SourceRepository:SourceTag


At least lower case repository name as it can only be lower case. I would recommend lower case login server and tag also. Tag is case sensitive.

Lower-cased

djyou · 2018-05-30T23:35:12Z

+    with self.argument_context('acr import') as c:
+        c.argument('source', help="A source identifier, either fully qualified (e.g. 'registry.azurecr.io/repository:tag') or partially qualified (e.g. 'repository@sha').")
+        c.argument('source_registry', options_list=['--registry', '-r'], help='The container registry of the source. It can be name, canonical name (login server) or resource ID of the source registry.')
+        c.argument('target_tags', options_list=['--target-tags', '-t'], help="The repository and optionally a tag in the 'repository:tag' format for target images.", action='append')


It is shortened as '-t'

As the variable itself implies "target_tags"

djyou · 2018-05-30T23:35:37Z

        c.argument('no_logs', help="Do not show logs after successfully queuing the build.", action='store_true')

+    with self.argument_context('acr import') as c:
+        c.argument('source', help="A source identifier, either fully qualified (e.g. 'registry.azurecr.io/repository:tag') or partially qualified (e.g. 'repository@sha').")


NIT: add a comma after e.g.

djyou · 2018-05-30T23:36:24Z

+)
+
+SOURCE_REGISTRY_MISING = "Please specify the source container registry: "
+IMPORT_NOT_SUPPORTED = "Imports are only supported for managed registries."


NIT: Import is ...

The word "Imports" is paired with "Managed registries"

djyou · 2018-05-30T23:37:02Z

+
+SOURCE_REGISTRY_MISING = "Please specify the source container registry: "
+IMPORT_NOT_SUPPORTED = "Imports are only supported for managed registries."
+INVALID_SOURCE_IMAGE = "Please specify source image in the form of '[registry.azurecr.io/]repository:tag' or 'repository@sha'."


[registry.azurecr.io/]repository[:tag] or [registry.azurecr.io/]repository@sha?

Lines too long and also I have included the example for import-by-sha in the az acr import -h

djyou · 2018-05-30T23:38:32Z

+            except NoTTYException:
+                raise CLIError(NO_TTY_ERROR)
+        registry_by_registry_name = get_registry_from_name(cmd.cli_ctx, source_registry)
+        registry_by_login_server = get_registry_from_name(cmd.cli_ctx, source_registry)


Can you not call get_registry_from_name twice?

djyou · 2018-05-30T23:39:53Z

+            except NoTTYException:
+                raise CLIError(NO_TTY_ERROR)
+
+        source_image = source[slash + 1:]


Also move this validation earlier.

NatalieGong · 2018-05-31T00:42:33Z

@djyou @tjprescott @troydai @sajayantony @yugangw-msft

djyou · 2018-05-31T01:07:27Z


+    with self.argument_context('acr import') as c:
+        c.argument('source', help="A source identifier, either fully qualified (e.g., 'registry.azurecr.io/repository:tag') or partially qualified (e.g., 'repository@sha').")
+        c.argument('source_registry', options_list=['--registry', '-r'], help='The container registry of the source. It can be name, canonical name (login server) or resource ID of the source registry.')


Are we introducing canonical name to CLI now? /cc @sajayantony

troydai · 2018-05-31T05:19:09Z

@tjprescott this is for this sprint, please take another look.

djyou · 2018-05-31T05:06:58Z

        c.argument('no_logs', help="Do not show logs after successfully queuing the build.", action='store_true')

+    with self.argument_context('acr import') as c:
+        c.argument('source', help="The source identifier in the format '[registry.azurecr.io/]repository:tag' or '[registry.azurecr.io/]repository@digest'.")


'[registry.azurecr.io/]repository[:tag]'

I do not think tag is optional for source...unless you want to import a collection of images rather than a single one, do you?

Could you please validate? Think doesn't help here ...

FYI https://github.com/Azure/azure-rest-api-specs/blob/master/specification/containerregistry/resource-manager/Microsoft.ContainerRegistry/stable/2017-10-01/containerregistry.json#L1117

What if somebody is so unlucky that there is no latest tag in that repository he/she specifies?

It is possible right? Like all the images either built or imported into the repository were not defaulted to latest when they were born in that repository.

How do you think users might expect under such situation?

Optionalized the tag

djyou · 2018-05-31T05:07:30Z

+
+helps['acr import'] = """
+    type: command
+    short-summary: Imports to the container registry from source.


NIT: Imports an image ...?

djyou · 2018-05-31T05:07:55Z


+    with self.argument_context('acr import') as c:
+        c.argument('source', help="The source identifier in the format '[registry.azurecr.io/]repository:tag' or '[registry.azurecr.io/]repository@digest'.")
+        c.argument('source_registry', options_list=['--registry', '-r'], help='The container registry of the source. It can be name, login server or resource ID of the source registry.')


NIT: The source container registry.

djyou · 2018-05-31T05:09:10Z

+
+    if registry_name:
+        elements = [item for item in registry_list if
+                    item.login_server.lower() == login_server.lower() or item.name.lower() == registry_name.lower()]


What if login_server is None?

login_server will not be None guaranteed by the caller

How does the caller even know from the method signature without reading its implementation?

Line 53:
It promises a none empty value for source_registry because the value will be either from the command directly or from the interactivity by prompt. Again, as the client side, we cannot validate it however we can guarantee a nonempty value.

Similar for line 61 but the reason differs a little. If the source_registry_login_server is empty or none, an CLIError has already be thrown before reaching the call.

But you are shipping an open source project with a util method that can possibly be used by anyone and it's clearly broken if called otherwise.

Does caller have to handle the exception if login_server is None?

I have done validation on the parameter for registry_name

For the login server, yes, caller is the layer to handle exceptions before the actual call.

I think this method should be generic enough to accept either name or login_server (i.e., both should be default to None). Just use the non-empty ones to compare and filter the results.

I am hoping this method can further be reused to support scenarios such as acr login -n registry.azurecr.io.

For the login server, yes, caller is the layer to handle exceptions before the actual call.

It makes no sense to me that your method takes a None value and doesn't handle it but throws a null reference exception out and wishes the caller can handle it.

I mean the caller throws an exception when the login server is empty before the method actually gets called. Please see Line 59 for now. Btw I will combine the two errors together as you suggested :-)

djyou · 2018-05-31T05:38:18Z

+    if not elements:
+        return None
+    elif len(elements) > 1:
+        logger.warning("More than one registry is found by %s.", login_server)


What if login_server is None?

djyou · 2018-05-31T05:38:48Z

+
+SOURCE_REGISTRY_MISING = "Please specify the source container registry name, login server or resource ID: "
+IMPORT_NOT_SUPPORTED = "Imports are only supported for managed registries."
+INVALID_SOURCE_IMAGE = "Please specify source image in the form of '[registry.azurecr.io/]repository:tag' or " \


'[registry.azurecr.io/]repository[:tag]', also the source image?
Could you check if tag is optional or not?

Tag for the target image is optional. For the source, I do not think so.

I recall the server side attaches a latest if tag is empty, could you validate?

Why is IMPORT_NOT_SUPPORTED a client side check. I think we should move this to the server and throw if called on a classic registry. - @nathana1

Wow really? I am so curious. Can you explain more on what you meant by moving to the server? As I have checked all the modules in the commands including build-task, custom, repository and web hook. All of them seem to use the validate_managed_registries and have defined a static constant to be XXX_NOT_SUPPORTED

djyou · 2018-05-31T05:41:20Z

+            raise CLIError(INVALID_SOURCE_IMAGE)
+        source_registry_login_server = source[:slash]
+        if not source_registry_login_server:
+            raise CLIError(INVALID_SOURCE_IMAGE)


source_registry_login_server = source[:slash] source_image = source[slash + 1:] if not source_registry_login_server or not source_image: raise CLIError(INVALID_SOURCE_IMAGE)

djyou · 2018-05-31T05:42:58Z

+                source_registry = prompt(SOURCE_REGISTRY_MISING)
+            except NoTTYException:
+                raise CLIError(NO_TTY_ERROR)
+        registry_from_name = get_registry_from_name(cmd.cli_ctx, source_registry, source_registry)


NIT: I think you can just call this registry since it's an registry object and isn't necessarily obtained from name (can be login server too).

djyou · 2018-05-31T05:49:29Z

+        registry_from_name = get_registry_from_name(cmd.cli_ctx, source_registry_login_server)
+
+    if registry_from_name:
+        if source_registry and registry_from_name.id != source_registry:


source_registry can be specified as a name and this validation will fail. I think you need to use the is_valid_resource_id method to differentiate the two cases anyway.

No. I do not think so.

But thanks for pointing this out as you helped me recall the reason why I need the two lines:
if registry_from_name:
source_registry = registry_from_name.id

They are necessary because it ensures that when the control flow reaches Line 64, source_registry can only be none or resource id

…n, or none of them should. (inconsistent-return-statements)

tjprescott

LGTM. I still don't get the --image/-t pairing, but I'll leave that up to you.

tjprescott · 2018-05-31T19:56:33Z

+    with self.argument_context('acr import') as c:
+        c.argument('source', help="The source identifier in the format '[registry.azurecr.io/]repository[:tag]' or '[registry.azurecr.io/]repository@digest'.")
+        c.argument('source_registry', options_list=['--registry', '-r'], help='The source container registry can be name, login server or resource ID of the source registry.')
+        c.argument('target_tags', options_list=['--image', '-t'], help="The repository and optionally a tag in the 'repository:tag' format for target images.", action='append')


I'm still confused by this aliasing. Why --image and -t? Why not -i?

This is to align with docker's convention as well as build-task.

djyou · 2018-05-31T20:03:40Z

+    get_registry_from_name
+)
+
+SOURCE_REGISTRY_MISING = "Please specify the source container registry name, login server or resource ID: "


SOURCE_REGISTRY_MISSING

djyou · 2018-05-31T20:04:39Z

+            raise CLIError(INVALID_SOURCE_IMAGE)
+        registry = get_registry_from_name(cmd.cli_ctx, source_registry_login_server)
+        if source_registry:
+            registry_from_source = get_registry_from_name(cmd.cli_ctx, source_registry, source_registry)


Please avoid calling this list operation again, it's an expensive operation that fans out to all regions. You can call it once but filter twice.

djyou · 2018-05-31T20:05:19Z

+                source_registry = prompt(SOURCE_REGISTRY_MISING)
+            except NoTTYException:
+                raise CLIError(NO_TTY_ERROR)
+        registry_from_source = get_registry_from_name(cmd.cli_ctx, source_registry, source_registry)


If source_registry is already a resource ID, this request would be a waste since it must return None, or you need to support get registry from resource ID.

troydai · 2018-05-31T21:22:20Z

Please fix the CI before the end of the day so we can merge this in time for this release.

djyou

LGTM with one minor comment.

djyou · 2018-05-31T21:48:48Z

+                raise CLIError(NO_TTY_ERROR)
+        registry_from_source = get_registry_from_name_or_login_server(cmd.cli_ctx, source_registry, source_registry)
+        if registry_from_source:
+            source_registry = registry_from_source.id


Add a condition to execute the above 3 lines only if source_registry is not a valid resource id.

djyou reviewed May 17, 2018

View reviewed changes

This was referenced May 17, 2018

Enable limiting the number of manifests shown by "az acr repository show-manifest" command AzureCR/azure-cli#150

Closed

Align commands with az acr build and docker build to mult-tagging during import AzureCR/azure-cli#149

Closed

NatalieGong force-pushed the natalie-release branch 2 times, most recently from 5a81e23 to 8b24143 Compare May 18, 2018 00:26

tjprescott added New Command(s) Container Registry az acr labels May 18, 2018

tjprescott added this to the Sprint 38 milestone May 18, 2018

tjprescott suggested changes May 18, 2018

View reviewed changes

tjprescott reviewed May 18, 2018

View reviewed changes

troydai reviewed May 21, 2018

View reviewed changes

NatalieGong force-pushed the natalie-release branch 2 times, most recently from 7e8a2d3 to 2fb84f0 Compare May 23, 2018 16:50

sajayantony suggested changes May 25, 2018

View reviewed changes

NatalieGong requested a review from yugangw-msft May 30, 2018 01:08

Natalie and others added 13 commits May 29, 2018 18:57

Make util method more generic, update help (#154)

7534a3e

* Make util method more generic, update help * Improve examples

Update history and version

6d01a94

Add license headers

a610d54

Naming

149e15f

Fix for minor comments

c0ef9fb

Update version

cba9b5c

Version

5e7073b

Move import to commands under ACR

ffdec16

Add blank lines

2fe74d3

Version 2.0.25 of azure-cli-acr is already usef on in the base repo

5e769b5

Address feedback

3b43176

djyou reviewed May 30, 2018

View reviewed changes

nataliegong added 2 commits May 30, 2018 16:47

Lines too long

7199d03

Combine get_registry from registry name and login server

ecf271b

NatalieGong closed this May 31, 2018

NatalieGong reopened this May 31, 2018

djyou reviewed May 31, 2018

View reviewed changes

nataliegong added 2 commits May 30, 2018 18:36

Legacy problems

ec47a5a

Revisit name-ID supoort

ce70203

djyou reviewed May 31, 2018

View reviewed changes

nataliegong added 3 commits May 31, 2018 10:28

Validation on resource id

6e4d586

Either all return statements in a function should return an expressio…

4db9eb3

…n, or none of them should. (inconsistent-return-statements)

Inconsistent return type

9f03f05

tjprescott approved these changes May 31, 2018

View reviewed changes

djyou reviewed May 31, 2018

View reviewed changes

nataliegong added 2 commits May 31, 2018 13:26

Reduce a global search for registries

5e95d4f

Merge remote-tracking branch 'upstream/dev' into natalie-release

2e2bb13

nataliegong added 2 commits May 31, 2018 14:31

Fix CI

808160e

Indentation

22d971d

djyou approved these changes May 31, 2018

View reviewed changes

Filter resource ID

fbd1a60

troydai merged commit dbe2440 into Azure:dev May 31, 2018

NatalieGong deleted the natalie-release branch June 13, 2018 20:46

haroldrandom added ACR New Command(s) labels Oct 25, 2019

ghost added Container Registry az acr and removed ACR labels Jun 25, 2020

Conversation

NatalieGong commented May 17, 2018

Uh oh!

promptws commented May 17, 2018

Uh oh!

codecov-io commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

djyou 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!

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!

tjprescott May 18, 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!

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

codecov-io commented May 17, 2018 •

edited

Loading

tjprescott May 18, 2018 •

edited

Loading

djyou May 30, 2018 •

edited

Loading