Feature/communication server calling preview1 #21017

zihzhan-msft · 2021-05-12T01:21:25Z

All SDK Contribution checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

Please open PR in Draft mode if it is:
- Work in progress or not intended to be merged.
- Encountering multiple pipeline failures and working on fixes.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
I have read the contribution guidelines.
The pull request does not introduce breaking changes.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

SDK Generation Guidelines

The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code. (Track 2 only)
The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK. Please double check nuget.org current release version.

Additional management plane SDK specific contribution checklist:

Note: Only applies to Microsoft.Azure.Management.[RP] or Azure.ResourceManager.[RP]

Include updated management metadata.
Update AzureRP.props to add/remove version info to maintain up to date API versions.

Management plane SDK Troubleshooting

If this is very first SDK for a services and you are adding new service folders directly under /SDK, please add new service label and/or contact assigned reviewer.
If the check fails at the Verify Code Generation step, please ensure:
- Do not modify any code in generated folders.
- Do not selectively include/remove generated files in the PR.
- Do use generate.ps1/cmd to generate this PR instead of calling autorest directly.
  Please pay attention to the @microsoft.csharp version output after running generate.ps1. If it is lower than current released version (2.3.82), please run it again as it should pull down the latest version.
Note: We have recently updated the PSH module called by generate.ps1 to emit additional data. This would help reduce/eliminate the Code Verification check error. Please run following command:
```
  `dotnet msbuild eng/mgmt.proj /t:Util /p:UtilityName=InstallPsModules`
```

Old outstanding PR cleanup

Please note:
If PRs (including draft) has been out for more than 60 days and there are no responses from our query or followups, they will be closed to maintain a concise list for our reviewers.

…nto feature/communication-ServerCalling-preview1

sdk/communication/Azure.Communication.Calling.Server/CHANGELOG.md

sdk/communication/Azure.Communication.Calling.Server/README.md

sdk/communication/Azure.Communication.Calling.Server/tests/samples/Sample1_CreateCall.md

sdk/communication/Azure.Communication.Calling.Server/src/CallClient.cs

check-enforcer · 2021-05-12T22:31:31Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them. In order to bootstrap pipelines for a new service, please perform following steps:

For data-plane/track 2 SDKs Issue the following command as a pull request comment:

/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run net - [service] - ci

For track 1 management-plane SDKs

Please open a separate PR and to your service SDK path in this file. Once that PR has been merged, you can re-run the pipeline to trigger the verification.

RezaJooyandeh

This a great start! I left some comments.

sdk/communication/Azure.Communication.Calling.Server/README.md

...mmunication/Azure.Communication.Calling.Server/src/Azure.Communication.Calling.Server.csproj

sdk/communication/Azure.Communication.Calling.Server/README.md

sdk/communication/Azure.Communication.Calling.Server/src/Models/ToneValue.cs

sdk/communication/Azure.Communication.Calling.Server/src/autorest.md

...on/Azure.Communication.Calling.Server/tests/Azure.Communication.Calling.Service.Tests.csproj

...ure.Communication.Calling.Server/tests/ServerCallingClients/ServerCallingClientsLiveTests.cs

minnieliu · 2021-05-19T17:23:29Z

...re.Communication.Calling.Server/src/Models/CommunicationUserIdentifierModel.Serialization.cs

+
+namespace Azure.Communication
+{
+    internal partial class CommunicationUserIdentifierModel : IUtf8JsonSerializable


Same question, I thought Serialization code is supposed to be generated

minnieliu · 2021-05-19T17:48:40Z

...ommunication.Calling.Server/src/Models/Events/CallRecordingStateChangeEvent.Serialization.cs

+            Optional<string> conversationId = default;
+            foreach (var property in element.EnumerateObject())
+            {
+                if (property.NameEquals("recordingId") || property.NameEquals("RecordingId"))


Questions about these Serialization classes v introduced:

Looks like this is all manually written? Why is that?

Why do the properties check names with different cases? (e.g recordingId and RecordingId)

I created an issue thread in the channel
@navali-msft

https://teams.microsoft.com/l/message/19:f17d9cf475e74e1a94033cef0d612a67@thread.skype/1622219271717?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=3e9c1fc3-39df-4486-a26a-456d80e80f82&parentMessageId=1622219271717&teamName=Azure%20Communication%20Services&channelName=SDK&createdTime=1622219271717

@minnieliu - We have added these classes to deserialize the events which user received on the callback.

The reason for providing deserialization on our end is because of issue with deserializing CommunicationIdentifierModel. As Service side only understands model defined in Communication.Common, so it is sending that information to user and user don't have way to deserialize it.

We will fix point#2.

We have serializer and deserialzer for CommunicationIdentifierModel in the Shared folder. Please refer to how the chat project is using them.

This Deserialization methods are generated if any of the response contains CommunicationIdentifierModel. In our case we don't have any such response. This is done for deserializing Events.

This is fixed.

Co-authored-by: Paresh Arvind Patil <papati@microsoft.com>

minnieliu · 2021-06-03T23:09:06Z

sdk/communication/Azure.Communication.Calling.Server/samples/Sample1_CreateCallAsync.md

+           EventSubscriptionType.DtmfReceived
+       });
+```
+```C# Snippet:Azure_Communication_Call_Tests_CreateCallAsync


The syntax for this snippet seems wrong... it is missing the end ) bracket that closes off CreateCallAsync

Thanks for catching this. Addressed.

minnieliu · 2021-06-03T23:11:11Z

sdk/communication/Azure.Communication.Calling.Server/src/ConversationClient.cs

+            }
+        }
+
+        private static T AssertNotNull<T>(T argument, string argumentName)


We have added a PR that moves these into Azure.Core and we dont need to define them manually here. #21542

Please remove and refer to Arguments.cs instead.

Synced with master and remove those two methods.

…nto feature/communication-ServerCalling-preview1

…//github.com/Azure/azure-sdk-for-net into feature/communication-ServerCalling-preview1

Fixed most issues

…nto feature/communication-ServerCalling-preview1

…//github.com/Azure/azure-sdk-for-net into feature/communication-ServerCalling-preview1

minnieliu · 2021-06-07T20:53:30Z

sdk/communication/Azure.Communication.CallingServer/samples/README.md

+ - Create a Call from a Azure Communication Resource identity to a phone number
+ - Specify request payload for the created call.
+
+ #### You can find samples for each of these functions below.


Is #### the right mark up?

Yes, we are intended to use it.

minnieliu · 2021-06-07T21:14:02Z

sdk/communication/Azure.Communication.CallingServer/src/ConversationClient.cs

+
+                return RestClient.JoinCall(
+                    conversationId: conversationId,
+                    source: CommunicationIdentifierSerializer.Serialize(source),


Do you also need to check that the source is not null since you are serializing it?

this will get addressed in the update.

minnieliu · 2021-06-07T21:14:33Z

sdk/communication/Azure.Communication.CallingServer/src/ConversationClient.cs

+
+                return await RestClient.JoinCallAsync(
+                    conversationId: conversationId,
+                    source: CommunicationIdentifierSerializer.Serialize(source),


Do you also need to check that the source is not null since you are serializing it?

good catch, this will get addressed in the update.

minnieliu · 2021-06-07T21:38:54Z

...nication/Azure.Communication.CallingServer/tests/Infrastructure/CallingServerLiveTestBase.cs

+            return InstrumentClient(client);
+        }
+
+        // Todo: add CorrelationVectorLogs


Is this still planning to be done or in the next PR?

This is addressed, added in the update.

minnieliu · 2021-06-07T21:40:36Z

...ommunication.CallingServer/tests/SessionRecords/Sample1_CallClient/CreateCallAsyncAsync.json

+    }
+  ],
+  "Variables": {
+    "AZURE_PHONE_NUMBER": "\u002B15129104698",


We should be redacting our phone numbers in the recordings. See SMS or Phone Numbers SDK https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/communication/Azure.Communication.Sms/tests/SmsClientRecordedTestSanitizer.cs

This is addressed in updates.

…nto feature/communication-ServerCalling-preview1

…//github.com/Azure/azure-sdk-for-net into feature/communication-ServerCalling-preview1

…nto feature/communication-ServerCalling-preview1

* Enable living testing. * Test Playback. * Add playaudio test record. * Add createcall test record. * Add delete call record. * Add hangup call test record. * Add cancelAllMediaOperations call test record. * Enable living tests. * Address comments.

RezaJooyandeh · 2021-06-08T00:18:26Z

...ation/Azure.Communication.CallingServer/tests/Infrastructure/CallingServerTestEnvironment.cs

+    /// </summary>
+    public class CallingServerTestEnvironment : CommunicationTestEnvironment
+    {
+        public string ToPhoneNumber => GetRecordedVariable(AzurePhoneNumber, options => options.IsSecret());


We probably need a separate phone number for these tests to avoid conflicts with the phone number and sms packages tests

RezaJooyandeh

Looks good to me for the initial PR. Feel free to merge once @minnieliu 's comments are applied.

Great job on your first PR!

zihzhan-msft and others added 7 commits May 4, 2021 23:31

Onboard ServerCalling feature project.

4abf245

Onboard Server Calling Test project.

bf61192

Onboard Server Calling Test project.

f1b4af7

New apis and models for different operations.

ea62eb0

merge

3643e72

Update Recorded Test files.

88e0887

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-net i…

c6314a3

…nto feature/communication-ServerCalling-preview1

zihzhan-msft requested review from DominikMe, RezaJooyandeh and minnieliu May 12, 2021 01:21

zihzhan-msft requested a review from benbp as a code owner May 12, 2021 01:21

ghost added the Communication label May 12, 2021

minnieliu previously requested changes May 12, 2021

View reviewed changes

Naming changes and fix build.

76983b4

zihzhan-msft requested review from zhengni-msft, satyakonmsft, navali-msft, arifsaikat-microsoft, ajaych-ms and jsandoval-msft May 12, 2021 23:12

Resolve comments.

40ac96a

RezaJooyandeh reviewed May 14, 2021

View reviewed changes

minnieliu reviewed May 19, 2021

View reviewed changes

Paresh-Arvind-Patil and others added 6 commits May 25, 2021 18:04

Removed operationContext from Recording apis (#21309)

2dec113

Co-authored-by: Paresh Arvind Patil <papati@microsoft.com>

Onboard ServerCalling feature project.

409c063

Onboard Server Calling Test project.

d1b16fa

New apis and models for different operations.

9245b49

Onboard Server Calling Test project.

37132da

Update Recorded Test files.

b6b8d50

minnieliu reviewed Jun 3, 2021

View reviewed changes

zihzhan-msft added 3 commits June 3, 2021 16:48

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-net i…

1020011

…nto feature/communication-ServerCalling-preview1

Address comments.

166c582

Merge branch 'feature/communication-ServerCalling-preview1' of https:…

4b7e450

…//github.com/Azure/azure-sdk-for-net into feature/communication-ServerCalling-preview1

zihzhan-msft and others added 10 commits June 3, 2021 19:08

Specify parameters in RestApiCall.

fc693ef

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-net i…

36d1328

…nto feature/communication-ServerCalling-preview1

Update Namespace to CallingServer.

a20e269

Update links in md files.

c014fb2

Update links in md files.

f67c267

Merge branch 'feature/communication-ServerCalling-preview1' of https:…

4111f57

…//github.com/Azure/azure-sdk-for-net into feature/communication-ServerCalling-preview1

Merge branch 'feature/communication-ServerCalling-preview1' of https:…

4d5e510

…//github.com/Azure/azure-sdk-for-net into feature/communication-ServerCalling-preview1

Merge branch 'feature/communication-ServerCalling-preview1' of https:…

e42c846

…//github.com/Azure/azure-sdk-for-net into feature/communication-ServerCalling-preview1

Update CallClient.cs

30c901d

Update ConversationClient.cs

8b14d8c

minnieliu reviewed Jun 7, 2021

View reviewed changes

zihzhan-msft and others added 4 commits June 7, 2021 15:47

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-net i…

6c94329

…nto feature/communication-ServerCalling-preview1

Merge branch 'feature/communication-ServerCalling-preview1' of https:…

d2634cf

…//github.com/Azure/azure-sdk-for-net into feature/communication-ServerCalling-preview1

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-net i…

1ffd619

…nto feature/communication-ServerCalling-preview1

Feature/zihzhan livetesting (#21677)

78d1f6e

* Enable living testing. * Test Playback. * Add playaudio test record. * Add createcall test record. * Add delete call record. * Add hangup call test record. * Add cancelAllMediaOperations call test record. * Enable living tests. * Address comments.

RezaJooyandeh reviewed Jun 8, 2021

View reviewed changes

RezaJooyandeh approved these changes Jun 8, 2021

View reviewed changes

minnieliu approved these changes Jun 8, 2021

View reviewed changes

minnieliu merged commit b227152 into master Jun 8, 2021

minnieliu deleted the feature/communication-ServerCalling-preview1 branch June 8, 2021 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/communication server calling preview1 #21017

Feature/communication server calling preview1 #21017

zihzhan-msft commented May 12, 2021 •

edited

Loading

check-enforcer bot commented May 12, 2021

RezaJooyandeh left a comment

minnieliu May 19, 2021

minnieliu May 19, 2021

zihzhan-msft May 28, 2021

navali-msft May 28, 2021

RezaJooyandeh May 28, 2021

navali-msft May 28, 2021

navali-msft Jun 2, 2021

minnieliu Jun 3, 2021

zihzhan-msft Jun 4, 2021

minnieliu Jun 3, 2021

zihzhan-msft Jun 3, 2021

minnieliu Jun 7, 2021

zihzhan-msft Jun 7, 2021

minnieliu Jun 7, 2021

zihzhan-msft Jun 7, 2021

minnieliu Jun 7, 2021

zihzhan-msft Jun 7, 2021

minnieliu Jun 7, 2021

zihzhan-msft Jun 7, 2021

minnieliu Jun 7, 2021 •

edited

Loading

zihzhan-msft Jun 7, 2021

RezaJooyandeh Jun 8, 2021

RezaJooyandeh left a comment

Feature/communication server calling preview1 #21017

Feature/communication server calling preview1 #21017

Conversation

zihzhan-msft commented May 12, 2021 • edited Loading

All SDK Contribution checklist:

Additional management plane SDK specific contribution checklist:

Management plane SDK Troubleshooting

Old outstanding PR cleanup

check-enforcer bot commented May 12, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

For data-plane/track 2 SDKs Issue the following command as a pull request comment:

For track 1 management-plane SDKs

RezaJooyandeh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minnieliu Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RezaJooyandeh left a comment

Choose a reason for hiding this comment

zihzhan-msft commented May 12, 2021 •

edited

Loading

minnieliu Jun 7, 2021 •

edited

Loading