[Breadth Coverage] Support StorageSync in Azure CLI cmdlets #1389

jsntcy · 2020-03-13T12:18:08Z

What does this PR do?

Support the key use case for Storage.
Issue link is [Breadth Coverage] Onboard Storage Sync Commands azure-cli#11737

What's the key use case?

Create ‘Storage Sync Service’ resource
Create a ‘Registered Server'
Create a ‘Sync Group’
Create a 'Cloud Endpoint’ requiring a sync group
Create a ‘Server Endpoint’ requiring a sync group and a registered server.

Which commands will be supported?
az storagesync storage-sync-service create\delete\show\list
az storagesync sync-group create\delete\show\list
az storagesync cloud-endpoint create\delete\show\list
az storagesync server-endpoint create\delete\show\list
az storagesync registered-server delete\show\list

Notes
az storagesync registered-server create and az storagesync registered-server reset-certificate are operations mixing cloud and local COM operations. Azure CLI will not support these two commands because of technical limitation. User need use Azure PowerShell or Azure File Share Agent instead for these two commands.

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

General Guidelines

Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

My extension description/summary conforms to the Extension Summary Guidelines.

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

azuresdkci · 2020-03-13T12:18:11Z

If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR:

docker run -it microsoft/azure-cli:latest
export EXT=<NAME>
pip install --upgrade --target ~/.azure/cliextensions/$EXT "git+https://github.com/jsntcy/azure-cli-extensions.git@bc-storagesync-final#subdirectory=src/$EXT&egg=$EXT"

yonzhan · 2020-03-13T13:00:53Z

add to S167

yungezz · 2020-03-15T07:27:29Z

just curious, is AzStorageSyncServerCertificate Azure powershell module? Any context it could be Azure Powershell, but not in CLI? #WontFix

yungezz · 2020-03-15T07:28:46Z

src/storagesync/README.md

+az storagesync cloud-endpoint create \
+    --resource-group rg \
+    --name cloud-endpoint-name \
+    --storage-sync-service-name storage-sync-service-name \


storage-sync-service to support both name and id?

no, only support name

In reply to: 392648605 [](ancestors = 392648605)

it should support both of them. #Resolved

support both now

In reply to: 392866324 [](ancestors = 392866324)

yungezz · 2020-03-15T07:30:27Z

src/storagesync/azext_storagesync/_help.py

+               "SampleStorageSyncService"
+"""
+
+helps['storagesync registered-server wait'] = """


pair with no-wait? #WontFix

Yes, because we define 'delete' as supporting 'no-wait', we need a 'wait' command to query the 'delete' status.

In reply to: 392648677 [](ancestors = 392648677)

yonzhan · 2020-03-15T09:47:43Z

just curious, is AzStorageSyncServerCertificate Azure powershell module? Any context it could be Azure Powershell, but not in CLI?

Thanks for the review. After confirm with PM. az storagesync registered-server create and az storagesync registered-server reset-certificate are operations mixing cloud and local COM operations. Azure CLI will not support these two commands because of technical limitation. User need use Azure PowerShell or Azure File Share Agent instead for these two commands.

jsntcy · 2020-03-16T01:23:45Z

This command mixes cloud call and local COM calls which internally call Win32 API, while local COM calls are not easy to implement in Python (PowerShell is using .Net), I forward a email to you about the context.

In reply to: 599177453 [](ancestors = 599177453)

fengzhou-msft · 2020-03-16T07:32:00Z

.github/CODEOWNERS

@@ -73,3 +73,5 @@
 /src/connectedmachine/ @farehar

 /src/ip-group/ @haroldrandom
+
+/src/storagesync/ @jsntcy


Shall we use storage-sync as the extension name?

Not sure which one should be as both patterns exist in CLI, and we can have a discussion.

In reply to: 392828794 [](ancestors = 392828794)

There is guideline on command naming. But we can discuss it for sure.

az [ group ] [ subgroup ] [ command ] {parameters}
Multi-word subgroups should be hyphenated (e.g. foo-resource instead of fooresource)

It seems storagesync is group, but not subgroup.

Also discuss within our team, they agree I can keep it without hyphen.
So I'll keep it without hyphen.

In reply to: 392864381 [](ancestors = 392864381)

fengzhou-msft · 2020-03-16T07:39:03Z

src/storagesync/azext_storagesync/_params.py

+                                              validator=parse_server_id)
+    server_id_type = CLIArgumentType(help='GUID identifying the on-premises server.')
+
+    with self.argument_context('storagesync storage-sync-service create') as c:


The prefix of the subcommand name seems redundant. just use storagesync service create? #Resolved

After internal discussion, we decide to use 'storagesync create'

In reply to: 392831198 [](ancestors = 392831198)

fengzhou-msft · 2020-03-16T07:45:05Z

src/storagesync/azext_storagesync/custom.py

+
+def list_storagesync_storage_sync_service(client,
+                                          resource_group_name=None):
+    if resource_group_name is not None:


Use if resource_group_name: in order to use the workaround -g= to temparorily disable appending default resource group as we discussed. #Resolved

will update it.

In reply to: 392833381 [](ancestors = 392833381)

mmyyrroonn · 2020-03-16T08:57:08Z

src/storagesync/README.md

+
+##### Delete a given storage sync service.
+```
+az storagesync storage-sync-service delete \


should it be az storage-sync service? #Resolved

will be az storagesync delete after internal discussion

In reply to: 392865778 [](ancestors = 392865778)

mmyyrroonn · 2020-03-16T08:57:27Z

src/storagesync/README.md

+
+##### Create a new sync group.
+```
+az storagesync sync-group create \


az storage-sync group create ?

use az storagesync sync-group create after discussion

In reply to: 392865940 [](ancestors = 392865940)

mmyyrroonn · 2020-03-16T08:58:09Z

src/storagesync/README.md

+az storagesync cloud-endpoint create \
+    --resource-group rg \
+    --name cloud-endpoint-name \
+    --storage-sync-service-name storage-sync-service-name \


it should support both of them. #Resolved

mmyyrroonn · 2020-03-16T09:00:15Z

src/storagesync/azext_storagesync/_help.py

+    examples:
+      - name: Create a new storage sync service "SampleStorageSyncService" in resource group 'SampleResourceGroup'.
+        text: |-
+               az storagesync storage-sync-service create --resource-group "SampleResourceGroup" \\


better to use MyResourceGroup. Align with other examples.

It's generated by codegen, and it should be from swagger samples, in the future, we should just use the rg from swagger without manual update.

In reply to: 392867347 [](ancestors = 392867347)

mmyyrroonn · 2020-03-16T09:01:33Z

src/storagesync/azext_storagesync/_params.py

+
+    with self.argument_context('storagesync storage-sync-service delete') as c:
+        c.argument('resource_group_name', resource_group_name_type)
+        c.argument('storage_sync_service_name', storage_sync_service_name_type, options_list=['--name', '-n'])


how about adding id_part=name? #Resolved

updated

In reply to: 392868000 [](ancestors = 392868000)

mmyyrroonn · 2020-03-16T09:02:11Z

src/storagesync/azext_storagesync/_params.py

+
+    with self.argument_context('storagesync sync-group create') as c:
+        c.argument('resource_group_name', resource_group_name_type)
+        c.argument('storage_sync_service_name', storage_sync_service_name_type)


is it suitable to support id of storage sync service as well? #Resolved

supported now

In reply to: 392868326 [](ancestors = 392868326)

mmyyrroonn · 2020-03-16T09:03:32Z

src/storagesync/azext_storagesync/_params.py

+        c.argument('cloud_endpoint_name', cloud_endpoint_name_type, options_list=['--name', '-n'])
+        c.argument('storage_account_resource_id', storage_account_type)
+        c.argument('azure_file_share_name', azure_file_share_name_type)
+        c.argument('storage_account_tenant_id', storage_account_tenant_id_type)


can we get this tenant id by storage_account_resource_id? #WontFix

Yes, I do it in validator: _get_tenant_id():

In reply to: 392868993 [](ancestors = 392868993)

mmyyrroonn · 2020-03-16T09:05:25Z

src/storagesync/azext_storagesync/_params.py

+        c.argument('server_resource_id', server_resource_id_type)
+        c.argument('server_local_path', help='The local path of the registered server.')
+        c.argument('cloud_tiering', arg_type=get_enum_type(['on', 'off']), help='Cloud Tiering.')
+        c.argument('volume_free_space_percent',


The following four parameters are too long. It's also a little bit confused to me. Is it able to add more help messages and examples? #Resolved

I'll add more help for them and add use these parameters in example

In reply to: 392869872 [](ancestors = 392869872)

mmyyrroonn · 2020-03-16T09:07:16Z

src/storagesync/azext_storagesync/custom.py

+    body['server_resource_id'] = server_resource_id  # str
+    body['server_local_path'] = server_local_path  # str
+    body['cloud_tiering'] = cloud_tiering  # str
+    body['volume_free_space_percent'] = volume_free_space_percent  # number


what's the meaning of number? should it be type=int? #Resolved

should be int,, number is generated from codegen,

In reply to: 392870777 [](ancestors = 392870777)

mmyyrroonn · 2020-03-16T09:12:10Z

src/storagesync/azext_storagesync/tests/latest/test_storagesync_scenario.py

+                 '--name {server_endpoint_name} '
+                 '--server-id {server_id} '
+                 '--server-local-path "c:\\users\\syncfolder"',
+                 checks=[JMESPathCheck('provisioningState', 'Succeeded'),


this command can accept four arguments. Could we add more tests? Looks like this should be an important command. #WontFix

OK, I'll try to add more parameters for test.

In reply to: 392873316 [](ancestors = 392873316)

jsntcy added 4 commits February 3, 2020 22:12

draft storage sync cli commands

08a93e0

upload for vm test

d02a444

save to remote temp

c67cdef

[Breadth Coverage] support storage sync in CLI extensions

a648fab

jsntcy requested review from Juliehzl, yonzhan, qianwens, mmyyrroonn, fengzhou-msft, jiasli and haroldrandom March 13, 2020 12:19

jsntcy self-assigned this Mar 13, 2020

yonzhan added this to the S167 milestone Mar 13, 2020

yungezz reviewed Mar 15, 2020

View reviewed changes

add code owner and default tags

fed323d

fengzhou-msft reviewed Mar 16, 2020

View reviewed changes

mmyyrroonn reviewed Mar 16, 2020

View reviewed changes

update code based on review comments

37416bd

yonzhan modified the milestones: S167, S168 Mar 28, 2020

jsntcy added 2 commits April 7, 2020 15:08

add experimental tag

31bf83b

remove useless dependencies

b44d532

jsntcy force-pushed the bc-storagesync-final branch from 0aa238f to b44d532 Compare April 7, 2020 07:34

jsntcy added 2 commits April 7, 2020 15:36

Merge branch 'master' into bc-storagesync-final

7d0cd78

Merge branch 'master' into bc-storagesync-final

9bd57d1

mmyyrroonn approved these changes Apr 7, 2020

View reviewed changes

jsntcy merged commit 73928e0 into Azure:master Apr 7, 2020

ManuInNZ pushed a commit to ManuInNZ/azure-cli-extensions that referenced this pull request Apr 11, 2020

[Breadth Coverage] Support StorageSync in Azure CLI cmdlets (Azure#1389)

7de7a21

[Breadth Coverage] Support StorageSync in Azure CLI cmdlets #1389

[Breadth Coverage] Support StorageSync in Azure CLI cmdlets #1389

Conversation

jsntcy commented Mar 13, 2020

General Guidelines

About Extension Publish

azuresdkci commented Mar 13, 2020

yonzhan commented Mar 13, 2020

yungezz commented Mar 15, 2020 • edited by jsntcy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmyyrroonn Mar 16, 2020 • edited by jsntcy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungezz Mar 15, 2020 • edited by jsntcy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonzhan commented Mar 15, 2020

jsntcy commented Mar 16, 2020 • edited Loading

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

fengzhou-msft Mar 16, 2020 • edited by jsntcy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fengzhou-msft Mar 16, 2020 • edited by jsntcy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmyyrroonn Mar 16, 2020 • edited by jsntcy Loading

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

mmyyrroonn Mar 16, 2020 • edited by jsntcy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmyyrroonn Mar 16, 2020 • edited by jsntcy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmyyrroonn Mar 16, 2020 • edited by jsntcy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmyyrroonn Mar 16, 2020 • edited by jsntcy Loading

Choose a reason for hiding this comment

jsntcy Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

mmyyrroonn Mar 16, 2020 • edited by jsntcy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmyyrroonn Mar 16, 2020 • edited by jsntcy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmyyrroonn Mar 16, 2020 • edited by jsntcy Loading

Choose a reason for hiding this comment

jsntcy Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

yungezz commented Mar 15, 2020 •

edited by jsntcy

Loading

mmyyrroonn Mar 16, 2020 •

edited by jsntcy

Loading

yungezz Mar 15, 2020 •

edited by jsntcy

Loading

jsntcy commented Mar 16, 2020 •

edited

Loading

fengzhou-msft Mar 16, 2020 •

edited by jsntcy

Loading

fengzhou-msft Mar 16, 2020 •

edited by jsntcy

Loading

mmyyrroonn Mar 16, 2020 •

edited by jsntcy

Loading

mmyyrroonn Mar 16, 2020 •

edited by jsntcy

Loading

mmyyrroonn Mar 16, 2020 •

edited by jsntcy

Loading

mmyyrroonn Mar 16, 2020 •

edited by jsntcy

Loading

mmyyrroonn Mar 16, 2020 •

edited by jsntcy

Loading

jsntcy Mar 16, 2020 •

edited

Loading

mmyyrroonn Mar 16, 2020 •

edited by jsntcy

Loading

mmyyrroonn Mar 16, 2020 •

edited by jsntcy

Loading

mmyyrroonn Mar 16, 2020 •

edited by jsntcy

Loading

jsntcy Mar 17, 2020 •

edited

Loading