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

splits authentication and access token interfaces for go, java, dotnet and docs #1050

Conversation

baywet
Copy link
Member

@baywet baywet commented Jan 21, 2022

completion of #1039

CC @Ndiritu @SilasKenneth when you move on to implementing azure authentication provider for PHP, and to update your abstractions as well.

@baywet baywet requested a review from ddyett January 24, 2022 15:10
@jobala
Copy link
Contributor

jobala commented Jan 24, 2022

I don't have historical context as to why we have AzureIdentity in struct and function names but we would be fine if we omit it because we only use azure identity so there's no need to differentiate structs and function names by an identity provider.

@baywet
Copy link
Member Author

baywet commented Jan 24, 2022

@jobala to answer your question it's just a branding/convenience naming.
On the technical aspect, TokenCredential, which is what our providers depend on at build time, is defined in azure-core. So the technical name of the structs should be TokenCredentialAuthenticationProvider.
However, with just TokenCredential, your app won't do much, you need one of its implementations which are all located in azure-identity, hence the name it to enable an easier mental association of the different components.
I hope this clarifies things.

@jobala
Copy link
Contributor

jobala commented Jan 24, 2022

it's just a branding/convenience naming.

In this case we should consider omitting AzureIdentity because it increases the length of names which takes a toll on readability. Long function/struct names are harder to read. But of course my suggestion trade offs mental association for shorter variable names and assumes doing so won't result in breaking changes.

Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

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

I believe we still need to bump up the version numbers for the abstractions/authentication packages to avoid any issues during release

@baywet
Copy link
Member Author

baywet commented Jan 25, 2022

We do, I'm planning to do that in #1039 once all other PRs are merged into it

@sonarcloud
Copy link

sonarcloud bot commented Jan 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@jobala jobala left a comment

Choose a reason for hiding this comment

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

Looks good, my only reservation is with having AzureIdentity in function, method and struct names in the Go components of this PR.

@baywet
Copy link
Member Author

baywet commented Jan 25, 2022

@jobala thanks for taking the time to review it. And that reservation is because the name is getting long?

@baywet baywet merged commit 2146425 into enhancement/nikithauc-authentication-encapsulation Jan 25, 2022
@baywet baywet deleted the feature/go-and-java-auth-int-split branch January 25, 2022 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Csharp Pull requests that update .net code documentation Improvements or additions to documentation Go Java
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants