{KeyVault} Track 2 mgmt-plane adoption by bim-msft · Pull Request #14150 · Azure/azure-cli

bim-msft · 2020-06-30T09:36:54Z

Use azure-mgmt-keyvault==7.0.0b2.
This is just an internal upgrade, the UX didn't change.

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

The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.

yonzhan · 2020-08-19T04:52:34Z

KeyVault

arrownj

What's this src/azure-cli/azure/cli/command_modules/keyvault/tests/latest/cert_secret file for ?

arrownj · 2020-08-20T08:26:41Z

src/azure-cli-core/azure/cli/core/commands/arm.py

                provisioning_state = getattr(properties, 'provisioning_state', None)
+                if not provisioning_state:
+                    additional_properties = getattr(properties, 'additional_properties', {})
+                    provisioning_state = additional_properties.get('provisioningState')


For this case, is privisioningState always existing ?

@arrownj This is a bug in core. The provisioningState label for keyvault is under properties.provisioning_state.additional_properties, the original code was not able to read it.

better to add some comments here to explain it

arrownj · 2020-08-20T08:28:47Z

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

-        raise CLIError("The specified resource group does not match that of the deleted vault %s. The vault "
-                       "must be recovered to the original resource group %s."
-                       % (vault_name, id_comps['resource_group']))
+    if 'purge' not in cmd.name:


What's purge for ?

@arrownj az keyvault purge doesn't require parameter resource_group. In track 1, if we pass an additional resource_group argument into purge function, it would be ignored. But in track 2, any unexpected arguments would cause a failure.

The code below line 245 is for automatically setting resource_group parameter.

arrownj · 2020-08-20T08:33:29Z

src/azure-cli/azure/cli/command_modules/keyvault/commands.py

        g.custom_command('list', 'list_keyvault')
        g.show_command('show', 'get')
+
+        g.command('purge', 'begin_purge_deleted', supports_no_wait=True)


Are these new commands ? It seems that we didn't add test for them.

@arrownj No, it's an old command. Actually purge command has been already covered:

azure-cli/src/azure-cli/azure/cli/command_modules/keyvault/tests/latest/test_keyvault_commands.py

Line 221 in f3db6bd

self.cmd('keyvault purge -n {kv}')

seems line 71 ~ 73 is duplicated with line 81 ~ 83

seems line 71 ~ 73 is duplicated with line 81 ~ 83

@arrownj Good catch! Removed.

bim-msft · 2020-08-20T08:49:57Z

What's this src/azure-cli/azure/cli/command_modules/keyvault/tests/latest/cert_secret file for ?

@arrownj dev branch also has this file. Binary files wouldn't show anything in git diff.

yungezz · 2020-08-20T08:58:08Z

scripts/sdk_process/patch_models.py

 from msrest.exceptions import HttpOperationError
 """

+track2_header = copyright_header + b"""import msrest.serialization


this is quite hack. is there better way to do it, like feedback to sdk team, maybe add it in track2 generation as a option?

@yungezz Actually the SDK is great, the problem is within this patch_models.py script. This script is totally a hack script.

yungezz · 2020-08-20T08:59:29Z

scripts/sdk_process/patch_models.py

    logging.basicConfig(level=logging.INFO)

+    track2_packages = [
+        'azure.mgmt.keyvault'


will future track2 mgmt sdks need to add this line also?

@yungezz Yes, I think. Anyway, this is a hack script and maybe we should totally understand it and try to deprecate it.

This script was involved at this point: #7061 The motivation is to resolve SDK model references. I think track 2 SDKs won't have the reference related issue.

yungezz · 2020-08-20T09:00:52Z

src/azure-cli-core/azure/cli/core/commands/arm.py

                provisioning_state = getattr(properties, 'provisioning_state', None)
+                if not provisioning_state:
+                    additional_properties = getattr(properties, 'additional_properties', {})
+                    provisioning_state = additional_properties.get('provisioningState')


better to add some comments here to explain it

yungezz · 2020-08-20T09:03:02Z

src/azure-cli/azure/cli/command_modules/keyvault/_help.py

    crafted: true
 """
+
+helps['keyvault wait'] = """


is this a new command? or just fix of help file?

is this a new command? or just fix of help file?

@yungezz This is for LRO. keyvault create and keyvault purge became LRO in track 2.

yungezz · 2020-08-20T09:03:42Z

src/azure-cli/azure/cli/command_modules/keyvault/custom.py

+        no_wait = kwargs.pop('no_wait')
+
+    if cmd.cli_ctx.cloud.profile == 'latest':
+        function_name = 'begin_' + function_name


this is also specific to track2 right?

@yungezz Yes, track 2 only.

it only applies to latest profile and for existing 2019-03-01-hybrid profile with api version 2016-10-01, you will still use track 1 SDK, right?
But there is a plan to create a new profile named 2020-09-01-hybrid with api version 2019-09-01, if new profile is used you will still go to track1 SDK, is it expected?

@Juliehzl No, all profiles after this commit will use track 2. For the new plan we also use track 2. Track 1 would be abandoned at this point. (Management-plane only)

arrownj

LGTM

bim-msft · 2020-08-20T09:41:00Z

Hi @fengzhou-msft You are the code owner of scripts/sdk_process/patch_models.py, could please take a look?

Juliehzl · 2020-08-20T13:26:06Z

src/azure-cli/azure/cli/command_modules/keyvault/commands.py

+        g.custom_command('add', 'add_network_rule', supports_no_wait=True)
+        g.custom_command('remove', 'remove_network_rule', supports_no_wait=True)


in these scenarios, is there a wait command to wait util condition satisfied?

@Juliehzl Good catch. Added.

Juliehzl

Approve for azure_stack_wrapper

jsntcy and others added 30 commits March 17, 2020 18:10

draft: integrate azure-mgmt-core 1.0.0b1

6a8684e

remove code related to storage track 2

3b7055d

use client_kwargs to replace client.config in track2

63d7e11

fix style errors

34a27f8

pass accurate paramters for generic update

85d4da9

Test

23871be

Update tests

20cd498

Add tests

fccd60d

All tests passed

210168e

Add tests

d1e1926

Update yaml

6a0b5e4

updated

38150e0

Updated

37f700f

Updated

91064fb

Merge azure-core-preview

36f3549

Updated

1cb4823

data-plane

577c17d

Fix

feff5c8

draft: integrate azure-mgmt-core 1.0.0b1

e6b148b

remove code related to storage track 2

9776668

use client_kwargs to replace client.config in track2

5fd1527

fix style errors

175b174

pass accurate paramters for generic update

0f8cdc3

fix test error

50159f1

pass raw=True explicitly for sdk calls

f35deb3

remove unused import

c9a9a6d

use azure-mgmt-core 1.0.0b2

9a649d4

Support key list/list-versions/list-deleted

0a565fb

Support create/set-attributes/delete

2ec8658

Merge azure-core-preview

4309dc9

bim-msft added 8 commits August 20, 2020 11:36

merge dev

061234e

updated

d78a396

try remove cache

b311c7c

updated

35a6ffa

updated

b20847c

updated

dbe9023

bugfix

d3fb97f

bugfix

9a87853

bim-msft requested review from Juliehzl, fengzhou-msft, houk-ms, qwordy and yungezz August 20, 2020 07:23

arrownj reviewed Aug 20, 2020

View reviewed changes

yungezz reviewed Aug 20, 2020

View reviewed changes

Refined

2baeac6

arrownj approved these changes Aug 20, 2020

View reviewed changes

Remove *args

2ee7546

Juliehzl reviewed Aug 20, 2020

View reviewed changes

bim-msft added 3 commits August 21, 2020 10:52

Add wait command

a3807d9

Refine azure_stack_wrapper

d0677ba

Use api-version in azure_stack_wrapper

18f529c

Juliehzl approved these changes Aug 21, 2020

View reviewed changes

fengzhou-msft approved these changes Aug 21, 2020

View reviewed changes

bim-msft merged commit 3ffbdb8 into Azure:dev Aug 21, 2020

bim-msft deleted the bim_kv_track2_mgmt branch August 21, 2020 05:48

qwordy mentioned this pull request Jan 13, 2021

{Doc} Track 2 Migration Guidance, revision #16495

Merged

3 tasks

		g.custom_command('add', 'add_network_rule', supports_no_wait=True)
		g.custom_command('remove', 'remove_network_rule', supports_no_wait=True)

Conversation

bim-msft commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yonzhan commented Aug 19, 2020

Uh oh!

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

bim-msft Aug 20, 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!

arrownj Aug 20, 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!

bim-msft commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

bim-msft commented Aug 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Juliehzl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bim-msft commented Jun 30, 2020 •

edited

Loading

bim-msft Aug 20, 2020 •

edited

Loading

arrownj Aug 20, 2020 •

edited

Loading

bim-msft commented Aug 20, 2020 •

edited

Loading