Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preview support for Key Vault 7.1 #10124

Merged
merged 6 commits into from
Mar 7, 2020
Merged

Conversation

chlowell
Copy link
Member

@chlowell chlowell commented Mar 6, 2020

This adds support for Key Vault version 7.1-preview to azure-keyvault-* 4.2.0b1. This PR looks daunting but it's mostly yaml and duplicating shared code. The public API change is small:

@chlowell chlowell added KeyVault Client This issue points to a problem in the data-plane of the library. labels Mar 6, 2020
@chlowell chlowell self-assigned this Mar 6, 2020
@adxsdk6
Copy link

adxsdk6 commented Mar 6, 2020

Can one of the admins verify this patch?

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

Reviewed the non-test files. A couple of questions/comments, but otherwise LGTM.

@property
def recoverable_days(self):
# type: () -> Optional[int]
"""The number of days the certificate is retained before being deleted from a soft-delete enabled Key Vault.
Copy link
Member

Choose a reason for hiding this comment

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

If this is consistent already, leave it. But it would really be "soft delete-enabled" when used as an adjective. I changed all these in .NET. Again, though, just leave it if this is consistent.

sdk/keyvault/azure-keyvault-keys/CHANGELOG.md Show resolved Hide resolved
@@ -40,7 +40,8 @@ class CertificateClient(KeyVaultClientBase):
:param str vault_url: URL of the vault the client will access. This is also called the vault's "DNS Name".
:param credential: An object which can provide an access token for the vault, such as a credential from
:mod:`azure.identity`
:keyword str api_version: version of the Key Vault API to use. Defaults to the most recent.
:keyword api_version: version of the Key Vault API to use. Defaults to the most recent.
:paramtype api_version: ~azure.keyvault.certificates.ApiVersion
Copy link
Contributor

Choose a reason for hiding this comment

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

should this include "or str" since it's an enum

Copy link
Member Author

Choose a reason for hiding this comment

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

Our other enums allow arbitrary string values because they subclass str; that isn't a feature of Enum. ApiVersion subclasses only Enum because it doesn't need to be forward compatible.

However, it's true a string works for api_version here, provided that string is a valid ApiVersion value. That's by design, to prevent a breaking change for anyone passing a string. I mention only ApiVersion in the docstring to point everyone else at the type they should be using.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
None yet
5 participants