[Identity] Update get_token method signatures - #34953
[Identity] Update get_token method signatures#34953Paul Van Eck (pvaneck) wants to merge 1 commit into
Conversation
aa58aa3 to
82a4ded
Compare
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
Several `get_token` method signatures were updated to include the `enable_cae` keyword argument to match the TokenCredential protocol method signature. Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
82a4ded to
cafdf20
Compare
| https://learn.microsoft.com/entra/identity-platform/scopes-oidc. | ||
| :keyword str claims: not used by this credential; any value provided will be ignored. | ||
| :keyword str tenant_id: optional tenant to include in the token request. | ||
| :keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested |
There was a problem hiding this comment.
Do we really use it?
It seems not make sense to explicitly list the argument then explicitly say not supported.
There was a problem hiding this comment.
Based on discussion yesterday, I thought the agreement was to have our get_token implementations use matching signatures as the TokenCredential Protocol get_token.
The current signature mismatch is what is causing the PyCharm IDE to complain:
I agree it is weird to list it and not use it, but that's pretty much the nature of trying to match the signature for credentials with differing sets of capabilities. In this same credential, we do the same thing with claims.
We can sit on this PR for now as we solicit more feedback on if we should cater to the PyCharm type-checker. It definitely doesn't need to go into next week's release. MyPy is fine resolving our Credentials as TokenCredentials even without the exact signature match, so there is still a bit of uncertainty here on if we should make this change.
There was a problem hiding this comment.
My concern is if we explicitly say one argument is not supported.
Does that mean for the arguments that we don't explicitly say not supported means they are supported?
|
Hi Paul Van Eck (@pvaneck). Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
1 similar comment
|
Hi Paul Van Eck (@pvaneck). Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
github-actions
Bot
added
the
no-recent-activity
|
Hi Paul Van Eck (@pvaneck). Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing |
3 participants
Several
get_tokenmethod signatures were updated to explicitly include theenable_caekeyword argument to match the TokenCredential protocol method signature.This is similar to the changes made previously in #31047.