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] - SMS - Refactor SMS SDK and add *withResponse methods #19666

Merged
merged 6 commits into from
Mar 6, 2021
Merged

[Communication] - SMS - Refactor SMS SDK and add *withResponse methods #19666

merged 6 commits into from
Mar 6, 2021

Conversation

minnieliu
Copy link
Member

This PR updates:

  • README.md / CHANGLOG
  • Async and Sync SMS Tests
  • Async and Sync SMSClients
  • SMSClientBuilder

@minnieliu minnieliu requested a review from a team as a code owner March 5, 2021 20:22
@ghost ghost added the Communication label Mar 5, 2021
sacheu
sacheu approved these changes Mar 5, 2021
View reviewed changes
Copy link

@sacheu sacheu left a comment

Choose a reason for hiding this comment

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

Changes looks good. General Question: Do we need to apply this 'withResponse' method to other langs sdks?

@minnieliu
Copy link
Member Author

minnieliu commented Mar 5, 2021 •
edited
Loading

Changes looks good. General Question: Do we need to apply this 'withResponse' method to other langs sdks?
Thanks Sam. And no, "withResponse" is a specific Java SDK Guideline: Access Key Authentication
@sacheu

sacheu
sacheu reviewed Mar 5, 2021
View reviewed changes
...cation/azure-communication-sms/src/test/java/com/azure/communication/sms/SmsClientTests.java
public void sendSmsToGroup(HttpClient httpClient) {
// Arrange
SmsClientBuilder builder = getSmsClient(httpClient);
client = setupSyncClient(builder, "sendSmsToGroupSync");
Copy link

Choose a reason for hiding this comment

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

name should be without Sync

Copy link
Member Author

Choose a reason for hiding this comment

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

For all the Sync test cases, I've put "Sync" so that we can differentiate it from the async test cases. I didn't think it was necessary to suffix all the Async test cases with "Async" since it's a blit clunky.

Copy link
Member

Choose a reason for hiding this comment

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

how does this work in other sdks? i would've thought the default was sync, and async needs to be specified.
I'm ok as long as it's consistent

Copy link
Member Author

Choose a reason for hiding this comment

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

I've named it with the same convention with the other SDKs as well :)

@minnieliu
Copy link
Member Author

/azp run java - communication - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@minnieliu minnieliu merged commit 536abd6 into Azure:master Mar 6, 2021
beltr0n
beltr0n reviewed Mar 6, 2021
View reviewed changes
sdk/communication/azure-communication-sms/README.md

### Azure Active Directory Token Authentication
A `DefaultAzureCredential` object must be passed to the `SmsClientBuilder` via the credential() function. Endpoint and httpClient must also be set via the endpoint() and httpClient() functions respectively.
Copy link
Member

Choose a reason for hiding this comment

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

I'm ok using the DefaultAzureCredential as our example, but this statement makes me think we can only use DefaultAzureCredential. The .net readme has "SMS clients can also be authenticated using a valid token credential." So maybe something more like that?

sdk/communication/azure-communication-sms/README.md
SmsClient smsClient = new SmsClientBuilder()
.connectionString(connectionString)
.httpClient(httpClient)
.buildClient();
```

## Key concepts

There are two different forms of authentication to use the Azure Communication SMS Service.
Copy link
Member

Choose a reason for hiding this comment

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

this should be before instead of after the auth section

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beltr0n beltr0n beltr0n left review comments

@sacheu sacheu sacheu approved these changes

@paolamvhz paolamvhz paolamvhz left review comments

@RezaJooyandeh RezaJooyandeh RezaJooyandeh approved these changes

@ankitarorabit ankitarorabit ankitarorabit approved these changes

@JianpingChen JianpingChen Awaiting requested review from JianpingChen

@jbeauregardb jbeauregardb Awaiting requested review from jbeauregardb

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

Successfully merging this pull request may close these issues.
6 participants
@minnieliu @beltr0n @RezaJooyandeh @sacheu @paolamvhz @ankitarorabit