Skip to content

Conversation

@MilenaHristova
Copy link
Contributor

@MilenaHristova MilenaHristova commented Apr 27, 2023

dotnet/arcade-services#2631

Migrating from Microsoft.IdentityModel.Clients.ActiveDirectory to Microsoft.Identity.Client for authenticating with Azure Active Directory and creating aka.ms links, because Microsoft.IdentityModel.Clients.ActiveDirectory is deprecated.
Adding a feature switch so that the change can get to the Stage-DotNet-Test pipeline without breaking arcade and we can make sure publishing works with the new library.
Once it's validated by running the staging pipeline we'll remove the switch and leave the code that uses the new library. #13318

To double check:

Copy link
Member

@premun premun left a comment

Choose a reason for hiding this comment

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

@MilenaHristova if this is a new feature that should eventually replace the current way and this is a feature switch you should ideally:

  • Desribe this in the PR
  • Create an issue that says something like "switch to the new library in full once we know it works"
  • Add TODOs in the code to say it's temporary, link the issue and say it will be removed as it's a feature switch only

@MilenaHristova MilenaHristova requested a review from premun April 28, 2023 07:46
@MilenaHristova MilenaHristova merged commit 4d5f225 into dotnet:main Apr 28, 2023
// using the new Microsoft.Identity.Client library
private async Task<HttpClient> CreateClientUsingMSAL()
{
//_log.LogMessage(MessageImportance.High, "Creating a client using MSAL.NET");
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I guess this was forgotten behind. You could log this on a lower level and you'd still find it in the binlog just not the regular log

</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.Identity.Client" Version="4.53.0" />
Copy link
Member

Choose a reason for hiding this comment

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

We define all package versions in Versions.props. Please move it there into the "externals" section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it in the next pr #13476

@ViktorHofer
Copy link
Member

Great change. Note that symboluploader in the dotnet-symboluploader also uses Microsoft.IdentityModel.Clients.ActiveDirectory and should be updated as well.

@andriipatsula
Copy link
Member

@MilenaHristova do we need to make changes in the dotnet-release repository?

@tkapin
Copy link
Member

tkapin commented May 11, 2023

Great change. Note that symboluploader in the dotnet-symboluploader also uses Microsoft.IdentityModel.Clients.ActiveDirectory and should be updated as well.

@hoyosjs - you seem to be the last one contributing to dotnet-symuploader repo, do you know who's would be best to take care of this?

@hoyosjs
Copy link
Member

hoyosjs commented May 11, 2023

The diagnostics teams helps maintain it - the AAD option hasn't been deeply supported lately. All our usage is around is PATs. We do know the migration is necessary - and at the same time we are trying to deprecate the tool in favor of symbol. cc: @dotnet/dotnet-diag

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