{Core} Add . in prefix for operation handler #12702
Conversation
Juliehzl
requested review from
arrownj,
fengzhou-msft,
haroldrandom,
jiasli and
qianwens
as code owners
|
add to S167 |
|
May I have some contexts? |
| @@ -534,7 +534,7 @@ def get_op_handler(self, operation, operation_group=None): | |||
| from azure.cli.core.profiles._shared import get_versioned_sdk_path | |||
|
|
|||
| for rt in AZURE_API_PROFILES[self.cli_ctx.cloud.profile]: | |||
| if operation.startswith(rt.import_prefix): | |||
| if operation.startswith(rt.import_prefix + '.'): | |||
There was a problem hiding this comment.
While this does handles some scenarios (please provide some real use cases in the PR description), this still can't handle the keyvault track 1 and 2 SDKs' compatibility:
Track 1 SDK:
azure.keyvault
Track 2 SDK:
azure.keyvault.secretsazure.keyvault.keys
azure.keyvault.secrets.SecretClient.xxx and azure.keyvault.keys.KeyClient.xxx will be matched as Track 1 SDK azure.keyvault..
There was a problem hiding this comment.
the change itself looks resolving the issue mentioned in description, same question on is there multiple namespaces containing dot after first dot? such as azure.multipleapi.storage.blob, azure.multipleapi.storage.file
There was a problem hiding this comment.
@jiasli If azure.keyvault is totally replaced with submodules such as azure.keyvault.secrets and azure.keyvault.keys, there will be no problem.
But if the resource type for azure.keyvault still exists with new resource types with azure.keyvault.secrets and azure.keyvault.keys, it will have some problem.
@yungezz There are packages as you mentioned, e.g. azure.mgmt.resource in https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure/cli/core/profiles/_shared.py#L44-L49 .
There was a problem hiding this comment.
azure-cli/src/azure-cli-core/azure/cli/core/profiles/_shared.py
Lines 44 to 49 in 31a9724
This doesn't have problem because there is no azure.mgmt.resource that can cause the same conflict as azure.keyvault.
I am ok with this for now, but still we need to figure out a better solution.
There was a problem hiding this comment.
Yes. If you want to using submodules for keyvault, you cannot use azure.keyvault any more.
|
What's the background of this modification? |
|
This sentence "With the change, we can prevent such scenario and go into the" is incomplete |
I have updated the description and you can take a look. |
Thanks! Updated. 😊 |
History Notes:
(Fill in the following template if multiple notes are needed, otherwise PR title will be used for history note.)
[Component Name 1] (BREAKING CHANGE:) (az command:) make some customer-facing change.
[Component Name 2] (BREAKING CHANGE:) (az command:) make some customer-facing change.
Description:ResourceTypeis defined as enum with the following structure for each element:DATA_STORAGE = ('azure.multiapi.storage', None)If we have two Resource Types named
DATA_STORAGEandDATA_STORAGESYNCwith the following definition:and the following api versions in latest profile:
For the command type with
operations_tmpl = 'azure.mgmt.storagesync.operations#ManagementPoliciesOperations.{}', the condition will be fit for resource typeDATA_STORAGEin original code and CLI will go into the pathazure.multiapi.storagewith api version defined in
DATA_STORAGE, notazure.multiapi.storagesync:azure-cli/src/azure-cli-core/azure/cli/core/__init__.py
Lines 537 to 540 in ac20369
But the right api version should be in
ResourceType.DATA_STORAGESYNC.With the change, we can prevent such scenario and go into azure-multiapi-storagesync package as expected.
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.