8000 Update IdentityModel to 8.0.1 by BrennanConroy · Pull Request #352 · dotnet/source-build-externals · GitHub
[go: up one dir, main page]

Skip to content

Update IdentityModel to 8.0.1 #352

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

Merged
merged 2 commits into from
Aug 7, 2024
Merged

Conversation

BrennanConroy
Copy link
Member

No description provided.

@BrennanConroy BrennanConroy marked this pull request as ready for review August 6, 2024 22:19
Copy link
Member
@mthalman mthalman left a comment

Choose a reason for hiding this comment

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

What is the corresponding change on the ASP.NET Core side? I want to make sure everything is ready to go there before merging this since dependency flow of this repo's package will be broken until the corresponding aspnetcore changes are merged with it.

@BrennanConroy
Copy link
Member Author
BrennanConroy commented Aug 7, 2024

Don't we need to wait for source-build-externals dependency update in aspnetcore after this is merged?
See last IdentityModel update as an example dotnet/aspnetcore#56858

@mthalman
Copy link
Member
mthalman commented Aug 7, 2024

Don't we need to wait for source-build-externals dependency update in aspnetcore after this is merged? See last IdentityModel update as an example dotnet/aspnetcore#56858

Yes, in order to have a working solution. But I would at least like to see the PR prepped for this change. Everything should work in it except the source build leg. Then we can merge this and incorporate the dependency flow into that PR.

@wtgodbe
Copy link
Member
wtgodbe commented Aug 7, 2024

@mthalman is it doc'd somewhere that that's the preferred workflow? The source-build-externals update & the IdentityModel update need to happen in the same PR, so it's not clear to me why having a failing IdentityModel update PR waiting for a SBE update, is better than having a failing SBE PR waiting for an IdentityModel update. Especially since the SBE update will automatically get opened in its own PR, so if we already have a separate IdentityModel update PR waiting, we'd have to cherry-pick the commit from the SBE update PR & close it manually. That seems more cumbersome than just pushing an update to the auto-generated SBE update PR.

@mthalman
Copy link
Member
mthalman commented Aug 7, 2024

I just want to make sure things are efficient from a dependency flow standpoint. I'm happy to hash out a workflow that works for everyone involved. The reason I'd prefer the aspnetcore repo have a PR staged is to iron out any potential issues related to the upgrade (outside the context of source build). If there are issues, that might take time to resolve. Meanwhile, if this were already merged it would cause dependency flow failure in sdk because aspnetcore didn't update to align with this change.

@BrennanConroy
Copy link
Member Author
BrennanConroy commented Aug 7, 2024

I don't know why you chose this PR to start trying to enforce a new workflow. Every other update I can see updated the dependency in aspnetcore 8000 when the source-build-externals dep update came in.

If you are trying to create a workflow that works for everyone, I vote for one where I'm able to update our dependencies without having to be aware of source-builds existence.

@mthalman
Copy link
Member
mthalman commented Aug 7, 2024

I don't know why you chose this PR to start trying to enforce a new workflow. Every other update I can see updated the dependency in aspnetcore when the source-build-externals dep update came in.

There's been cases where the upgrade was done first in aspnetcore. Because source build failed there, that then prompted the update to be made in this repo.

Can I ask why there's pushback on my proposal? It's not like I'm suggesting extra work. The changes that would be done in the PR to upgrade the version have to be done anyway. It's just a matter of timing. I'm only suggesting this to ensure less frequency of breaks in dependency flow (which has happened in the past with this upgrade). Can we all agree that'd be a good thing?

@mthalman
Copy link
Member
mthalman commented Aug 7, 2024

I discussed this with the rest of my team so that we can all get on the same page with the recommended approach here.

I was mistaken with my thinking that merging this would cause a dependency flow failure in the sdk repo until it had also received the corresponding aspnetcore flow with the upgrade. That was a large part of my concern.

When making version upgrades in this repo, here is the recommendation:

  • Provide a link to an issue describing the need for the change. This would be in the consuming repo (e.g. aspnetcore). Link to that issue from both here and in the PR that upgrades the version in the consuming repo. This allows proper tracing so you can find all related changes.
  • Have a reasonable level of confidence that the upgrade can be done in a timely manner when consuming the dependency flow from this PR. If things take a long time or need to be backed out, it may require the reversion of the upgrade in this PR which we want to avoid.
  • When consuming the dependency flow from this repo for the purposes of a version upgrade, consider using a separate PR or at least changing the title of the dependency flow PR to accurately reflect the purpose of the change. Seeing a PR named "Upgrade IdentityModel to 8.0.1" provides better clarity than one named "Update dependencies from dotnet/source-build-externals".

We'll get these recommendations documented in the repo.

@mthalman mthalman merged commit 51b029e into dotnet:main Aug 7, 2024
2 checks passed
@BrennanConroy BrennanConroy deleted the brecon/identity branch August 7, 2024 21:48
@mthalman mthalman mentioned this pull request Aug 8, 2024
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.

3 participants
0