Skip to content

[KeyVault] Adds abortSignal to all the *Options we have in keys and secrets#4383

Merged
sadasant merged 1 commit intoAzure:masterfrom
sadasant:feature/fix4357
Jul 23, 2019
Merged

[KeyVault] Adds abortSignal to all the *Options we have in keys and secrets#4383
sadasant merged 1 commit intoAzure:masterfrom
sadasant:feature/fix4357

Conversation

@sadasant
Copy link
Copy Markdown
Contributor

With these changes, our current API fully supports cancelling a request
before we get a response. The idea is that this feature is already
supported by the core dependencies we use, and by passing this property
through, we'll get the benefits right away without having to do other
changes.

keyvault-keys and keyvault-secrets currently have a unit test that
proves that this works for one of the functions that shares this
underlying APIs. This PR adds the support to the other similar methods.
We could also add other tests demonstrating this behavior for these
other functions.

Fixes #4357

…ecrets

With these changes, our current API fully supports cancelling a request
before we get a response. The idea is that this feature is already
supported by the core dependencies we use, and by passing this property
through, we'll get the benefits right away without having to do other
changes.

keyvault-keys and keyvault-secrets currently have a unit test that
proves that this works for one of the functions that shares this
underlying APIs. This PR adds the support to the other similar methods.
We could also add other tests demonstrating this behavior for these
other functions.

Fixes Azure#4357
@sadasant sadasant added Client This issue points to a problem in the data-plane of the library. KeyVault labels Jul 22, 2019
@sadasant sadasant requested a review from sophiajt July 22, 2019 20:35
@sadasant sadasant self-assigned this Jul 22, 2019
@sophiajt
Copy link
Copy Markdown
Contributor

cc @bterlson - I can't remember where we landed with cancellation. Would you mind looking at this to see if it matches with your expectations?

@sophiajt sophiajt requested a review from bterlson July 22, 2019 20:37
Copy link
Copy Markdown
Member

@bterlson bterlson left a comment

Choose a reason for hiding this comment

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

LGTM (abortSignal option typed as AbortSignalLike interface)

@sadasant sadasant merged commit 5514e80 into Azure:master Jul 23, 2019
@sadasant sadasant deleted the feature/fix4357 branch July 23, 2019 16:50
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

Development

Successfully merging this pull request may close these issues.

[KeyVault] Add abortSignal as an optional property to all of the relevant interfaces

3 participants