[Communication] - Phone Number Management - Added support for AAD auth #16075

jbeauregardb · 2021-01-11T18:25:52Z

No description provided.

check-enforcer · 2021-01-26T19:04:32Z

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, you can 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 python - [service] - ci

sacheu · 2021-01-27T19:13:01Z

sdk/communication/azure-communication-administration/README.md

+# AZURE_TENANT_ID, AZURE_CLIENT_ID and AZURE_CLIENT_SECRET as env variables.
+phone_number_administration_client = PhoneNumberAdministrationClient(endpoint, DefaultAzureCredential())
+
+# You can also authenticate using your connection string
 phone_number_administration_client = PhoneNumberAdministrationClient.from_connection_string(connection_str)


Do we need to remove this line (line 37), when there is an addition of line 34?

We're keeping both forms of authentication, so line 37 is still relevant

True, but it is quite weird to look at it in the code snippet so maybe keep only one in the first snippet.. then write a regular text saying that you could also authenticate using the connection string and then have a separate snippet containing just the separate constructor.. that way it would easier to look at and understand better.

sacheu · 2021-01-27T19:17:02Z

...nication/azure-communication-administration/tests/test_phone_number_administration_client.py


 SKIP_PHONE_NUMBER_TESTS = True
 PHONE_NUMBER_TEST_SKIP_REASON= "Phone Number Administration live tests infra not ready yet"

-class PhoneNumberAdministrationClientTest(PhoneNumberCommunicationTestCase):
+class FakeTokenCredential(object):


I see this test and the async version of the test both use this class FakeTokenCredential, can we move this class into common code

...nication/azure-communication-administration/tests/test_phone_number_administration_client.py

sacheu · 2021-01-28T18:52:53Z

PR looks good.

sacheu

PR Looks Good.

sacheu

Looks good. Left a few suggestions.

sacheu · 2021-02-02T17:10:50Z

...on/azure-communication-administration/tests/test_phone_number_administration_client_async.py

+            items = []
+            async for item in pages:
+                items.append(item)
+        assert len(items) > 0


should we add assert to check for what we get in item? instead of just making sure the len(items) is not empty?

Same as other comment.

sacheu · 2021-02-02T17:11:42Z

...nication/azure-communication-administration/tests/test_phone_number_administration_client.py

+        credential = create_token_credential()
+        phone_number_client = PhoneNumberAdministrationClient(endpoint, credential)
+        pages = phone_number_client.list_all_phone_numbers()
+        assert pages.next()


should we check if the # of phone numbers we get match with what we expected?

We don't know which (or how many) phone numbers we're going to receive exactly, that's why this test only checks that we indeed returned something. This test is also copied from the current list_all_phone_numbers test from this same file, just with the added managed identity authentication.

ankitarorabit · 2021-02-02T17:14:10Z

...stration/azure/communication/administration/aio/_phone_number_administration_client_async.py

@@ -70,7 +69,7 @@ def __init__(
        self._endpoint = endpoint
        self._phone_number_administration_client = PhoneNumberAdministrationClientGen(
            self._endpoint,
-            authentication_policy=HMACCredentialsPolicy(endpoint, credential),


wondering if we really need the isAsync flag here?

yes, because the get_authentication_policy function receives that optional parameter and passes it to the HMACCredentialPolicy object. Like this:

return HMACCredentialsPolicy(endpoint, credential, decode_url=is_async)

…into enum-meta * 'master' of https://github.com/Azure/azure-sdk-for-python: bump six dependencies in some libraries (#16496) call on_error if timeout in flush (#16485) Sync eng/common directory with azure-sdk-tools for PR 1365 (#16505) Fix min dependency tests - update azure core (#16504) Sync eng/common directory with azure-sdk-tools for PR 1364 (#16503) Ma arch feedback (#16502) Adding a new limitation to the README file. (#16475) [Blob][Datalake] STG76 Preview (#16349) append code coverage over each other (#16202) Arch preview feedback (#16441) Support CAE in azure-identity (#16323) [EventHubs] Support for Custom endpoint adddress and custom certificate (#16295) [Communication] - Phone Number Management - Added support for AAD auth (#16075) fix live tests (#16495)

… AAD auth (#16075)" This reverts commit 1c69e0c.

…into analyze_redesign * 'master' of https://github.com/Azure/azure-sdk-for-python: (32 commits) Adopt new MSAL auth code flow API (Azure#16449) [formrecognizer] use ARM template for tests (Azure#16432) T2 kusto 2021 02 04 (Azure#16527) T2 applicationinsights 2021 02 04 (Azure#16525) Sync eng/common directory with azure-sdk-tools for PR 1366 (Azure#16506) [Python] python track2 new pipeline fix (Azure#16494) Added package properties SDKType and NewSDK (Azure#16476) bump six dependencies in some libraries (Azure#16496) call on_error if timeout in flush (Azure#16485) Sync eng/common directory with azure-sdk-tools for PR 1365 (Azure#16505) Fix min dependency tests - update azure core (Azure#16504) Sync eng/common directory with azure-sdk-tools for PR 1364 (Azure#16503) Ma arch feedback (Azure#16502) Adding a new limitation to the README file. (Azure#16475) [Blob][Datalake] STG76 Preview (Azure#16349) append code coverage over each other (Azure#16202) Arch preview feedback (Azure#16441) Support CAE in azure-identity (Azure#16323) [EventHubs] Support for Custom endpoint adddress and custom certificate (Azure#16295) [Communication] - Phone Number Management - Added support for AAD auth (Azure#16075) ...

jbeauregardb requested a review from turalf January 11, 2021 18:25

ghost added the Communication label Jan 11, 2021

jbeauregardb force-pushed the pnm_managed_identity branch from 5ca5c4b to 8c1e1c2 Compare January 11, 2021 19:18

jbeauregardb marked this pull request as ready for review January 26, 2021 18:59

jbeauregardb requested review from ankitarorabit, memontic-ms and a team as code owners January 26, 2021 18:59

jbeauregardb force-pushed the pnm_managed_identity branch 3 times, most recently from 9d7792e to f7f9ca7 Compare January 27, 2021 19:07

sacheu reviewed Jan 27, 2021

View reviewed changes

jbeauregardb force-pushed the pnm_managed_identity branch 2 times, most recently from 269874a to b56a60e Compare January 28, 2021 18:11

sacheu reviewed Jan 28, 2021

View reviewed changes

...nication/azure-communication-administration/tests/test_phone_number_administration_client.py Show resolved Hide resolved

jbeauregardb force-pushed the pnm_managed_identity branch 5 times, most recently from d8e0aa7 to 376ba75 Compare January 28, 2021 18:50

sacheu self-requested a review January 28, 2021 18:54

sacheu approved these changes Jan 28, 2021

View reviewed changes

sacheu reviewed Feb 2, 2021

View reviewed changes

ankitarorabit reviewed Feb 2, 2021

View reviewed changes

ankitarorabit approved these changes Feb 2, 2021

View reviewed changes

jorge-beauregard added 3 commits February 2, 2021 11:20

Added support for AAD auth

d1785ce

added successful test recording

45ffafb

Addressed comments

b10b68c

jbeauregardb force-pushed the pnm_managed_identity branch from 376ba75 to 71b8326 Compare February 2, 2021 17:49

Edited readme

57f878c

jbeauregardb force-pushed the pnm_managed_identity branch from 71b8326 to 57f878c Compare February 2, 2021 22:34

jbeauregardb enabled auto-merge (squash) February 2, 2021 22:43

Fixed README

5913854

jbeauregardb disabled auto-merge February 3, 2021 17:25

jbeauregardb merged commit 1c69e0c into Azure:master Feb 3, 2021

beltr0n added a commit that referenced this pull request Feb 4, 2021

Revert "[Communication] - Phone Number Management - Added support for…

00e294c

… AAD auth (#16075)" This reverts commit 1c69e0c.

beltr0n mentioned this pull request Feb 4, 2021

Revert "[Communication] - Phone Number Management - Added support for AAD auth" #16516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Communication] - Phone Number Management - Added support for AAD auth #16075

[Communication] - Phone Number Management - Added support for AAD auth #16075

jbeauregardb commented Jan 11, 2021

check-enforcer bot commented Jan 26, 2021

sacheu Jan 27, 2021

jbeauregardb Jan 28, 2021

ankitarorabit Feb 2, 2021

sacheu Jan 27, 2021

sacheu commented Jan 28, 2021

sacheu left a comment

sacheu left a comment

sacheu Feb 2, 2021

jbeauregardb Feb 2, 2021

sacheu Feb 2, 2021

jbeauregardb Feb 2, 2021

ankitarorabit Feb 2, 2021

jbeauregardb Feb 2, 2021

[Communication] - Phone Number Management - Added support for AAD auth #16075

[Communication] - Phone Number Management - Added support for AAD auth #16075

Conversation

jbeauregardb commented Jan 11, 2021

check-enforcer bot commented Jan 26, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

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

sacheu commented Jan 28, 2021

sacheu left a comment

Choose a reason for hiding this comment

sacheu 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