Skip to content

[KeyVault] - Replace keyId: URL with certificateKeyId: string #13908

Merged
maorleger merged 7 commits intoAzure:masterfrom
maorleger:10726-secrets-key-id
Feb 23, 2021
Merged

[KeyVault] - Replace keyId: URL with certificateKeyId: string #13908
maorleger merged 7 commits intoAzure:masterfrom
maorleger:10726-secrets-key-id

Conversation

@maorleger
Copy link
Copy Markdown
Member

@maorleger maorleger commented Feb 22, 2021

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

Copy link
Copy Markdown
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

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

I love it! Thank you so much 🌞

@maorleger maorleger force-pushed the 10726-secrets-key-id branch from 86c6cd3 to fc42dfc Compare February 22, 2021 19:01
@maorleger
Copy link
Copy Markdown
Member Author

maorleger commented Feb 22, 2021

@bterlson / @xirzec - mind giving this a quick look when you have a second? I want to make sure this isn't harmful (or if it is, if there's a better way to handle it - any vs unknown or something else). Context here is: we had a property keyId?: URL which is the only reason we needed to pull in dom but it was never actually populated at runtime.

We can't remove the field entirely to avoid breaking compilation (see #9639) so we deprecate it, make it unknown, and introduce a better named field that's a string instead (also taken from feedback on #9639). Then we can remove dom as a dependency?

@maorleger maorleger added the APIChange This PR contains an addition or change to the API signature and must be reviewed by an architect. label Feb 22, 2021
Copy link
Copy Markdown
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

I'm a fan of this approach. We should start using deprecation more to signal when things are going to move in the next major.

@maorleger maorleger merged commit eafa579 into Azure:master Feb 23, 2021
* this field specifies the corresponding key backing the KV certificate.
* **NOTE: This property will not be serialized. It can only be populated by
* the server.**
* @deprecated Please use {@link SecretProperties.certificateKeyId} instead. This field will always be undefined.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Some may take the "this" here to refer to certificateKeyId :)

How about the below?

The keyId field has and will be left as undefined.

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

Labels

APIChange This PR contains an addition or change to the API signature and must be reviewed by an architect. KeyVault

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Key Vault Secrets] The keyId property of SecretProperties should be populated

4 participants