-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[communication] add LROs #11713
[communication] add LROs #11713
Conversation
/** | ||
* Represents the poller client used internally. | ||
*/ | ||
client: PhoneNumberPollerClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I know we've been using this pattern in other clients, but the client's don't need to live as part of the operation state. The client can be passed only through parameters. I'll make a PR today to showcase this. Please ping me if you don't hear of me today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added you as a reviewer to this PR: #11717 it's a cleanup of the Key Vault Keys LROs, based on a recent brainstorming session I had with @willmtemple ! Reviews appreciated 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this :-)
Left a few initial comments. I'm curious to see how the code gets simpler once we don't use the internal Client subset in the poller state objects.
sdk/communication/communication-administration/review/communication-administration.api.md
Outdated
Show resolved
Hide resolved
sdk/communication/communication-administration/review/communication-administration.api.md
Outdated
Show resolved
Hide resolved
sdk/communication/communication-administration/src/phoneNumber/lro/utils.ts
Outdated
Show resolved
Hide resolved
...ommunication/communication-administration/src/phoneNumber/phoneNumberAdministrationClient.ts
Outdated
Show resolved
Hide resolved
sdk/communication/communication-administration/test/lro.purchase.spec.ts
Outdated
Show resolved
Hide resolved
sdk/communication/communication-administration/test/lro.purchase.spec.ts
Outdated
Show resolved
Hide resolved
…/lro/utils.ts Co-authored-by: Dominik <domessin@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greate work, I've done another pass and added some more comments. I agree that once those are addressed we can merge this and refactor later.
@@ -397,6 +416,12 @@ export interface PhoneNumberEntity { | |||
status?: string; | |||
} | |||
|
|||
// @public | |||
export interface PhoneNumberPollerOptions extends OperationOptions { | |||
intervalInMs?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the guidelines want us to name this pollInterval
but I like that we're specifying milliseconds here.
https://azure.github.io/azure-sdk/typescript_design.html#ts-lro
@xirzec What would the right property name?
|
||
await purchasePoller.pollUntilDone(); | ||
assert.ok(purchasePoller.getOperationState().isCompleted); | ||
}).timeout(60000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the timeout need to be that high?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea the test actually took about that time to run
sdk/communication/communication-administration/test/lro.reserve.spec.ts
Outdated
Show resolved
Hide resolved
|
||
const { status } = await client.getReservation(reservationId); | ||
assert.equal(status, "Cancelling" || "Cancelled"); | ||
}).timeout(30000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be shorter?
sdk/communication/communication-administration/test/lro.purchase.spec.ts
Outdated
Show resolved
Hide resolved
sdk/communication/communication-administration/src/phoneNumber/lro/purchase/operation.ts
Outdated
Show resolved
Hide resolved
sdk/communication/communication-administration/review/communication-administration.api.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Lgtm, let's get this merged
Fixed some more Swagger issues (Azure#11713) Co-authored-by: Amogh Natu <amnat@microsoft.com>
This PR:
beginReservePhoneNumbers
LRObeginReservePhoneNumbers
LRObeginReleasePhoneNumbers
LRObeginPurchaseReservation
LRObeginPurchaseReservation
LRObeginReservePhoneNumbers
LRO recorded testsbeginReleasePhoneNumbers
LRO recorded testsbeginPurchaseReservation
LRO recorded testsPhoneNumberSearch
toPhoneNumberReservation
on code generationsearchId
toreservationId
on code generation