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

[Identity] Comment improvements. Identity generally doesn't return null anymore #15783

Merged
5 commits merged into from
Jun 22, 2021

Conversation

sadasant
Copy link
Contributor

I’ve got some customers writing to me directly with some confusion regarding getToken returning null. We’ve got some remnants of this type, but at this point, Identity doesn’t return null (except for ClientSecretCredential in the browser, which is generally unusable outside of our tests). This PR is to update these docs and to give a bit of clarity on what errors we’re throwing.

@sadasant sadasant self-assigned this Jun 16, 2021
@sadasant sadasant requested a review from schaabs as a code owner June 16, 2021 22:01
@ghost ghost added the Azure.Identity label Jun 16, 2021
Copy link
Member

@witemple-msft witemple-msft left a comment

Choose a reason for hiding this comment

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

Couple of nitpicks and a typo where a sentence didn't get completely removed.

* client secret details from environment variables. If the expected
* environment variables are not found at this time, the getToken method
* will return null when invoked.
* Creates an instance of the EnvironmentCredential class and reads client secret details from environment variables.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: certificates as well as username/password can also be used, maybe they should be mentioned as well (currently it just says "client secret details")?

sadasant and others added 2 commits June 17, 2021 20:36
Co-authored-by: Will Temple <witemple@microsoft.com>
Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>
@sadasant sadasant requested a review from scottaddie June 18, 2021 00:54
Copy link
Member

@witemple-msft witemple-msft left a comment

Choose a reason for hiding this comment

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

LGTM.

@check-enforcer

This comment has been minimized.

@sadasant sadasant force-pushed the identity/improve-comments branch from 909e2f8 to e1dde79 Compare June 21, 2021 19:59
@ghost
Copy link

ghost commented Jun 21, 2021

Hello @sadasant!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@sadasant
Copy link
Contributor Author

I had an issue with the git history of this PR, but it should be good now.

Copy link
Member

@scottaddie scottaddie left a comment

Choose a reason for hiding this comment

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

:shipit:

@ghost ghost merged commit 53d6089 into Azure:main Jun 22, 2021
@sadasant sadasant deleted the identity/improve-comments branch June 22, 2021 11:09
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Aug 26, 2021
updated readme.python.md for Service Bus and Event Hub for 2021-06-01-preview (Azure#15783)

Co-authored-by: v-ajnava <v-ajnava@microsoft.com>
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Aug 26, 2021
updated readme.python.md for Service Bus and Event Hub for 2021-06-01-preview (Azure#15783)

Co-authored-by: v-ajnava <v-ajnava@microsoft.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants