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

Revert HasKey binary breaking change and add a test for IdentityServer4 #20620

Merged
merged 1 commit into from
Apr 14, 2020
Merged

Revert HasKey binary breaking change and add a test for IdentityServer4 #20620

merged 1 commit into from
Apr 14, 2020

Conversation

AndriySvyryd
Copy link
Member

No description provided.

@AndriySvyryd AndriySvyryd requested a review from ajcvickers April 14, 2020 00:31
@AndriySvyryd AndriySvyryd requested a review from dougbu as a code owner April 14, 2020 00:31
@AndriySvyryd
Copy link
Member Author

cc @davidfowl

@ajcvickers
Copy link
Contributor

@AndriySvyryd I we just not going to allow a generic key builder ever, or should we have a plan to try to do this in some other way, or in conjunction with Identity Server changes?

@lajones
Copy link
Contributor

lajones commented Apr 14, 2020

@AndriySvyryd Agree with @ajcvickers above. Could we add a comment saying why we are not doing this the normal way in the generic EntityTypeBuilder ?

@davidfowl
Copy link
Member

Maybe add an extension method to hide the cast?

@AndriySvyryd
Copy link
Member Author

@AndriySvyryd I we just not going to allow a generic key builder ever, or should we have a plan to try to do this in some other way, or in conjunction with Identity Server changes?

We can try again when we have a more compelling reason to do this or when Identity Server is going to be broken by a different change. I don't think it's worth it to keep this on the backlog.

@AndriySvyryd Agree with @ajcvickers above. Could we add a comment saying why we are not doing this the normal way in the generic EntityTypeBuilder ?

It would be basically the comment that's applicable to any public API: "Don't change this, it breaks stuff". The test I added is more reliable.

Maybe add an extension method to hide the cast?

We'll consider this in the future. For now the users don't need to have the generic type.

@AndriySvyryd AndriySvyryd merged commit 7a30fe7 into master Apr 14, 2020
@AndriySvyryd AndriySvyryd deleted the IdentityServerTests branch April 14, 2020 18:57
@AndriySvyryd
Copy link
Member Author

@dougbu EF can be unpinned now

dougbu added a commit to dotnet/aspnetcore that referenced this pull request Apr 15, 2020
@dougbu
Unpin dependencies from dotnet/efcore
9be6bff 
- dotnet/efcore#20620 unblocks flowing these dependencies
@dougbu dougbu mentioned this pull request Apr 15, 2020
Merged
dougbu added a commit to dotnet/aspnetcore that referenced this pull request Apr 15, 2020
@dougbu
[master] Unpin and update efcore dependencies (#20836)
162817d 
* Unpin dependencies from dotnet/efcore
  - dotnet/efcore#20620 unblocks flowing these dependencies

* [master] Update to dependencies from build '20200414.8' of 'https://github.com/dotnet/efcore'
@ajcvickers ajcvickers mentioned this pull request Apr 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajcvickers ajcvickers ajcvickers approved these changes

@dougbu dougbu Awaiting requested review from dougbu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AndriySvyryd @ajcvickers @lajones @davidfowl