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] Clearer message on the getToken methods #11487

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

sadasant
Copy link
Contributor

Since some of our customers have started to work with our credentials directly, we've agreed that we should let them know that if they decide to work with the getToken methods themselves, they will need to figure out caching and token refreshing.

Fixes #11343

The specific message the team wants to add is this one:

// This method is called automatically by Azure SDK client libraries. You may call this method
// directly, but you must also handle token caching and token refreshing.

The main change here is an extra comment on both ChainedTokenCredential and TokenCredential to help users see this if they decide to use any of both directly. DefaultAzureCredential gets automatically handled by ChainedTokenCredential since the method is inherited.

Feedback always appreciated 🙌❤

Copy link
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

How often do you think this is used by mistake rather than by real need? If most of the time it is by mistake would it make sense to log a Warning as well?

Also can we point to a sample that shows a common scenario where users may be tempted to call this and show the better option?

@sadasant
Copy link
Contributor Author

@joheredi I'm not sure if pointing to a concrete scenario makes sense. The idea is not to publicize this use case, rather to avoid confusion in case users end up working with these internals themselves.

@jianghaolu what do you think of Jose's idea? (comment above)

@joheredi
Copy link
Member

joheredi commented Sep 29, 2020

@joheredi I'm not sure if pointing to a concrete scenario makes sense. The idea is not to publicize this use case, rather to avoid confusion in case users end up working with these internals themselves.

@jianghaolu what do you think of Jose's idea? (comment above)

I was thinking more on a sample discouraging the use of these internals and showing the recommended way.

Of course these suggestions wouldn't block this PR at all, just wanted to bring them up in case we find it useful to be more proactive in notifying the user.

@jianghaolu
Copy link

In .NET and Python SDK today we have this in the doc: "This method is called by Azure SDK clients. It isn't intended for use in application code." and we have customers complaining this has discouraged their valid use case - thus we decided to loosen it a little bit. With that being said, I agree most use cases should not involve calling this method directly, thus a sample could be helpful here.

@sadasant
Copy link
Contributor Author

Thank you all! I'm merging.

@sadasant sadasant merged commit cc8c8fc into Azure:master Sep 29, 2020
@sadasant sadasant deleted the identity/11343 branch September 29, 2020 19:38
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.

Improve Doc phrasing for DefaultAzureCredential.GetToken
3 participants