Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[communication] add LROs #11713

Merged
merged 25 commits into from
Oct 28, 2020
Merged

[communication] add LROs #11713

merged 25 commits into from
Oct 28, 2020

Conversation

0rland0Wats0n
Copy link
Contributor

@0rland0Wats0n 0rland0Wats0n commented Oct 7, 2020 •
edited
Loading

This PR:

  1. adds beginReservePhoneNumbers LRO
    • implements cancel on beginReservePhoneNumbers LRO
  2. adds beginReleasePhoneNumbers LRO
  3. adds beginPurchaseReservation LRO
    • implements cancel on beginPurchaseReservation LRO
  4. adds beginReservePhoneNumbers LRO recorded tests
  5. adds beginReleasePhoneNumbers LRO recorded tests
  6. adds beginPurchaseReservation LRO recorded tests
  7. updates README to reflect new LROs
  8. renames PhoneNumberSearch to PhoneNumberReservation on code generation
  9. renames searchId to reservationId on code generation

@0rland0Wats0n 0rland0Wats0n requested review from bterlson, xirzec and a team as code owners October 7, 2020 23:19
sadasant
sadasant reviewed Oct 8, 2020
View reviewed changes
sdk/communication/communication-administration/src/phoneNumber/lroModels.ts Outdated
/**
* Represents the poller client used internally.
*/
client: PhoneNumberPollerClient;
Copy link
Contributor

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.

Copy link
Contributor

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 🙏

DominikMe
DominikMe reviewed Oct 9, 2020
View reviewed changes
Copy link
Member

@DominikMe DominikMe left a 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
@ramya-rao-a ramya-rao-a mentioned this pull request Oct 15, 2020
Closed
DominikMe
DominikMe reviewed Oct 26, 2020
View reviewed changes
Copy link
Member

@DominikMe DominikMe left a 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.

sdk/communication/communication-administration/README.md Outdated Show resolved Hide resolved
sdk/communication/communication-administration/README.md Outdated Show resolved Hide resolved
sdk/communication/communication-administration/README.md Outdated Show resolved Hide resolved
sdk/communication/communication-administration/review/communication-administration.api.md Outdated
@@ -397,6 +416,12 @@ export interface PhoneNumberEntity {
status?: string;
}

// @public
export interface PhoneNumberPollerOptions extends OperationOptions {
intervalInMs?: number;
Copy link
Member

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?

sdk/communication/communication-administration/test/lro.purchase.spec.ts

await purchasePoller.pollUntilDone();
assert.ok(purchasePoller.getOperationState().isCompleted);
}).timeout(60000);
Copy link
Member

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?

Copy link
Contributor Author

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
sdk/communication/communication-administration/test/lro.reserve.spec.ts

const { status } = await client.getReservation(reservationId);
assert.equal(status, "Cancelling" || "Cancelled");
}).timeout(30000);
Copy link
Member

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/swagger/PhoneNumber.md Show resolved Hide resolved
sdk/communication/communication-administration/src/phoneNumber/lro/purchase/operation.ts Outdated Show resolved Hide resolved
DominikMe
DominikMe approved these changes Oct 27, 2020
View reviewed changes
Copy link
Member

@DominikMe DominikMe left a 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

@DominikMe DominikMe merged commit 351da5f into Azure:master Oct 28, 2020
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Nov 18, 2020
CodeGen from PR 11713 in Azure/azure-rest-api-specs
9acaaf8 
Fixed some more Swagger issues (Azure#11713)

Co-authored-by: Amogh Natu <amnat@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sadasant sadasant sadasant left review comments

@DominikMe DominikMe DominikMe approved these changes

@RezaJooyandeh RezaJooyandeh Awaiting requested review from RezaJooyandeh

@ankitarorabit ankitarorabit Awaiting requested review from ankitarorabit

@whisper6284 whisper6284 Awaiting requested review from whisper6284

@bterlson bterlson Awaiting requested review from bterlson

@xirzec xirzec Awaiting requested review from xirzec

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@0rland0Wats0n @sadasant @DominikMe