Conversation
|
Thank you for your contribution @niuzheng168! We will review the pull request and get back to you soon. |
There was a problem hiding this comment.
Pull request overview
This pull request attempts to enable Azure CLI credential authentication for the foundry agent sample, addressing an issue where the agent couldn't use key authentication. However, the implementation is incomplete and introduces API design inconsistencies.
Changes:
- Added imports for
AzureCliCredentialandDefaultAzureCredentialfrom azure.identity.aio - Changed the
credentialparameter type annotation to only acceptAzureCliCredentialorDefaultAzureCredential - Removed API key authentication logic and validation, hardcoding credential to
AzureCliCredential()
| self, | ||
| endpoint: str, | ||
| credential: Union[AzureKeyCredential, AsyncTokenCredential], | ||
| credential: Union[AzureCliCredential, DefaultAzureCredential], |
There was a problem hiding this comment.
The type annotation is too restrictive and inconsistent with the rest of the codebase. It should be Union[AzureKeyCredential, AsyncTokenCredential] to support both API key and token-based authentication methods. Other samples in this repository (async_function_calling_sample.py and basic_voice_assistant_async.py) use this more flexible type annotation, which allows users to choose their preferred authentication method.
| from azure.identity.aio import AzureCliCredential, DefaultAzureCredential | ||
|
|
There was a problem hiding this comment.
The imports AzureKeyCredential and AsyncTokenCredential (lines 50-51) are no longer used after removing API key authentication support. These unused imports should be removed to keep the code clean. However, if API key support is restored (as recommended), these imports should remain.
|
|
||
| credential = AzureCliCredential() |
There was a problem hiding this comment.
Hardcoding the credential to only AzureCliCredential removes flexibility for users. The sample should support multiple authentication methods like other samples in this repository. Consider either: (1) keeping the commented-out options to show users both API key and token credential choices, or (2) adding a command-line parameter like basic_voice_assistant_async.py does with --use-token-credential flag. This would make the sample more useful and consistent with other samples in the codebase.
| credential = AzureCliCredential() | |
| # Choose the authentication method you want to use: | |
| # | |
| # Option 1: Use DefaultAzureCredential (recommended for most applications) | |
| # This will use environment variables, managed identity, Visual Studio Code, Azure CLI, etc. | |
| # from azure.identity.aio import DefaultAzureCredential | |
| # credential = DefaultAzureCredential() | |
| # | |
| # Option 2: Use AzureCliCredential (for local development with an authenticated Azure CLI session) | |
| credential = AzureCliCredential() | |
| # | |
| # Option 3: Use API key authentication | |
| # Set the AZURE_VOICELIVE_API_KEY environment variable with your key. | |
| # from azure.core.credentials import AzureKeyCredential | |
| # credential = AzureKeyCredential(os.environ["AZURE_VOICELIVE_API_KEY"]) |
| self, | ||
| endpoint: str, | ||
| credential: Union[AzureKeyCredential, AsyncTokenCredential], | ||
| credential: Union[AzureCliCredential, DefaultAzureCredential], |
There was a problem hiding this comment.
this one is better from azure.core.credentials_async import AsyncTokenCredential
Description
foundry agent cannot use key auth
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines