Skip to content

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

Merged
laiapat merged 1 commit into
Azure:mainfrom
laiapat:tokencred-typing
Aug 20, 2022
Merged

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

Conversation

@laiapat

@laiapat laiapat commented Jul 12, 2022

Copy link
Copy Markdown
Member

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.

@laiapat laiapat 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 left a comment

Copy link
Copy Markdown
Member

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

azure-sdk commented Jul 12, 2022

Copy link
Copy Markdown
Collaborator

API change check

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

azure-core

@laiapat
laiapat force-pushed the tokencred-typing branch from 5ffd6d4 to e93d46d Compare July 12, 2022 21:22
@laiapat
laiapat requested a review from xiangyan99 July 12, 2022 21:22

@xiangyan99 xiangyan99 left a comment

Copy link
Copy Markdown
Member

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?

@laiapat

laiapat commented Jul 12, 2022

Copy link
Copy Markdown
Member Author

@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 left a comment

Copy link
Copy Markdown
Member

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

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

lmazuel commented Jul 21, 2022

Copy link
Copy Markdown
Member

@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

@laiapat

laiapat commented Jul 21, 2022

Copy link
Copy Markdown
Member Author

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

lmazuel commented Jul 21, 2022

Copy link
Copy Markdown
Member

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

@laiapat

laiapat commented Aug 20, 2022

Copy link
Copy Markdown
Member Author

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

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

@laiapat

laiapat commented Aug 20, 2022

Copy link
Copy Markdown
Member Author

/check-enforcer override

@laiapat
laiapat merged commit 388efaa into Azure:main Aug 20, 2022
@laiapat
laiapat deleted the tokencred-typing branch August 20, 2022 00:25
laiapat added a commit to laiapat/azure-sdk-for-python that referenced this pull request Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants