[Key Vault Secrets] keyId should be populated, as a string #9639
Conversation
| @@ -4,6 +4,8 @@ | |||
|
|
|||
| - Fixed [bug 8378](https://github.com/Azure/azure-sdk-for-js/issues/8378), which caused the challenge based authentication to re-authenticate on every new request. | |||
| - Fixed [bug 9005](https://github.com/Azure/azure-sdk-for-js/issues/9005), which caused parallel requests to throw if one of them needed to authenticate. | |||
| - Fixed a bug in which the `keyId` was missing on the secret properties. | |||
| - Removed the dependency of the TypeScript types for the `dom`. | |||
There was a problem hiding this comment.
How is this change related to populating keyId?
There was a problem hiding this comment.
Before this specific change: https://github.com/Azure/azure-sdk-for-js/pull/9639/files#diff-f3c93f553a685134bd5ce75e2fbad25eR961 keyId wouldn't be populated, because none of the previous objects had this property.
| @@ -123,7 +123,7 @@ export interface SecretProperties { | |||
| enabled?: boolean; | |||
| readonly expiresOn?: Date; | |||
| id?: string; | |||
| readonly keyId?: URL; | |||
| readonly keyId?: string; | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
KeyVault Certificates uses string already. That if that you mention couldn't have been used because this property was never received as keyId.
There was a problem hiding this comment.
If someone has written the code Ramya wrote above with a KeyVaultSecret, then it will break their compilation if we change the type signature to string, even though the current implementation can't set it to anything at runtime.
There was a problem hiding this comment.
I'm assuming that we could get away with it since we're in preview. What is the recommended path forward? If it's a version change, I can do that.
There was a problem hiding this comment.
This property was introduced in 4.0 GA, so even though 4.1 is in preview, it create a breaking change between 4.0 and 4.1 to remove something from the API.
I think we have to put a deprecation on the docs for keyId that mentions that the property will never be defined and create a new property name (I agree with Ramya that something that lets a customer know that this is about correlation to a certificate would be good). When we eventually do 5.0 we can remove the property entirely.
There was a problem hiding this comment.
I don't think we should deprecate keyId since it will be a misalignment with the other languages.
There was a problem hiding this comment.
we could deprecate keyId in the GA versions and re-introduce it in 4.1.0? in the GA versions we would introduce kid, which is the one that gets populated? (then deprecate kid again in 4.1.0)
|
Closing on favor of a new issue: #10726 |
…3908) ## what - Deprecate `secretProperties.keyId` and replace it with `secretProperties.certificateKeyId` - Remove `dom` from tsconfig ## why `secretProperties.keyId` being a URL is the only reason we need `dom` lib in this library. Even though it has never been populated we cannot remove it because we might break someone's compilation (although it would have never worked at runtime). Instead we'll deprecate it, note that it will always be undefined, and make it an any so we can remove the dependency on `dom`. Refer to #9639 for previous work in this area. Resolves #10726
Just like how we're doing in certificates: https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/keyvault/keyvault-certificates/src/index.ts#L2130
This is a bug fix, really.