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.

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

Add SDK for Tiering #26121

Merged
merged 24 commits into from
Jun 16, 2023
Merged

Conversation

classBabacar-msft
Copy link
Contributor

@classBabacar-msft classBabacar-msft commented Jun 6, 2023

This is the (internal/private) SDK for Tiering, currently, retrieves limits for each capability (SMS/PSTN Calling/Phone Number purchasing) for a specific resource/tier.

On a high level all of these endpoints are here to achieve that:

Get acquired number limits - how many numbers a resource has already acquired and the limits
Get tier info - retrieving tier information for a resource

@github-actions github-actions bot added Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Jun 6, 2023
@github-actions
Copy link

github-actions bot commented Jun 6, 2023

Thank you for your contribution @classBabacar-msft! We will review the pull request and get back to you soon.

@@ -0,0 +1,142 @@
# Azure Communication Tiering client library for JavaScript

The Tiering client library allows developers to retrieves limits for each capability (SMS/PSTN Calling/Phone Number purchasing) for a specific resource/tier.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The Tiering client library allows developers to retrieves limits for each capability (SMS/PSTN Calling/Phone Number purchasing) for a specific resource/tier.
The Tiering client library allows developers to retrieve limits for each capability (SMS/PSTN Calling/Phone Number purchasing) for a specific resource/tier.

*
* @param options - Additional request options.
*/
public getAcquiredNumberLimits(
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a very thin wrapper around the generated code. I wonder whether we should just create a Rest-Level-Client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How exactly would I do that?

Copy link
Member

Choose a reason for hiding this comment

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

Here's the doc https://github.com/Azure/azure-sdk-for-js/blob/main/documentation/RLC-Swagger-quickstart.md?plain=1#L1

It basically means you publish the generated code without having to add and maintain high-level clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see, if the client is lightweight its best to use RLC, one thing I should note is we do plan on growing this client in size as we go further down the line with Tiering. Would it make sense to keep it a RLC at that point? Because at that moment in time this SDK will be equal or greater than the recipient verification SDK in size.

Copy link
Member

Choose a reason for hiding this comment

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

@classBabacar-msft it's not about lightweight or not, but whether you would do heavy customization on top of the generated code to provide better customer experiences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't know if we will do heavy customization yet, wouldn't it be best to keep it like it is currently? Because I would prefer to keep it this way to match the other SDK's we created under "communication" in case we plan to make a big change to most of them in the future.

Copy link
Member

Choose a reason for hiding this comment

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

that's a fair point. I am fine with having a High Level Client. We probably want a better developer experiences for a HLC.

v3: true
title: Tiering Client
use-extension:
"@autorest/typescript": "6.0.0-rc.1"
Copy link
Member

Choose a reason for hiding this comment

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

would GA version work for you?

Suggested change
"@autorest/typescript": "6.0.0-rc.1"
"@autorest/typescript": "6.0.0"

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 it should be fine, fixed.

```yaml
package-name: "@azure/communication-tiering"
description: The tiering client library retrieves limits for each capability (SMS/PSTN Calling/Phone Number purchase) for a specific resource/tier.
package-version: 1.0.0-beta.0
Copy link
Member

Choose a reason for hiding this comment

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

first version should probably be beta.1

Suggested change
package-version: 1.0.0-beta.0
package-version: 1.0.0-beta.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}

// @public (undocumented)
export const AssetDetailsMapper: coreClient.CompositeMapper;
Copy link
Member

Choose a reason for hiding this comment

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

The mappers shouldn't be in the public api surface. Could you check why they are exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok fixed.

*
* @param options - Additional request options.
*/
public getAcquiredNumberLimits(
Copy link
Member

Choose a reason for hiding this comment

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

that's a fair point. I am fine with having a High Level Client. We probably want a better developer experiences for a HLC.

public getAcquiredNumberLimits(
resourceId: string,
options: NumberAllotmentGetAcquiredNumberLimitsOptionalParams = {}
): Promise<NumberAllotmentGetAcquiredNumberLimitsResponse> {
Copy link
Member

Choose a reason for hiding this comment

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

can you define a return type directly for getAcquiredNumberLimits() instead of re-exporting generated one. Indirections make it harder to see the shape of the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

public getTierByResourceId(
resourceId: string,
options: TieringGetByResourceIdOptionalParams = {}
): Promise<TieringGetByResourceIdResponse> {
Copy link
Member

Choose a reason for hiding this comment

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

can we return Promise<AcsTier>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -23,7 +23,7 @@ describe(`TieringClient - Get Acquired Number Limits`, function () {

it("get acquired number limits", async function () {
// print all acquire number limits
const resourceId = "9d787bd6-07fc-4c7b-8e57-17f1fee41298";
const resourceId = env.RESOURCE_ID || "9d787bd6-07fc-4c7b-8e57-17f1fee41298";
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest avoiding hardcoding this value and simply read from the env vars. In any case, in recordedClient.ts you are already setting up this same GUID as the value for env.RESOURCE_ID whenever running in playback mode, which should be more than enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok that makes sense

@classBabacar-msft
Copy link
Contributor Author

@danielortega-msft Can you hit the merge button, I don't have authorization to do so

@danielortega-msft danielortega-msft merged commit ac2f393 into Azure:main Jun 16, 2023
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Oct 16, 2023
Release machinelearningservices microsoft.machine learning services 2023 10 01 shadow (Azure#26104)

* Adds base for updating Microsoft.MachineLearningServices from version stable/2023-04-01 to version 2023-10-01

* fix rebase

* Updates API version in new specs and examples

* mfe.json update description to fix linting errors (Azure#24968)

Co-authored-by: Kayla Ames <[email protected]>

* add headers for start/stop/restart compute examples (Azure#25072)

* Adding SubscriptionId and ResourceGroup for Azure Datastore (Azure#25289)

* Add query param in list job api (Azure#25374)

Co-authored-by: Shail Paragbhai Shah <[email protected]>

* [MachineLearning]Try fixing some lint error in 10-01 version (Azure#25519)

* Try fixing some lint error

* bug fix

* remove unused definition

* Update Example

* adding queue settings to Oct 2023 stable api version (Azure#25765)

* adding queue settings to Oct 2023 stable api version

* update new line

---------

Co-authored-by: Aaheli Chattopadhyay <[email protected]>

* remove job priority (Azure#25775)

Co-authored-by: Aaheli Chattopadhyay <[email protected]>

* Adding MachineLearningService workspace resource publish API and registry datareference API (Azure#25619)

* Adding machinelearningservices workspace publish and registry
datareference API

* minimize diff

* minor updates on put job api summary

* remove two unneeded APIs

* remove extra adding long running annotation

* Add managed Vnet to stable version (Azure#25729)

* Add managed Vnet to stable version

* update missing parameter

* Update missing ref

* meaningless commit trigger pipeline again

* Revert "meaningless commit trigger pipeline again"

This reverts commit baa386b8cf0abb96b461a27d9060188e7ed0581e.

* add missing ref

* Update example version

* trigger pipeline

* Revert "trigger pipeline"

This reverts commit f989c4c91ca85dde28fa636ed82ba9c742a05db9.

* add force to purge parameter

* trigger pipeline

* Revert "trigger pipeline"

This reverts commit 42ea0f8c2dc17e42757112b279fae42a4dc6e557.

* Update examples

* Add format and update examples

* [Model Monitoring][GA] - Add model monitoring scenarios (Azure#25801)

* Add model monitoring scenarios

* add

* add

* Update mfe.json

* Update mfe.json

* Update mfe.json

* Update mfe.json

* add ws vnet properties (Azure#25882)

Co-authored-by: Aaheli Chattopadhyay <[email protected]>

* Fix circular reference 2023-10-01 (Azure#25879)

* TriggerType->ComputeTriggerType

* RecurrenceFrequency->ComputeRecurrenceFrequency

* RecurrenceSchedule->ComputeRecurrenceSchedule

* description

---------

Co-authored-by: Kayla Ames <[email protected]>

* Adding proxy resource (Azure#25904)

Co-authored-by: Shail Paragbhai Shah <[email protected]>

* Add feature store api's to GA version (Azure#25594)

* Add examples

* Adding feature store GA api's

* Sync examples

* Add examples

* Converting to ProxyResource

---------

Co-authored-by: Shail Paragbhai Shah <[email protected]>

* add serverless compute settings (Azure#26012)

Co-authored-by: Aaheli Chattopadhyay <[email protected]>

* [MachineLearningService]Add featurestore property to stable version (Azure#26121)

* Add workspace kind and featureStoreSettings

* add description

* Add description for ComputeRuntimeDti

* remove featurestore settings for lint

* remove Kind

* Revert "remove featurestore settings for lint"

This reverts commit f4d7ae352baf966aeba9085bd06df3456bd17bef.

* Update readme

fix

---------

Co-authored-by: Kayla Ames <[email protected]>
Co-authored-by: libc16 <[email protected]>
Co-authored-by: Chunyu Li <[email protected]>
Co-authored-by: shail2208 <[email protected]>
Co-authored-by: Shail Paragbhai Shah <[email protected]>
Co-authored-by: ZhidaLiu <[email protected]>
Co-authored-by: ac923 <[email protected]>
Co-authored-by: Aaheli Chattopadhyay <[email protected]>
Co-authored-by: chaoyu-msft <[email protected]>
Co-authored-by: Mathieu St-Louis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants