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.

Sign up for GitHub

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

Jump to bottom

Swapping Microsoft.Azure.Services.AppAuthentication nuget dependency for Azure.Identity #1399

Merged
merged 3 commits into from
Jul 7, 2020
Merged

Swapping Microsoft.Azure.Services.AppAuthentication nuget dependency for Azure.Identity #1399

merged 3 commits into from
Jul 7, 2020

Conversation

bachuv
Copy link
Collaborator

@bachuv bachuv commented Jun 25, 2020 •
edited
Loading

Resolves #1232 by using DefaultAzureCredential from Azure.Identity instead of AzureServiceTokenProvider from Microsoft.Azure.Services.AppAuthentication.

@bachuv bachuv requested review from cgillum and ConnorMcMahon June 25, 2020 23:25
@cgillum
Copy link
Member

cgillum commented Jun 25, 2020

The code change looks good to me. I'll sign off once you are able to verify:

  1. It works when running locally (using the current user's Azure identity)
  2. It works in Azure when using managed identities.

ConnorMcMahon
ConnorMcMahon approved these changes Jun 26, 2020
View reviewed changes
Copy link
Contributor

@ConnorMcMahon ConnorMcMahon left a comment

Choose a reason for hiding this comment

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

I second Chris's sentiment.

ConnorMcMahon
ConnorMcMahon suggested changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@ConnorMcMahon ConnorMcMahon left a comment

Choose a reason for hiding this comment

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

It looks like Functions V1 does not like these changes, which is odd because the new library seems to be Net Standard compatible...

cgillum
cgillum approved these changes Jul 7, 2020
View reviewed changes
Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

This all looks good me to me now. :)

@cgillum
Copy link
Member

cgillum commented Jul 7, 2020

@ConnorMcMahon do you have any more feedback on this PR?

ConnorMcMahon
ConnorMcMahon approved these changes Jul 7, 2020
View reviewed changes
Copy link
Contributor

@ConnorMcMahon ConnorMcMahon left a comment

Choose a reason for hiding this comment

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

LGTM!

@bachuv bachuv added this to the v2.3.0 milestone Jul 7, 2020
@bachuv bachuv merged commit 4d22fee into dev Jul 7, 2020
@bachuv bachuv deleted the vabachu/identitynuget branch July 7, 2020 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ConnorMcMahon ConnorMcMahon ConnorMcMahon approved these changes

@cgillum cgillum cgillum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Extension v2.3.0
Development

Successfully merging this pull request may close these issues.

Investigate swapping Microsoft.Azure.Services.AppAuthentication nuget dependency for Azure.Identity
3 participants
@bachuv @cgillum @ConnorMcMahon