Knack - Storage Command Module #5076

troydai · 2017-12-11T02:55:20Z

Fixes: #4941

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)

azuresdkci · 2017-12-11T02:55:37Z

View a preview at https://prompt.ws/r/Azure/azure-cli/5076
This is an experimental preview for @microsoft.com users.
(It may take a minute or two for your instance to be ready)
Email feedback to 'azfeedback' with subject 'Prompt Feedback'.

tjprescott

General points:

Accessing _cmd through the namespace in a validator should not be done. Add cmd to the validator signature and access that way.
Where you have cmd.supported_api_version(..), you should be able to omit the ResourceType, since it is presumably already set (either at the module level or command group level).
Where you are using the stand-alone get_sdk to retrieve models, you should be able to use cmd.get_models(...). That is the recommended way, so if it doesn't work for some reason, I'd be very interested to understand why as it might mean there's a bug I need to address.
I would be surprised if the completers worked as they don't conform to the signature that @completer calls for.

I pointed out a few other places where you did things that I wouldn't expect you to have to do. If the simplifications I suggest don't work, please let me know as it means there's probably an edge-case that the core doesn't handle the way it should.

tjprescott · 2017-12-11T16:50:11Z

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

        c.argument('tags', tags_type)
        c.argument('custom_domain', help='User domain assigned to the storage account. Name is the CNAME source.')
-        c.argument('sku', help='The storage account SKU.', arg_type=get_enum_type(SkuName, default='standard_ragrs'))
+        c.argument('sku', help='The storage account SKU.', arg_type=get_enum_type(t_sku_name, default='standard_ragrs'))

    with self.argument_context('storage account update') as c:
        register_common_storage_account_options(c)


This could be moved into the argument context to be more idiomatic with the other statements (argument, ignore, etc).

tjprescott · 2017-12-11T16:51:49Z

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

+    with self.argument_context('storage blob lease') as c:
+        c.argument('lease_duration', type=int)
+        c.argument('lease_break_period', type=int)
+        c.argument('blob_name', arg


These can be combined: c.ignore('blob_name', 'snapshot')

tjprescott · 2017-12-11T16:52:14Z

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

+    with self.argument_context('storage blob lease') as c:
+        c.argument('lease_duration', type=int)
+        c.argument('lease_break_period', type=int)
+        c.argument('blob_name', arg


tjprescott · 2017-12-11T16:53:57Z

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

+    with self.argument_context('storage blob lease') as c:
+        c.argument('lease_duration', type=int)
+        c.argument('lease_break_period', type=int)
+        c.argument('blob_name', arg


If you put arg_group='Copy Source' into line 560, this should also work. less repetition. If it doesn't, it would be a bug.

tjprescott · 2017-12-11T16:56:54Z

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

    n = namespace

+    def get_config_value(section, key, default):
+        return namespace._cmd.cli_ctx.config.get(section, key, default)


This approach relies on an implementation-detail that may change. Instead of putting namespace._cmd here, you should simply put cmd into validate_client_parameters signature. The you just return cmd.cli_ctx....

tjprescott · 2017-12-11T17:09:29Z

src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/operations/acl.py

+
+def _get_service_container_type(cmd, client):
+    from ..sdkutil import get_table_data_type
+    t_block_blob_svc, t_file_svc, t_queue_svc = get_sdk(


Why does cmd.get_models not work here?

Because for storage multiapi package the models are in different namespaces. For example, blob.modes and file.models.

tjprescott · 2017-12-11T17:11:14Z

src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/operations/blob.py

@@ -174,13 +180,13 @@ def upload_append_blob():
            'container_name': container_name,
            'blob_name': blob_name,
            'file_path': file_path,
-            'progress_callback': _update_progress,
+            'progress_callback': get_update_progress_fn(cmd),


Does this show progress for each file? If so, it seems very much like with Tamir is trying to do in the image copy extension. If this works for us, it might work in his situation.

yes, it does.

Please touch base with him and see if this helps with his image_copy extension, since I'm assuming this doesn't break due to the cli_ctx.

tjprescott · 2017-12-11T17:12:14Z

src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/operations/blob.py

            'maxsize_condition': maxsize_condition,
            'lease_id': lease_id,
            'timeout': timeout
        }

-        if supported_api_version(ResourceType.DATA_STORAGE, min_api='2016-05-31'):
+        if cmd.supported_api_version(ResourceType.DATA_STORAGE, min_api='2016-05-31'):


I wouldn't expect that you would need to specify the ResourceType.

will update.

tjprescott · 2017-12-11T17:14:30Z

src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/operations/directory.py

+
+
+def list_share_directories(cmd, client, share_name, directory_name=None, timeout=None):
+    t_dir_properties = get_sdk(cmd.cli_ctx, ResourceType.DATA_STORAGE, 'file.models#DirectoryProperties')


Again, here you should just be able to call cmd.get_models('file.models#DirectoryProperties')

tjprescott · 2017-12-11T17:20:46Z

src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/util.py

+
+
+@Completer
+def file_path_completer(cmd, prefix, action, parsed_args, **kwargs):  # pylint: disable=unused-argument


Do these completers work? The signature is supposed to be def my_completer(cmd, prefix, namespace, [**kwargs])

What should be changed to the completer to make it conform the expected signature?

Remove action and rename parsed_args to namespace.

derekbekoe · 2017-12-11T19:01:04Z

src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/_client_factory.py

-        kwargs.pop('account_key', None),
-        connection_string=kwargs.pop('connection_string', None),
-        sas_token=kwargs.pop('sas_token', None))
+    t_file_svc = get_sdk(cli_ctx, ResourceType.DATA_STORAGE, 'file#FileService')


Why use t_file_svc instead of the original name of FileService?
Same everywhere else.

In my opinion this is a better name convention. The original one is misleading. The reader is not able to tell if it is a type actually defined in source code or a type from the SDK. Alternatively, we can adopt this pattern: https://www.python.org/dev/peps/pep-0008/#type-variable-names (of course it makes your code very "C" like).

tjprescott · 2017-12-14T18:24:47Z

.flake8

@@ -14,7 +14,7 @@ exclude =
    scripts
    doc
    build_scripts
-    src/command_modules/azure-cli-storage
+    src/command_modules/azure-cli-lab


This looks like a bad rebase. Lab should be included in flake 8 (and thus shouldn't be in this list).

troydai requested a review from derekbekoe December 11, 2017 02:55

troydai changed the title ~~Kstore~~ Knack - Storage Command Module Dec 11, 2017

tjprescott suggested changes Dec 11, 2017

View reviewed changes

derekbekoe reviewed Dec 11, 2017

View reviewed changes

tjprescott reviewed Dec 14, 2017

View reviewed changes

troydai added 2 commits December 14, 2017 11:15

Update azdev to list commands information

62b8720

Convert storage command modules to knack

8183f7f

tjprescott approved these changes Dec 14, 2017

View reviewed changes

tjprescott merged commit bc2368b into Azure:KnackConversion Dec 14, 2017

tjprescott mentioned this pull request Dec 14, 2017

KNACK Storage module conversion #4941

Closed

troydai deleted the kstore branch December 14, 2017 22:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Knack - Storage Command Module #5076

Knack - Storage Command Module #5076

troydai commented Dec 11, 2017 •

edited

Loading

azuresdkci commented Dec 11, 2017

tjprescott left a comment

tjprescott Dec 11, 2017

tjprescott Dec 11, 2017

tjprescott Dec 11, 2017

tjprescott Dec 11, 2017

troydai Dec 11, 2017

tjprescott Dec 11, 2017

tjprescott Dec 11, 2017

troydai Dec 11, 2017

tjprescott Dec 11, 2017

troydai Dec 11, 2017

tjprescott Dec 12, 2017

tjprescott Dec 11, 2017

troydai Dec 11, 2017

tjprescott Dec 11, 2017

tjprescott Dec 11, 2017

troydai Dec 12, 2017

tjprescott Dec 12, 2017

derekbekoe Dec 11, 2017

troydai Dec 11, 2017

derekbekoe Dec 11, 2017

tjprescott Dec 14, 2017



		def list_share_directories(cmd, client, share_name, directory_name=None, timeout=None):
		t_dir_properties = get_sdk(cmd.cli_ctx, ResourceType.DATA_STORAGE, 'file.models#DirectoryProperties')



		@Completer
		def file_path_completer(cmd, prefix, action, parsed_args, **kwargs): # pylint: disable=unused-argument

Knack - Storage Command Module #5076

Knack - Storage Command Module #5076

Conversation

troydai commented Dec 11, 2017 • edited Loading

General Guidelines

Command Guidelines

azuresdkci commented Dec 11, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troydai commented Dec 11, 2017 •

edited

Loading