[ACR] Improve perf and fix permission in repository commands #4566

djyou · 2017-09-28T06:03:30Z

Avoid making list-manifests call when users wish to force delete a manifest either by digest or tag.
Fix an issue with AAD authentication when logged in using read-access-only role service principal.

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

General Guidelines

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).

Command Guidelines

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

(see Authoring Command Modules)

djyou · 2017-09-28T06:22:04Z

/cc @sajayantony

sajayantony · 2017-09-28T07:43:56Z

/cc @yuwaMSFT2

sajayantony · 2017-09-28T07:47:17Z

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

    if username is None:
        auth = _bearer_auth_str(password)
    else:
        auth = _basic_auth_str(username, password)

+    if is_manifest:


Can you break this out into its own function like AddManifestHeader

troydai · 2017-09-28T16:18:43Z

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

-                               retry_times=3,  # pylint: disable=unused-argument
-                               retry_interval=5):  # pylint: disable=unused-argument
-    registryEndpoint = 'https://' + login_server
+def _params(count=20):


This function name is too generic and will lead to ambiguity. Please make it more accurate.

troydai · 2017-09-28T16:20:08Z

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

-    else:
-        raise CLIError(response.text)
+def _delete_data_from_registry(login_server, path, username, password, **kwargs):
+    retry_times = kwargs['retry_times'] if 'retry_times' in kwargs else 3


If retry_times is a value given by user through command line option, the default value should be defined with the parameter.

Otherwise, why not list retry_times and retry_interval as the parameter in the method signature and define default value along with it?

troydai · 2017-09-28T16:20:15Z

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

-        raise CLIError(response.text)
+def _delete_data_from_registry(login_server, path, username, password, **kwargs):
+    retry_times = kwargs['retry_times'] if 'retry_times' in kwargs else 3
+    retry_interval = kwargs['retry_interval'] if 'retry_interval' in kwargs else 5


Same as above.

troydai · 2017-09-28T16:23:58Z

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


+    if errorMessage:


Consider user for ... else in this situation: https://docs.python.org/2/tutorial/controlflow.html#break-and-continue-statements-and-else-clauses-on-loops

troydai · 2017-09-28T16:25:55Z

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

+                headers=_headers(username, password)
+            )
+
+            if response.status_code == 200 or response.status_code == 202:


Is this condition too rigorous? Do the other status code in 2XX represents a failure?

troydai · 2017-09-28T16:27:36Z

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

+def _obtain_data_from_registry(login_server, path, username, password, **kwargs):
+    result_index = kwargs['result_index'] if 'result_index' in kwargs else ''
+    retry_times = kwargs['retry_times'] if 'retry_times' in kwargs else 3
+    retry_interval = kwargs['retry_interval'] if 'retry_interval' in kwargs else 5


Same as above.

troydai · 2017-09-28T16:27:52Z

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

@@ -130,9 +146,43 @@ def _obtain_data_from_registry(login_server,
    return resultList


+def _get_manifest_digest(login_server, path, username, password, **kwargs):
+    retry_times = kwargs['retry_times'] if 'retry_times' in kwargs else 3
+    retry_interval = kwargs['retry_interval'] if 'retry_interval' in kwargs else 5


Same as above.

troydai · 2017-09-28T16:33:05Z

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

+        try:
+            errorMessage = None
+            response = requests.get(
+                'https://{}/{}'.format(login_server, path),


Are login_server and path from user input? If so, should this be validated before form this URL?

login_server is queried from ACR RP.

yuwaMSFT2 · 2017-09-28T18:09:58Z

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

+    for i in range(0, retry_times):
+        try:
+            errorMessage = None
+            response = requests.get(


Is this sending a manifest GET request? If you only want to know the digest from response header, you can send HEAD request to avoid transferring the body which you don't care.

yuwaMSFT2 · 2017-09-28T18:20:00Z

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

@@ -226,6 +276,7 @@ def acr_repository_show_tags(registry_name,
        registry_name=registry_name,
        resource_group_name=resource_group_name,
        path='/v2/{}/tags/list'.format(repository),
+        permission='pull',


one question: for CLI to send request to registry, does it first get a challenge response from registry? If we do, there is information in the challenge response on what permission is needed. If we are not getting challenge response but hard-code the permission, there is a concern that this may change from the server side (although unlikely, just keep in mind).

yuwaMSFT2 · 2017-09-28T18:25:03Z

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

-                               retry_times=3,  # pylint: disable=unused-argument
-                               retry_interval=5):  # pylint: disable=unused-argument
-    registryEndpoint = 'https://' + login_server
+def _params(count=20):


So this query parameter is used for all list related API calls (list manifest, catalog, tag list)?

Correct, all registry list calls will use this pagination count by default.

troydai · 2017-09-28T21:09:53Z

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

-        registryEndpoint + path,
-        headers=_headers(username, password)
-    )
+def _pagination_params(count):


A preferable method naming schema is verb + noun.

troydai · 2017-09-28T21:11:18Z

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

+    print(login_server)
+    print(username)
+    print(password)
+    print(pagination)


Wait ... what's this? Please don't print password.

Missed the cleanup here :-)

troydai · 2017-09-28T21:23:40Z

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

+                                  repository,
+                                  tag,
+                                  manifest,
+                                  yes):


yes is not a good parameter name. how about changes it to confirmed

yes is the parameter exposed to users so it seems natural to use it along the path rather than creating a new name internally.

I believe "yes" is required by the CLI for confirmation... #2520

Oh, my bad. But it is not a good name anyway. We may need to address this in the core.

troydai · 2017-09-28T21:24:50Z

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

+            path='/v2/{}/manifests/{}'.format(repository, tag),
+            username=username,
+            password=password
+        )


The indentations and condition statement can be saved in the following form:

manifest = manifest or _get_manifest_digest(...)

troydai · 2017-09-28T21:29:08Z

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

-
-
-class Unauthorized(Exception):
+class RetryableException(Exception):


Why do you define a new type of exception while there isn't any mechanism to treat this exception differently (for example, except RetryableException.)?

Please avoid introducing new exception type unless there is specific logic build around it.

troydai · 2017-09-29T16:34:41Z

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

-    if errorMessage:
-        raise CLIError(errorMessage)
+        if errorMessage:
+            raise CLIError(errorMessage)


Ugr, this is not a minor formatting update. This change the control flow. Please double check and make sure this is what you intended to do.

This is an intended change, the error should be on the retry for loop.

djyou · 2017-09-29T18:51:15Z

@troydai Please merge this PR if there is no more comments, thank you.

djyou · 2017-10-02T20:57:38Z

@troydai Can you please merge this? Thank you :-)

Improve perf and fix permission in repository commands

35c6914

sajayantony reviewed Sep 28, 2017

View reviewed changes

troydai suggested changes Sep 28, 2017

View reviewed changes

yuwaMSFT2 approved these changes Sep 28, 2017

View reviewed changes

djyou added 2 commits September 28, 2017 13:44

Address review comments

b21604a

Cleanup

f55d4db

troydai suggested changes Sep 28, 2017

View reviewed changes

djyou added 2 commits September 28, 2017 14:41

Fix namings

5e09654

Minor formatting update

a17899e

troydai reviewed Sep 29, 2017

View reviewed changes

derekbekoe approved these changes Oct 3, 2017

View reviewed changes

troydai approved these changes Oct 3, 2017

View reviewed changes

troydai merged commit 5198bd8 into Azure:master Oct 3, 2017

djyou mentioned this pull request Oct 3, 2017

SPN with Reader role cannot do show tags on a repository Azure/acr#65

Closed

djyou deleted the doyou/delete branch October 3, 2017 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ACR] Improve perf and fix permission in repository commands #4566

[ACR] Improve perf and fix permission in repository commands #4566

djyou commented Sep 28, 2017 •

edited

Loading

djyou commented Sep 28, 2017

sajayantony commented Sep 28, 2017

sajayantony Sep 28, 2017

troydai Sep 28, 2017

troydai Sep 28, 2017

troydai Sep 28, 2017

troydai Sep 28, 2017

troydai Sep 28, 2017

troydai Sep 28, 2017

troydai Sep 28, 2017

troydai Sep 28, 2017

troydai Sep 28, 2017

djyou Sep 28, 2017

yuwaMSFT2 Sep 28, 2017

yuwaMSFT2 Sep 28, 2017

yuwaMSFT2 Sep 28, 2017

djyou Sep 28, 2017

troydai Sep 28, 2017

troydai Sep 28, 2017

djyou Sep 28, 2017

troydai Sep 28, 2017

djyou Sep 28, 2017

tjprescott Sep 28, 2017

troydai Sep 29, 2017

troydai Sep 28, 2017

troydai Sep 28, 2017

troydai Sep 29, 2017

djyou Sep 29, 2017

djyou commented Sep 29, 2017

djyou commented Oct 2, 2017



		class Unauthorized(Exception):
		class RetryableException(Exception):

[ACR] Improve perf and fix permission in repository commands #4566

[ACR] Improve perf and fix permission in repository commands #4566

Conversation

djyou commented Sep 28, 2017 • edited Loading

General Guidelines

Command Guidelines

djyou commented Sep 28, 2017

sajayantony commented Sep 28, 2017

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

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 Sep 29, 2017

djyou commented Oct 2, 2017

djyou commented Sep 28, 2017 •

edited

Loading