{KeyVault} Track 2 mgmt-plane adoption #14150
Conversation
|
KeyVault |
bim-msft
requested review from
yungezz,
qwordy,
houk-ms,
fengzhou-msft and
Juliehzl
| @@ -636,6 +636,9 @@ def get_provisioning_state(instance): | |||
| properties = getattr(instance, 'properties', None) | |||
| if properties: | |||
| provisioning_state = getattr(properties, 'provisioning_state', None) | |||
| if not provisioning_state: | |||
| additional_properties = getattr(properties, 'additional_properties', {}) | |||
| provisioning_state = additional_properties.get('provisioningState') | |||
There was a problem hiding this comment.
For this case, is privisioningState always existing ?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
better to add some comments here to explain it
| 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: |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
The code below line 245 is for automatically setting resource_group parameter.
| g.custom_command('list', 'list_keyvault') | ||
| g.show_command('show', 'get') | ||
|
|
||
| g.command('purge', 'begin_purge_deleted', supports_no_wait=True) |
There was a problem hiding this comment.
Are these new commands ? It seems that we didn't add test for them.
There was a problem hiding this comment.
@arrownj No, it's an old command. Actually purge command has been already covered:
There was a problem hiding this comment.
seems line 71 ~ 73 is duplicated with line 81 ~ 83
There was a problem hiding this comment.
seems line 71 ~ 73 is duplicated with line 81 ~ 83
@arrownj Good catch! Removed.
@arrownj dev branch also has this file. Binary files wouldn't show anything in git diff. |
| @@ -35,6 +35,11 @@ | |||
| from msrest.exceptions import HttpOperationError | |||
| """ | |||
|
|
|||
| track2_header = copyright_header + b"""import msrest.serialization | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@yungezz Actually the SDK is great, the problem is within this patch_models.py script. This script is totally a hack script.
| @@ -220,7 +229,15 @@ def find_autorest_generated_folder(module_prefix="azure.mgmt"): | |||
| if __name__ == "__main__": | |||
| logging.basicConfig(level=logging.INFO) | |||
|
|
|||
| track2_packages = [ | |||
| 'azure.mgmt.keyvault' | |||
There was a problem hiding this comment.
will future track2 mgmt sdks need to add this line also?
There was a problem hiding this comment.
@yungezz Yes, I think. Anyway, this is a hack script and maybe we should totally understand it and try to deprecate it.
There was a problem hiding this comment.
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.
| @@ -636,6 +636,9 @@ def get_provisioning_state(instance): | |||
| properties = getattr(instance, 'properties', None) | |||
| if properties: | |||
| provisioning_state = getattr(properties, 'provisioning_state', None) | |||
| if not provisioning_state: | |||
| additional_properties = getattr(properties, 'additional_properties', {}) | |||
| provisioning_state = additional_properties.get('provisioningState') | |||
There was a problem hiding this comment.
better to add some comments here to explain it
| @@ -388,3 +388,12 @@ | |||
| az keyvault update --enabled-for-disk-encryption true --name MyKeyVault --resource-group MyResourceGroup | |||
| crafted: true | |||
| """ | |||
|
|
|||
| helps['keyvault wait'] = """ | |||
There was a problem hiding this comment.
is this a new command? or just fix of help file?
There was a problem hiding this comment.
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.
| no_wait = kwargs.pop('no_wait') | ||
|
|
||
| if cmd.cli_ctx.cloud.profile == 'latest': | ||
| function_name = 'begin_' + function_name |
There was a problem hiding this comment.
this is also specific to track2 right?
There was a problem hiding this comment.
@yungezz Yes, track 2 only.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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)
|
Hi @fengzhou-msft You are the code owner of |
| g.custom_command('add', 'add_network_rule', supports_no_wait=True) | ||
| g.custom_command('remove', 'remove_network_rule', supports_no_wait=True) |
There was a problem hiding this comment.
in these scenarios, is there a wait command to wait util condition satisfied?
There was a problem hiding this comment.
@Juliehzl Good catch. Added.
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.