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

Should we add Core::Credentials::TokenCredential::GetName()? #4418

Closed
antkmsft opened this issue Mar 8, 2023 · 2 comments · Fixed by #4428
Closed

Should we add Core::Credentials::TokenCredential::GetName()? #4418

antkmsft opened this issue Mar 8, 2023 · 2 comments · Fixed by #4428
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. feature-request This issue requires a new behavior in the product in order be resolved.

Comments

@antkmsft
Copy link
Member

antkmsft commented Mar 8, 2023

Currently, that would be used for log messages that get emitted by ChainedTokenCredential, and would eliminate the need for internal constructor that is used by DefaultAzureCredential.

See #4409 (comment) and #4409 (comment)

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Mar 8, 2023
@antkmsft
Copy link
Member Author

antkmsft commented Mar 8, 2023

@LarryOsterman in #4427 (comment):

Taking a second look at the changes to ChainedTokenCredential and I'm concerned about the use of the sourcesFriendlyNames parameter.

That parameter is pretty fragile, it needs to exactly match the parameters in the Sources list.
Instead of adding this parameter, why not add a GetFriendlyName virtual method to the TokenCredential base class and have the different credential classes implement that method. To avoid a breaking change, add a default implementation for GetFriendlyName which returns something neutral ("unknown"?), but that way we don't need to keep the order in sync.

I also suspect that having the GetFriendlyName method may be useful in other situations.

Once that happens, it's not clear that we need a private constructor for the ChainedTokenCredential, which reduces the complexity of the design.

@antkmsft
Copy link
Member Author

antkmsft commented Mar 8, 2023

I like GetFriendlyName(), let's implement it. We could not add it in time this time around, because it would require us to ship Core.

Practically, the constructor with sourcesFriendlyNames should not be that fragile, because it is only used by DefaultAzureCredential, and we should review the code if we change it. But I still agree with you, it is better to have GetName() method.

It is not, however, clear, what to do with the enclosingCredentialName parameter, how to keep that constructor internal only for identity.

@RickWinter RickWinter added feature-request This issue requires a new behavior in the product in order be resolved. Azure.Core Client This issue points to a problem in the data-plane of the library. labels Mar 9, 2023
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Mar 9, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. feature-request This issue requires a new behavior in the product in order be resolved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants