Add IWebHostBuilder.UseKestrel(options) overload to configure Kestrel (#720) #741
Conversation
|
This looks good to me |
|
|
||
| namespace Microsoft.AspNetCore.Server.Kestrel | ||
| { | ||
| public static class KestrelServerOptionsExtensions |
There was a problem hiding this comment.
KestrelServerOptionsHttpsExtensions
|
Its less exotic that current, which can only be good 🚀 |
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.Logging; | ||
|
|
||
| namespace Microsoft.AspNetCore.Server.Kestrel.Filter |
There was a problem hiding this comment.
We can bring this into the Microsoft.AspNetCore.Server.Kestrel namespace with the options type. We just didn't want it in the Microsoft.AspNetCore.Builder namespace.
There was a problem hiding this comment.
Now that I think about it, it doesn't matter if this is in the same namespace as the options type because it's used inside a lamda.
|
This should be targeting Release, no? |
Yep, we spoke about this and @mikeharder will move it to release. |
|
👍 |
|
Appveyor seems like a random failure |
| return hostBuilder.ConfigureServices(services => services.AddSingleton<IServerFactory, ServerFactory>()); | ||
| } | ||
|
|
||
| public static IWebHostBuilder UseKestrel(this IWebHostBuilder hostBuilder, Action<KestrelServerOptions> options) |
There was a problem hiding this comment.
Is there some special reason we need to configure the options via WebHostBuilder rather than doing it in Start.ConfigureServices with a ConfigureKestrel(Action<KestrelServerOptions> options) extension like we'd use for middleware? This won't execute until after ConfigureServices anyways. And then devs can use services injected via Startup's ctor.
There was a problem hiding this comment.
We want to split server configuration from application configuration. Of course the user can also configure options in their startup class but this is the default experience we want to preserve. It also keeps the server specific stuff in Program.Main instead of putting it in the user's Startup class by default.
|
@davidfowl, @DamianEdwards, @Tratcher, @halter73 What namespaces should we use for the new extension methods UseHttps(this KestrelServerOptions) and UseConnectionLogging(this KestrelServerOptions)? The typical convention is to put extension methods in the namespace of the object they extend, which would be Microsoft.AspNetCore.Server.Kestrel in this case. However, this may be slightly confusing, since customers could call UseKestrel() without the Kestrel namespace, and many properties would be settable, but the extension methods UseHttps() and UseConnectionLogging() would not be available until they added the Kestrel namespace. Alternatively, we could put the new extension methods in the same namespace as UseKestrel(), which is Microsoft.AspNetCore.Hosting. However, this breaks the convention of having extension methods in the same namespace as the object they are extending. |
|
Put them in |
mikeharder
force-pushed
the
mikeharder/use-kestrel-options
branch
from
57bf20d
to
b2cd32d
Compare
|
👍 for Microsoft.AspNetCore.Hosting namespaced KestrelServerOptions*Extensions. |
|
🚢 |
…Filter to Kestrel. Rename class from LoggingFilterKestrelOptionsExtensions to KestrelServerOptionsConnectionLoggingExtensions.
…er needed since the UseConnectionLogging() extension method was moved to Microsoft.AspNetCore.Server.Kestrel.
…strelExtensions, to follow the pattern "<TypeExtended><FunctionalityAdded>Extensions".
Add doc comments to all overloads of UseHttps().
…e.Hosting namespace.
…rnal class in Kestrel namespace to public class in Kestrel.Internal namespace.
mikeharder
force-pushed
the
mikeharder/use-kestrel-options
branch
from
ee57216
to
9515b61
Compare
Src and samples have been changed. Still working on tests. Most diffs are just renames from "information" to "options".
@halter73, @davidfowl, @Tratcher, @DamianEdwards