Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Merge Kestrel.Https into Kestrel.Core #2224

Merged
merged 2 commits into from
Dec 18, 2017
Merged

Merge Kestrel.Https into Kestrel.Core #2224

merged 2 commits into from
Dec 18, 2017

Conversation

Tratcher
Copy link
Member

This is being done because #2186 is trying to make Https first class, but the assembly separation is causing some awkward design problems.

Type forwards have been included to mitigate potential breaks.

This also fixes the sample app.

@Tratcher Tratcher added this to the 2.1.0-preview1 milestone Dec 18, 2017
@Tratcher Tratcher self-assigned this Dec 18, 2017
@Tratcher Tratcher requested a review from halter73 December 18, 2017 20:51
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Assuming it builds ofc.

@davidfowl
Copy link
Member

We’ll have to figure out how this affects bedrock. The more I think about it. The more it might just make sense to make a new sever and set of APIs

@Tratcher Tratcher force-pushed the tratcher/mergehttps branch from bd48264 to 9cb1acd Compare December 18, 2017 22:46
@halter73
Copy link
Member

@davidfowl We can still use the bedrock abstractions for HTTPS even if it's shipped in one assembly. We only want the assemblies merged to make the default-on HTTPS scenario better in Kestrel.

I don't expect any other connection middleware has the same requirements. If we weren't trying to do the following, I wouldn't merge the two assemblies:

  1. Skipping the application of the default HTTPS adapter only when one hasn't already been applied to a given endpoint. The IsHttps addition to IConnectionAdapter is a clear sign that the HTTPS is indeed a very special case.

    Right now we check IsHttps in the Core assembly, but this might be less of an issue if the UseHttps extension method is changed to examine the existing ConnectionAdapters and throw whenever an HTTPS adapter has already been added. If we did this though, it could cause issues for people trying to reconfigure the default HTTPS endpoint using ConfigureHttpsDefaults.

  2. Allowing the developer to specify a configuration callback using ConfigureHttpsDefaults that runs for the default HTTPS endpoint any any HTTPS endpoints defined afterwards even if the endpoint is defined using the legacy UseHttps() API which is just a static extension method on Kestrel.Core.ListenOptions and therefore does not have a real way to access the default HTTPS configuration unless it's referenced by ListenOptions.

If it weren't for these two issues, I think I would be fine with keeping the assemblies separated. Maybe @Tratcher has more reasons to keep them merged though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants