[Key Vault Keys] LRO refactoring #11717
Conversation
ghost
added
the
| cancel, | ||
| toString | ||
| }; | ||
| return new DeleteKeyPollOperation(state, this.vaultUrl, this.client, this.requestOptions); |
There was a problem hiding this comment.
I'm curious, why return a new DeleteKeyPollOperation if the only state inside the object is inside state, but we've been mutating this.state inside this method, since we just assigned it to a local variable without copying it.
It seems like either we shouldn't be mutating state or we shouldn't bother creating a new DeleteKeyPollOperation.
There was a problem hiding this comment.
I'll update it to return this! Thank you!
We talked about this over private chat and the reason why update expects a return is because of: https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/core/core-lro/src/poller.ts#L316
The current contract of the poller allows for stateless workflows.
I think we made decisions like this in core-lro because of the flexibility they give us. So, no rush on making that contract change, in my opinion!
There was a problem hiding this comment.
We could do something like: this.operation = update() || this.operation, for example. Even more flexibility! (though the argument that we might not need this much flexibility is valid. I just feel more comfortable making things less restrictive 😅)
| const keyBundle = bundle as KeyBundle; | ||
| const deletedKeyBundle = bundle as DeletedKeyBundle; |
There was a problem hiding this comment.
Feels a little weird to work with the same object in two different variables with two different interfaces.
There was a problem hiding this comment.
This transformation was already there, but I agree!
| if (attributes.vaultUrl) { | ||
| delete (resultObject.properties as any).vaultUrl; | ||
| } | ||
| if (attributes.expires) { | ||
| delete (resultObject.properties as any).expires; | ||
| } | ||
| if (attributes.created) { | ||
| delete (resultObject.properties as any).created; | ||
| } | ||
| if (attributes.updated) { | ||
| delete (resultObject.properties as any).updated; | ||
| } |
There was a problem hiding this comment.
can we find a way to not spread these onto resultObject in the first place? Even if we have to copy attributes to its own object, delete them, then spread the remainder?
There was a problem hiding this comment.
I was about to do this, but the context switch is sufficient to give me a headache, since this might have consequences on the public API (hopefully not), so I made an issue instead: #11720
There was a problem hiding this comment.
I have an issue to remove all the object spreading on Key Vault: #9730 I'll do it as part of that issue!
sadasant
force-pushed
the
keyvault-keys/lro-refactoring-11698
branch
from
5611add
to
dfba0a4
Compare
| * @summary Reaches to the service and updates the delete key's poll operation. | ||
| * @param [options] The optional parameters, which are an abortSignal from @azure/abort-controller and a function that triggers the poller's onProgress function. | ||
| */ | ||
| public async update( |
There was a problem hiding this comment.
This pattern of managing the client seems pretty nice, and definitely less leaky than before. It feels like a good balance between ruthless abstraction and the levels of duplication that we had previously, but it doesn't seem like it's much less verbose than what we had before.
It still feels bizarre to me that we have this state machine model where update can return a different operation, but I can't think of anywhere that we use it. That API suggests to me that returning a different kind of operation should be the way that we make the pollers progress in state, but instead we just have monolithic update functions that handle everything and then return this.
There was a problem hiding this comment.
I understand how this might be weird! I wonder if it would make sense to open an issue to have a discussion over time of how we could improve this in core-lro 🤔
There was a problem hiding this comment.
Yeah I think we should consider if we want this to be stateless or not (no strong feelings from me) and either we lean fully into stateless (get rid of this) or we lean fully away from it (don't bother returning this, because caller already has a copy of us)
| * The method used by the poller to wait before attempting to update its operation. | ||
| * @memberof DeleteKeyPoller | ||
| */ | ||
| async delay(): Promise<void> { |
There was a problem hiding this comment.
Could we provide a default implementation of this method in core-lro? It already depends on core-http, and I'm having a hard time thinking of where anyone would use an implementation other than this in a package, but leaving it open to overrides would still be nice if someone wanted to instrument the delay for some purpose.
There was a problem hiding this comment.
The issue with this is that we didn't want core-lro to depend on core-http 🤔 and doing an independent delay function seems prone to errors. Would you be on board if I did a default delay method (on core-lro) with a non-core-http delay?
There was a problem hiding this comment.
core-lro already depends on core-http.
⇒ < ../../core/core-lro/package.json | jq .dependencies
{
"@azure/abort-controller": "^1.0.0",
"@azure/core-http": "^1.2.0",
"events": "^3.0.0",
"tslib": "^2.0.0"
}
The true problem is that the base Poller doesn't contain a polling interval. If it had one, I think we could just use setTimeout on all targets by default? (CC @xirzec )
For cancellation we would just have to store the timer ID and handle it when the operation is cancelled.
There was a problem hiding this comment.
stopping the poller and cancelling the operation is different! but I see what you mean. I'll make an issue.
There was a problem hiding this comment.
I made an issue! #12583
There was a problem hiding this comment.
Having some kind of default delay implementation makes sense to me.
Specifying shipping address required property to autorest. (Azure#11717) * Ensuring autorest generation follow specific order of required properties in shippingAddress * added details sharp yaml
This PR intends to refactor the Key Vault Keys pollers by:
pollerClientand re-using the generated KeyVaultClient. The only caveat is that two functions had to be duplicated in the individual poller operation files.These changes don't represent a behavioral change. It's a refactoring to help us clean up other LROs.
First effort towards: #11698