-
Notifications
You must be signed in to change notification settings - Fork 42
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
Conversation
There was a problem hiding this 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.
Don't we need to wait for source-build-externals dependency update in aspnetcore after this is merged? |
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. |
@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. |
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. |
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. |
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? |
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:
We'll get these recommendations documented in the repo. |
No description provided.