-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Communication identity api redesign #16420
Communication identity api redesign #16420
Conversation
…esign fix async impl + tests
…sign-clone-personal Updated sync identity client and tests
…sign-clone-personal Use Azure.Core.AccessToken instead of CommunicationUserToken
…ity-redesign-personal Updated formatting for method documentation
…ity-redesign-personal Fix trailing whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good. Just a few comments/questions.
@@ -73,5 +112,4 @@ This project has adopted the [Microsoft Open Source Code of Conduct](https://ope | |||
For more information see the [Code of Conduct FAQ](https://opensource.microsoft.com/codeofconduct/faq/) or contact [opencode@microsoft.com](mailto:opencode@microsoft.com) with any additional questions or comments. | |||
|
|||
<!-- LINKS --> | |||
[identitysamples]: https://github.com/Azure/azure-sdk-for-python/blob/master/sdk/communication/azure-communication-identity/samples/identity_samples.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we remove this sample?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link was to just the sync sample. We instead have a place where we link to both the sync/async samples (under "next steps"). The place this link was being used was also just a link, and I've replaced it with fully fleshed out samples instead.
def _replace_keys(self, body): | ||
def _replace_recursively(dictionary): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we need to change this to recursively? instead of a for loop before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous for loop logic did not work, it actually would not perform nested replacements.
Recursion felt more elegant because it traverses nested json. We also similarly use a recursive function in the responsereplacer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do this recursively to sanitize fields that are nested in the JSON response.
Example:
{"identity": {"id": "sanitized"}, "accessToken": {"token": "sanitized",
"expiresOn": "2021-01-30T22:50:20.3668737+00:00"}}
The id and token fields are nested, but need to be sanitized.
|
||
async def main(): | ||
sample = CommunicationIdentityClientSamples() | ||
await sample.create_user() | ||
await sample.delete_user() | ||
await sample.create_user_with_token() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we create 2 users now? one with create_user and the other with create_user_with_token? But I only see 1 delete_user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The methods are actually independent. e.g. delete_user never deleted the user created by create_user, it creates it's own user to delete
print("User created with id:" + user.identifier) | ||
``` | ||
|
||
Alternatively, use the `create_user_with_token` method to create a new user and issue a token for it.\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Remove \ after the .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, i'm doing this to force a newline here. I can still remove it if you think we shouldn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good.
…into enum-meta * 'master' of https://github.com/Azure/azure-sdk-for-python: (128 commits) Communication identity api redesign (Azure#16420) fix EH samples and docs (Azure#16457) T2 redisenterprise 2021 02 02 (Azure#16472) Update automation_generate.sh (Azure#16470) Sync eng/common directory with azure-sdk-tools for PR 1353 (Azure#16465) Normalize the package name for Doc.Ms readme (Azure#16401) fix changelog and version (Azure#16445) we should always run integration to publish from our artifacts. if the build pipeline crashed too early in the pipeline, this will fail regardless, due to inability to pull the artifacts down (Azure#15058) Fix DateTime bug (Azure#16456) Resolve Regression Failures (Azure#16455) [text analytics] Expose 'string_index_type' parameter in all service client methods where applicable (Azure#16412) adding devtools to the appconfig dev_reqs to solve python core issue (Azure#16381) Copy job matrix functionality (Azure#16450) Add APIView KV variable group to prepare pipelines bot (Azure#16451) [Datalake] Added support for PurePosixPath (Azure#16400) Regenerate baseline because last one break. (Azure#16415) adding step to test for crlf line endings (Azure#16398) [Datalake] Removed list_paths manual paging and deserialization (Azure#16309) Sync eng/common directory with azure-sdk-tools for PR 1351 (Azure#16448) Update auto_codegen.py (Azure#16443) ...
This reverts commit 30b917b.
API View: https://apiview.dev/Assemblies/Review/cf5b3c767f8340cab539691975abf54e