Skip to content

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Mar 11, 2021

Description
RDBMS command module has its own credential logic which uses Azure Identity's ClientSecretCredential(#17191 (comment)).

Install Azure Identity so that CI can pass.

https://dev.azure.com/azure-sdk/public/_build/results?buildId=776163&view=logs&j=36dd4138-4d53-5e46-00d9-e5cd9744be05&t=1cf3879c-0a42-5ccd-7693-7a4781d739d8&l=134

src/azure-cli/azure/cli/command_modules/rdbms/_client_factory.py:140:12: E0611: No name 'identity' in module 'azure' (no-name-in-module)
src/azure-cli/azure/cli/command_modules/rdbms/_client_factory.py:140:12: E0401: Unable to import 'azure.identity' (import-error)

@jiasli jiasli mentioned this pull request Mar 11, 2021
3 tasks
azure-cli==2.20.0
azure-cli-core==2.20.0
azure-cli-telemetry==1.0.6
azure-cli==2.20.0
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorted all lines alphabetically.

@yonzhan yonzhan added this to the S184 milestone Mar 11, 2021
@yonzhan
Copy link
Collaborator

yonzhan commented Mar 11, 2021

Packaging

@jiasli jiasli requested review from houk-ms and jsntcy as code owners March 11, 2021 04:57
Copy link
Contributor

@DaeunYim DaeunYim left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking care of this..!

Copy link
Contributor

@Juliehzl Juliehzl left a comment

Choose a reason for hiding this comment

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

If they do want to use azure-identity, I could approve for the change. But I am considering if it could utilize existing msal package for the same thing and all service could find a consistent way, which is better to have.
Anyway, approved from my side.

@jiasli
Copy link
Member Author

jiasli commented Mar 11, 2021

... I am considering if it could utilize existing msal package for the same thing and all service could find a consistent way, which is better to have.

MSAL doesn't expose get_token API for Track 2 SDK's usage. In order to make Track 2 SDK working, one must implement get_token API by themeselves.

The consistent way is to use the one provided by core (#17191 (comment)) instead of developing a custom auth mechanism.

@jiasli jiasli merged commit 5a82f8f into Azure:dev Mar 11, 2021
@jiasli jiasli deleted the identity-dep branch March 11, 2021 08:10
@DaeunYim
Copy link
Contributor

... I am considering if it could utilize existing msal package for the same thing and all service could find a consistent way, which is better to have.

MSAL doesn't expose get_token API for Track 2 SDK's usage. In order to make Track 2 SDK working, one must implement get_token API by themeselves.

The consistent way is to use the one provided by core (#17191 (comment)) instead of developing a custom auth mechanism.

RDBMS module uses get_mgmt_service_client for default auth but I think the predecessor also implemented a way to override the authentification using credential. I wanted to keep both ways in case users were using this approach....! Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants