Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

is_chained + black #31213

Merged
merged 7 commits into from
Jul 21, 2023
Merged

is_chained + black #31213

merged 7 commits into from
Jul 21, 2023

Conversation

xiangyan99
Copy link
Member

No description provided.

@azure-sdk
Copy link
Collaborator

API change check

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

azure-identity

@xiangyan99 xiangyan99 marked this pull request as ready for review July 20, 2023 00:12
@xiangyan99
Copy link
Member Author

Copy link
Member

@pvaneck pvaneck left a comment

Choose a reason for hiding this comment

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

Looks like the keyword argument is_chained is now a part of the public API for these dev-time credentials. Was this is supposed to be an internal only param?

@xiangyan99
Copy link
Member Author

Looks like the keyword argument is_chained is now a part of the public API for these dev-time credentials. Was this is supposed to be an internal only param?

Because those credentials did not accept kwargs, to accept extra parameter, there is no way to hide it from public API changes.

But they are not breaking changes.

@pvaneck
Copy link
Member

pvaneck commented Jul 20, 2023

Is within_credential_chain something we could use instead for this purpose?

Seems like we use it in quite a few places for determining the log-level depending on if the credential is within a chain or not (example).

@xiangyan99
Copy link
Member Author

Is within_credential_chain something we could use instead for this purpose?

Seems like we use it in quite a few places for determining the log-level depending on if the credential is within a chain or not (example).

When I synced with Bill, we only wanted to swallow the error in DAC. But not in all chained credentials.

@xiangyan99 xiangyan99 requested a review from pvaneck July 21, 2023 20:02
Copy link
Member

@pvaneck pvaneck left a comment

Choose a reason for hiding this comment

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

Just a comment about deduping error messages.

I'm generally okay with the added is_chained keyword argument. If we do get pushback for introducing this param to the API, then I think there are ways around it like setting credential._is_chained = True directly after instantiating it in DAC.

@xiangyan99 xiangyan99 merged commit 306cf01 into main Jul 21, 2023
@xiangyan99 xiangyan99 deleted the identity_add_is_chained branch July 21, 2023 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants