Skip to content

Conversation

@houk-ms
Copy link
Contributor

@houk-ms houk-ms commented Apr 14, 2021

Description

To support the new features for MHSM (soft-delete, private link, etc.), we have to upgrade the keyvault management sdk api version to 2021-04-01-preview.

Testing Guide

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change.
[Component Name 2] az command b: Add some customer-facing feature.


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

@houk-ms houk-ms added the KeyVault az keyvault label Apr 14, 2021
@houk-ms houk-ms requested a review from yungezz April 14, 2021 06:23
@houk-ms houk-ms self-assigned this Apr 14, 2021
@houk-ms
Copy link
Contributor Author

houk-ms commented Apr 14, 2021

The PR is based on private package. We need hold the PR until the azure-mgmt-keyvault==9.0.0 is officially released. CI won't pass before the official release.

The PR can be merged after the official SDK is released and CI passes.

@houk-ms
Copy link
Contributor Author

houk-ms commented Apr 20, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

- AZURECLI/2.21.0 azsdk-python-azure-mgmt-keyvault/8.0.0 Python/3.7.4 (Windows-10-10.0.19041-SP0)
method: PUT
uri: https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/cli_test_disk_encryption_set_update_000001/providers/Microsoft.KeyVault/vaults/vault1-000002?api-version=2019-09-01
uri: https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/cli_test_disk_encryption_set_update_000001/providers/Microsoft.KeyVault/vaults/vault1-000002?api-version=2021-04-01-preview
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure it can pass, or mark it if it fails. I do not recommend editing recording file. Just run it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, i will rerun live test for all the test related

@houk-ms
Copy link
Contributor Author

houk-ms commented Apr 21, 2021

All the tests were lively runned. Except for the following cases.

The following live tests also failed in live test pipeline. We modified their API version directly.

  • command_modules/cdn/tests/latest/recordings/test_cdn_custom_domain_byoc_latest.yaml
  • command_modules/cdn/tests/latest/recordings/test_cdn_custom_domain_https_msft.yaml
  • command_modules/eventhubs/tests/latest/recordings/test_eh_namespace_byok.yaml

The following tests are record-only. We modified their API version directly.

  • command_modules/keyvault/tests/latest/recordings/test_keyvault_hsm_mgmt.yaml
  • command_modules/synapse/tests/latest/recordings/test_workspace_with_cmk.yaml

ResourceType.MGMT_RESOURCE_TEMPLATESPECS: '2019-06-01-preview',
ResourceType.MGMT_NETWORK_DNS: '2018-05-01',
ResourceType.MGMT_KEYVAULT: '2020-04-01-preview',
ResourceType.MGMT_KEYVAULT: '2021-04-01-preview',
Copy link
Member

@jiasli jiasli Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not reasonable to mark az keyvault as GA while the underlying API is preview, not to mention 2021-04-01-preview doesn't even have REST API doc:

https://docs.microsoft.com/en-us/rest/api/keyvault/vaults/get

image

Due to the lack of REST API doc, there will be no customer-facing document for CLI's output format either (#17332).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more like a problem across all CLI command modules. I don't have a good idea about this

self.check('privateLinkServiceConnectionState.status', 'Approved'),
self.check('privateLinkServiceConnectionState.description', '{approval_desc}'),
self.check('provisioningState', 'Succeeded')
self.check('provisioningState', 'Updating')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the state could be both:

Suggested change
self.check('provisioningState', 'Updating')
self.check_pattern('provisioningState', '(?:Updating|Succeeded)')

Copy link
Contributor Author

@houk-ms houk-ms Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, how does it work? (?:Updating|Succeeded). Locally tested it, seems it can not make the test pass.

Copy link

@v-Ajnava v-Ajnava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed EventHub test : LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.