Skip to content

[typing] Add DefaultAzureCredential to allowed types for credentials in KeyVaultClient - #25144

Closed
Søren Skov Eriksen (soer7022) wants to merge 1 commit into
Azure:mainfrom
soer7022:Typing-fix-for-azure-keyvault-secret
Closed

[typing] Add DefaultAzureCredential to allowed types for credentials in KeyVaultClient#25144
Søren Skov Eriksen (soer7022) wants to merge 1 commit into
Azure:mainfrom
soer7022:Typing-fix-for-azure-keyvault-secret

Conversation

@soer7022

@soer7022 Søren Skov Eriksen (soer7022) commented Jul 8, 2022

Copy link
Copy Markdown

Description

The current typing for KeyVaultClient does not match the steps described in current documentation: here

I have added the DefaultAzureCredential from azure.identity based on the current recommended steps from the documentation.

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.

@ghost ghost added KeyVault customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Jul 8, 2022
@ghost

Copy link
Copy Markdown

Thank you for your contribution soer7022! We will review the pull request and get back to you soon.

@soer7022 Søren Skov Eriksen (soer7022) changed the title Add DefaultAzureCredential to allowed types for credentials in KeyVaultClient [typing] Add DefaultAzureCredential to allowed types for credentials in KeyVaultClient Jul 8, 2022
@laiapat

Copy link
Copy Markdown
Member

Hi Søren Skov Eriksen (@soer7022), thank you for opening a PR and contributing to the SDK!

In this instance though, I believe the current typing is correct. Key Vault clients (and other clients in the SDK) don't necessarily require azure-identity credentials. They accept any credential that supports the TokenCredential protocol, which does include the DefaultAzureCredential, but also includes other azure-identity credentials and even custom credentials (some examples). So, TokenCredential is used as the credential type here and elsewhere 🙂

Do you think the documentation should be more clear about that? And/or is there a type checker that raised an error when providing a DefaultAzureCredential to a Key Vault client? If so, that might be something we'd want to find a fix for.

@soer7022

Søren Skov Eriksen (soer7022) commented Jul 11, 2022

Copy link
Copy Markdown
Author

Hi @mccoyp, thank you for the detailed reply 😄

The main reason that I made this pull request is due to PyCharm flagging it as a incorrect type:

Screenshot 2022-07-11 at 07 35 14

Is my fix the right fix? Or should we look into making something more general?

@laiapat

Copy link
Copy Markdown
Member

Søren Skov Eriksen (@soer7022) Thank you for providing context about the issue! That's the first time I've seen an IDE raise a concern with the typing, and suggests that there's something we can do to make the protocol implementation more clear to Python (and users).

I can't reproduce the PyCharm warning on my end unfortunately -- I'm using a Python 3.10.0 interpreter on PyCharm Community Edition 2021.1.3. Could I know what interpreter and PyCharm version you're using?

If you don't mind, I'd like to continue this discussion on #25175, an issue I opened to track this. It would be great if you could continue to help us to fix this! If not, that's no problem -- I appreciate that you brought this to our attention 🙂

I'm going to close this PR though, since the solution should come from the credential definition and/or the TokenCredential protocol definition.

@soer7022

Copy link
Copy Markdown
Author

Thank you! I will add further details and comments on the issue

@soer7022
Søren Skov Eriksen (soer7022) deleted the Typing-fix-for-azure-keyvault-secret branch July 12, 2022 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

customer-reported Issues that are reported by GitHub users external to the Azure organization. KeyVault

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants