Skip to content

[Core] Add @runtime_checkable to TokenCredential protocol definitions#25187

Merged
mccoyp merged 1 commit intoAzure:mainfrom
mccoyp:tokencred-typing
Aug 20, 2022
Merged

[Core] Add @runtime_checkable to TokenCredential protocol definitions#25187
mccoyp merged 1 commit intoAzure:mainfrom
mccoyp:tokencred-typing

Conversation

@mccoyp
Copy link
Copy Markdown
Member

@mccoyp mccoyp commented Jul 12, 2022 •
edited
Loading

Description

See #25175. With this PR, doing an isinstance check with an azure-identity credential and a TokenCredential protocol returns True -- that check errors today since these protocols aren't runtime checkable.

isinstance and issubclass checks aren't available without @runtime_checkable, but explicitly declaring the TokenCredential implementation (as shown below) could possibly satisfy static type checkers.

# from https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/identity/azure-identity/azure/identity/_internal/get_token_mixin.py
...
from azure.core.credentials import TokenCredential
...
class GetTokenMixin(ABC, TokenCredential):
# from https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/identity/azure-identity/azure/identity/aio/_internal/get_token_mixin.py
...
from azure.core.credentials_async import AsyncTokenCredential
...
class GetTokenMixin(abc.ABC, AsyncTokenCredential):

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@mccoyp mccoyp added Client This issue points to a problem in the data-plane of the library. Azure.Core Azure.Identity labels Jul 12, 2022
xiangyan99
xiangyan99 requested changes Jul 12, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@xiangyan99 xiangyan99 left a comment

Choose a reason for hiding this comment

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

Let's split them into two separate PRs: core-only and non-core.

@azure-sdk
Copy link
Copy Markdown
Collaborator

azure-sdk commented Jul 12, 2022 •
edited
Loading

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-core

@mccoyp mccoyp force-pushed the tokencred-typing branch from 5ffd6d4 to e93d46d Compare July 12, 2022 21:22
@mccoyp mccoyp requested a review from xiangyan99 July 12, 2022 21:22
xiangyan99
xiangyan99 approved these changes Jul 12, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@xiangyan99 xiangyan99 left a comment

Choose a reason for hiding this comment

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

Do you want to add it into changelog?

@mccoyp mccoyp mentioned this pull request Jul 12, 2022
Closed
@mccoyp
Copy link
Copy Markdown
Member Author

mccoyp commented Jul 12, 2022

@xiangyan99 good point, it probably would be worth adding. I am just now noticing, though, that this isn't supported in Python 3.7. This'll need some reworking, unfortunately 🤔

lmazuel
lmazuel reviewed Jul 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@lmazuel lmazuel left a comment

Choose a reason for hiding this comment

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

Change my approve to comment, as we need 3.6 support. Maybe Krista has an idea.

@xiangyan99
Copy link
Copy Markdown
Member

xiangyan99 commented Jul 21, 2022

Change my approve to comment, as we need 3.6 support. Maybe Krista has an idea.

I guess what you wanted to say is we wanted 3.7 support?

@lmazuel
Copy link
Copy Markdown
Member

lmazuel commented Jul 21, 2022

@mccoyp we're dropping

Change my approve to comment, as we need 3.6 support. Maybe Krista has an idea.

I guess what you wanted to say is we wanted 3.7 support?

yeah :p

@mccoyp
Copy link
Copy Markdown
Member Author

mccoyp commented Jul 21, 2022

Yeah, this won't work because 3.7 doesn't support it 😔 And it doesn't solve the motivating issue (see this comment), so we'll need a different approach completely.

I don't think this change (rather, a 3.7-compatible version) is a bad idea, because being able to do an isinstance check of a protocol implementation sounds desirable. But I can also close this PR for now -- @lmazuel what do you think?

@lmazuel
Copy link
Copy Markdown
Member

lmazuel commented Jul 21, 2022

@mccoyp , ok let's try to reproduce already, it should be possible. Once we understand that, we'll talk about solution.

@mccoyp
Copy link
Copy Markdown
Member Author

mccoyp commented Aug 20, 2022

Identity CI failure is unrelated, and a fix is proposed here: #25787

Overriding check enforcer to merge since everything else is a-okay 🙂

@mccoyp
Copy link
Copy Markdown
Member Author

mccoyp commented Aug 20, 2022

/check-enforcer override

@mccoyp mccoyp merged commit 388efaa into Azure:main Aug 20, 2022
@mccoyp mccoyp deleted the tokencred-typing branch August 20, 2022 00:25
@mccoyp mccoyp mentioned this pull request Aug 20, 2022
Closed
6 tasks
mccoyp added a commit to mccoyp/azure-sdk-for-python that referenced this pull request Sep 22, 2022
@mccoyp
[Core] Add runtime_checkable to protocol definitions (Azure#25187)
3354496
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lmazuel lmazuel lmazuel approved these changes

@xiangyan99 xiangyan99 xiangyan99 approved these changes

@schaabs schaabs Awaiting requested review from schaabs

@annatisch annatisch Awaiting requested review from annatisch annatisch is a code owner

@johanste johanste Awaiting requested review from johanste johanste is a code owner

@tjprescott tjprescott Awaiting requested review from tjprescott

@iscai-msft iscai-msft Awaiting requested review from iscai-msft iscai-msft is a code owner

@kashifkhan kashifkhan Awaiting requested review from kashifkhan kashifkhan is a code owner

Assignees

No one assigned

Labels

Azure.Core Azure.Identity Client This issue points to a problem in the data-plane of the library.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mccoyp @azure-sdk @xiangyan99 @lmazuel