[Breadth Coverage] Onboard Import Export #1515

arrownj · 2020-04-07T10:43:07Z

Spec is here

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.

src/import-export/azext_import_export/azext_metadata.json

yonzhan · 2020-04-07T10:55:29Z

Import Export

yungezz · 2020-04-07T14:08:15Z

src/import-export/azext_import_export/generated/_help.py

+        text: |-
+               az import-export create --resource-group "myResourceGroup" --name "myJob"
+               --location "West US" --backup-drive-manifest true --diagnostics-path "waimportexport"
+               --drive-list bit-locker-key=238810-662376-448998-450120-652806-203390-606320-483076


drive-list is an array right

yes, we can specify multi --dirve-list here.

yungezz · 2020-04-07T14:11:51Z

src/import-export/azext_import_export/generated/_params.py

+def load_arguments(self, _):
+
+    with self.argument_context('import-export list') as c:
+        c.argument('top', help='An integer value that specifies how many jobs at most should be returned. The value cannot exceed 100.')


seems this isn't in help file

Currently top value can not be less than total count of jobs. It's very strange. I have sent an email to ask about this. I will remove these 2 parameters temporarily and add them back when get the final answer.

haroldrandom · 2020-04-08T02:02:25Z

src/import-export/setup.cfg

@@ -0,0 +1,2 @@
+[bdist_wheel]


azure-cli-core doesn't support Python 2 any more.

Leaving this would lead to the built wheel named as import-export-py2-py3-none-any.whl.

Should Leave this file empty and the wheel name would be import-export-py3-none-any.whl

haroldrandom · 2020-04-08T02:04:24Z

src/import-export/setup.py

+    'Intended Audience :: System Administrators',
+    'Programming Language :: Python',
+    'Programming Language :: Python :: 3',
+    'Programming Language :: Python :: 3.5',


azure-cli-core don't support Python 3.5 Azure/azure-cli#12694

qwordy · 2020-04-08T03:40:02Z

src/import-export/azext_import_export/generated/_validators.py

+# --------------------------------------------------------------------------------------------
+
+
+def example_name_or_id_validator(cmd, namespace):


Please delete it

qwordy · 2020-04-08T03:42:36Z

src/import-export/azext_import_export/generated/action.py

+            if kl == 'recipient-name':
+                d['recipient_name'] = v
+            elif kl == 'street-address1':
+                d['street_address1'] = v
+            elif kl == 'street-address2':
+                d['street_address2'] = v
+            elif kl == 'city':
+                d['city'] = v
+            elif kl == 'state-or-province':
+                d['state_or_province'] = v
+            elif kl == 'postal-code':
+                d['postal_code'] = v
+            elif kl == 'country-or-region':
+                d['country_or_region'] = v
+            elif kl == 'phone':
+                d['phone'] = v
+            elif kl == 'email':
+                d['email'] = v


Suggested change

if kl == 'recipient-name':

d['recipient_name'] = v

elif kl == 'street-address1':

d['street_address1'] = v

elif kl == 'street-address2':

d['street_address2'] = v

elif kl == 'city':

d['city'] = v

elif kl == 'state-or-province':

d['state_or_province'] = v

elif kl == 'postal-code':

d['postal_code'] = v

elif kl == 'country-or-region':

d['country_or_region'] = v

elif kl == 'phone':

d['phone'] = v

elif kl == 'email':

d['email'] = v

if kl in ['recipient-name', 'street-address1', ..., 'email']:

d[kl] = v

And the variable naming is hard for human to read. d, k, kl, v

Actually the parameter name and the map key are not the same. For example, state-or-province is parameter name, state_or_province is the key, they are not the same. I prefer the current way because it is explicit.

Oh. Sorry. I haven't noticed it.

qwordy · 2020-04-08T03:44:35Z

src/import-export/azext_import_export/generated/action.py

+            if kl == 'carrier-name':
+                d['carrier_name'] = v
+            elif kl == 'carrier-account-number':
+                d['carrier_account_number'] = v


Can be simplified.

how about feedback this to code gen team

Actually the key and parameter name is not the same. It's a underscore in the key, and a minus in parameter name. I prefer current code because it is more explicitly and we can rename it easily.

qwordy · 2020-04-08T03:44:50Z

src/import-export/azext_import_export/generated/action.py

+            if kl == 'recipient-name':
+                d['recipient_name'] = v
+            elif kl == 'street-address1':
+                d['street_address1'] = v
+            elif kl == 'street-address2':
+                d['street_address2'] = v
+            elif kl == 'city':
+                d['city'] = v
+            elif kl == 'state-or-province':
+                d['state_or_province'] = v
+            elif kl == 'postal-code':
+                d['postal_code'] = v
+            elif kl == 'country-or-region':
+                d['country_or_region'] = v
+            elif kl == 'phone':
+                d['phone'] = v


Can be simplified.

qwordy · 2020-04-08T03:45:01Z

src/import-export/azext_import_export/generated/action.py

+            if kl == 'carrier-name':
+                d['carrier_name'] = v
+            elif kl == 'tracking-number':
+                d['tracking_number'] = v
+            elif kl == 'drive-count':
+                d['drive_count'] = v
+            elif kl == 'ship-date':
+                d['ship_date'] = v


Can be simplified.

qwordy · 2020-04-08T03:45:20Z

src/import-export/azext_import_export/generated/action.py

+            if kl == 'carrier-name':
+                d['carrier_name'] = v
+            elif kl == 'tracking-number':
+                d['tracking_number'] = v
+            elif kl == 'drive-count':
+                d['drive_count'] = v
+            elif kl == 'ship-date':
+                d['ship_date'] = v


Can be simplified.

qwordy · 2020-04-08T03:46:37Z

src/import-export/azext_import_export/generated/action.py

+            if kl == 'drive-id':
+                d['drive_id'] = v
+            elif kl == 'bit-locker-key':
+                d['bit_locker_key'] = v
+            elif kl == 'manifest-file':
+                d['manifest_file'] = v
+            elif kl == 'manifest-hash':
+                d['manifest_hash'] = v
+            elif kl == 'drive-header-hash':
+                d['drive_header_hash'] = v
+            elif kl == 'state':
+                d['state'] = v
+            elif kl == 'copy-status':
+                d['copy_status'] = v
+            elif kl == 'percent-complete':
+                d['percent_complete'] = v
+            elif kl == 'verbose-log-uri':
+                d['verbose_log_uri'] = v
+            elif kl == 'error-log-uri':
+                d['error_log_uri'] = v
+            elif kl == 'manifest-uri':
+                d['manifest_uri'] = v
+            elif kl == 'bytes-succeeded':
+                d['bytes_succeeded'] = v


Can be simplified.
And where do these string literals come from? Are they defined in SDK? If so, avoid so many string literals.

qwordy · 2020-04-08T03:46:54Z

src/import-export/azext_import_export/generated/action.py

+            if kl == 'blob-listblob-path':
+                d['blob_listblob_path'] = v
+            elif kl == 'blob-path':
+                d['blob_path'] = v
+            elif kl == 'blob-path-prefix':
+                d['blob_path_prefix'] = v


Can be simplified.

qwordy · 2020-04-08T05:05:40Z

src/import-export/azext_import_export/vendored_sdks/__init__.py

+# regenerated.
+# --------------------------------------------------------------------------
+
+__path__ = __import__('pkgutil').extend_path(__path__, __name__)


Is it written by you manually?
Some extensions use the following sentence.

__import__('pkg_resources').declare_namespace(__name__)

should be autogenerated. what's the difference?

Reference document is here. __import__('pkg_resources').declare_namespace(__name__) is no longer recommentded.

yungezz · 2020-04-14T12:53:56Z

src/import-export/azext_import_export/generated/_params.py

+        c.argument('client_tenant_id', help='The tenant ID of the client making the request.')
+        c.argument('location', arg_type=get_location_type(self.cli_ctx), validator=get_default_location_from_resource_group)
+        c.argument('tags', tags_type)
+        c.argument('storage_account_id', help='The resource identifier of the storage account where data will be imported to or exported from.')


support id or name

yungezz · 2020-04-14T12:54:28Z

src/import-export/azext_import_export/generated/_params.py

+        c.argument('delivery_package', action=AddDeliveryPackage, nargs='+', help='Contains information about the package being shipped by the customer to the Microsoft data center.')
+        c.argument('return_package', action=AddReturnPackage, nargs='+', help='Contains information about the package being shipped by the customer to the Microsoft data center.')
+        c.argument('diagnostics_path', help='The virtual blob directory to which the copy logs and backups of drive manifest files (if enabled) will be stored.')
+        c.argument('log_level', help='Default value is Error. Indicates whether error logging or verbose logging will be enabled.')


is this enum in swagger? if yes, pls feedback to code gen team.

It's defined as string in swagger.

yungezz · 2020-04-14T12:55:35Z

src/import-export/azext_import_export/generated/_params.py

+        c.argument('incomplete_blob_list_uri', help='A blob path that points to a block blob containing a list of blob names that were not exported due to insufficient drive space. If all blobs were exported successfully, then this element is not included in the response.')
+        c.argument('drive_list', action=AddDriveList, nargs='+', help='List of up to ten drives that comprise the job. The drive list is a required element for an import job; it is not specified for export jobs.')
+        c.argument('export', action=AddExport, nargs='+', help='A property containing information about the blobs to be exported for an export job. This property is required for export jobs, but must not be specified for import jobs.')
+        c.argument('provisioning_state', help='Specifies the provisioning state of the job.')


this is an argument?

Nice finding. It should be a bug of swagger file. It reuse a JobDetail object in both PutParameter and JobResponse. As a result of that we can specify this value when create a job, however the server will ignore it. I think we should remove it from create command parameters.

yungezz · 2020-04-14T12:57:04Z

src/import-export/azext_import_export/generated/action.py

+            if kl == 'carrier-name':
+                d['carrier_name'] = v
+            elif kl == 'carrier-account-number':
+                d['carrier_account_number'] = v


how about feedback this to code gen team

yungezz · 2020-04-14T12:58:21Z

src/import-export/azext_import_export/generated/custom.py

+    return client.get(job_name=name, resource_group_name=resource_group_name)
+
+
+def import_export_job_create(cmd, client, name, resource_group_name, client_tenant_id=None, location=None, tags=None,


pls make sure indention correct.

Should be no problem with indention. It's a IDE suggested style and pass our style check.

yungezz · 2020-04-14T12:59:05Z

src/import-export/azext_import_export/vendored_sdks/__init__.py

+# regenerated.
+# --------------------------------------------------------------------------
+
+__path__ = __import__('pkgutil').extend_path(__path__, __name__)


should be autogenerated. what's the difference?

arrownj · 2020-04-14T14:02:45Z

/azp run

azure-pipelines · 2020-04-14T14:02:56Z

Azure Pipelines successfully started running 1 pipeline(s).

[Breadth Coverage] Onboard Import Export

d7fa25a

arrownj self-assigned this Apr 7, 2020

arrownj requested review from Juliehzl, fengzhou-msft, haroldrandom, jsntcy, qwordy and yungezz April 7, 2020 10:43

arrownj commented Apr 7, 2020

View reviewed changes

src/import-export/azext_import_export/azext_metadata.json Outdated Show resolved Hide resolved

arrownj added 2 commits April 7, 2020 21:19

update CODEOWNERS

78e36be

remove unused code

c2fe423

yungezz reviewed Apr 7, 2020

View reviewed changes

fix review problem

2b5f894

haroldrandom reviewed Apr 8, 2020

View reviewed changes

update setup.py & setup.cfg

c1435ff

qwordy reviewed Apr 8, 2020

View reviewed changes

remove unused code

3ff07a3

qwordy approved these changes Apr 9, 2020

View reviewed changes

yungezz reviewed Apr 14, 2020

View reviewed changes

haroldrandom approved these changes Apr 15, 2020

View reviewed changes

arrownj added 5 commits April 17, 2020 16:34

Merge remote-tracking branch 'upstream/master' into import_export

e1feead

fix tags issues

bffaa84

fix review issues

17948fc

Merge branch 'master' into import_export

c552dcc

update storage-account-id to storage-account

0a27a31

yungezz approved these changes Apr 21, 2020

View reviewed changes

arrownj added 2 commits April 21, 2020 17:45

add support for location list/show

607daf9

update README.md

abd7e7a

arrownj merged commit 581cb60 into Azure:master Apr 22, 2020

		# --------------------------------------------------------------------------------------------


		def example_name_or_id_validator(cmd, namespace):

		return client.get(job_name=name, resource_group_name=resource_group_name)


		def import_export_job_create(cmd, client, name, resource_group_name, client_tenant_id=None, location=None, tags=None,

[Breadth Coverage] Onboard Import Export #1515

[Breadth Coverage] Onboard Import Export #1515

Uh oh!

Conversation

arrownj commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General Guidelines

About Extension Publish

Uh oh!

Uh oh!

yonzhan commented Apr 7, 2020

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!

qwordy Apr 8, 2020 • 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!

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!

arrownj commented Apr 7, 2020 •

edited

Loading

qwordy Apr 8, 2020 •

edited

Loading