Add PreferHostingUrls to WebHostBuilder and IServerAddressesFeature #992

JunTaoLuo · 2017-03-27T18:16:02Z

Let's see if we like this naming.

cesarblum · 2017-03-27T19:20:21Z

src/Microsoft.AspNetCore.Hosting.Abstractions/IWebHostBuilder.cs

@@ -47,6 +48,12 @@ public interface IWebHostBuilder
        IWebHostBuilder UseSetting(string key, string value);

        /// <summary>
+        /// Indicate that the host should listen on the urls configured on the <see cref="IWebHostBuilder"/> instead of those configured on the <see cref="IServer"/>.


cesarblum · 2017-03-27T19:24:43Z

src/Microsoft.AspNetCore.Hosting.Server.Abstractions/Features/IServerAddressesFeature.cs

@@ -8,5 +8,7 @@ namespace Microsoft.AspNetCore.Hosting.Server.Features
    public interface IServerAddressesFeature
    {
        ICollection<string> Addresses { get; }
+
+        bool PreferHostingUrls { get; set; }


Why is it settable?

Hosting needs to set it.

On the concrete type not the interface.

We discussed this. The problem is that the feature is set by the server. Hosting sets it on the interface. Would have to cast to do that.

cesarblum · 2017-03-27T19:25:23Z

test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs

@@ -67,11 +67,13 @@ public void UseStartupThrowsWithNull()
        }

        [Fact]
-        public void NoDefaultAddressesIfNotConfigured()
+        public void NoDefaultAddressesAndDoNotPreferHostingUrlsIfNotConfigured()


There are orthogonal. Add a new test for PreferHostingUrls.

This is just making sure the default is false. I will add the check for the new flag in the other tests where only address is configured.

This is just making sure the default is false

I understand, but it should be in its own test.

cesarblum · 2017-03-27T19:27:33Z

test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs

@@ -121,6 +123,31 @@ public void UsesNewConfigurationOverLegacyConfigForAddresses()
        }

        [Fact]
+        public void DoNotPreferHostingUrlsWhenNoAddressConfigured()


Add test with address configured but PreferHostingUrls() not called.

cesarblum · 2017-03-27T19:30:02Z

test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs

+        public void DoNotPreferHostingUrlsWhenNoAddressConfigured()
+        {
+            var host = CreateBuilder().UseServer(this).PreferHostingUrls().Build();
+            host.Start();


Shouldn't you dispose host in all these tests?

Probably, though we never had a problem with it since these are using a fake server, it doesn't consume any ips/ports. The hosts will be GC'ed and disposed automatically.

Ok, it's mostly for consistency - I see other tests disposing it. Not sure all of them do though.

davidfowl · 2017-03-27T22:34:08Z

src/Microsoft.AspNetCore.Hosting.Abstractions/IWebHostBuilder.cs

+        /// Indicate that the host should listen on the URLs configured on the <see cref="IWebHostBuilder"/> instead of those configured on the <see cref="IServer"/>.
+        /// </summary>
+        /// <returns>The <see cref="IWebHostBuilder"/>.</returns>
+        IWebHostBuilder PreferHostingUrls();


Make it an extension method that sets a configuration change.

Only if we flow this through configuration which exposes it to things like environment variables, ... I was originally gonna put this option there but I thought we explicitly didn't want this.

davidfowl · 2017-03-28T04:34:42Z

test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs

@@ -67,15 +67,19 @@ public void UseStartupThrowsWithNull()
        }


The reformatting makes these tests hard to review. Was it needed?

We weren't disposing the hosts created during tests. It shouldn't be a problem but @CesarBS suggested to make it consistent and dispose. Hence the changes.

Those sorts of changes make it super hard to review... Next time do it later 😄

https://github.com/aspnet/Hosting/pull/992/files?w=1

@natemcmaster That doesn't even help a little bit.

natemcmaster · 2017-03-28T19:13:02Z

test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs

+        [Fact]
+        public void DoNotPreferHostingUrlsWhenNoAddressConfigured()
+        {
+            var host = CreateBuilder().UseServer(this).PreferHostingUrls(true).Build();


oops missed one

natemcmaster · 2017-03-28T19:13:23Z

test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs

@@ -67,15 +67,19 @@ public void UseStartupThrowsWithNull()
        }


https://github.com/aspnet/Hosting/pull/992/files?w=1

JunTaoLuo · 2017-03-28T23:05:38Z

cc @glennc @DamianEdwards @shirhatti for naming approval.

natemcmaster · 2017-03-28T23:17:05Z

@JunTaoLuo can you share samples of what users code might look like?

natemcmaster · 2017-03-28T23:21:33Z

I'm not sure PreferHostingUrls makes it clear what the user is configuring.

That said, naming is hard. Been thinking about this and can't come up w/ anything better. And PreferConfigurationFromIWebHostBuilderOverIServer is certainly not good.

JunTaoLuo · 2017-03-29T00:22:59Z

What would you like to see in a sample? I would imagine the default would look like the following

public static void Main(string[] args)
{
    var host = new WebHostBuilder()
        .PreferHostingUrls(true)
        .UseKestrel(options => options.Listen(IPAddress.Loopback, 5001))
        .UseContentRoot(Directory.GetCurrentDirectory())
        .useStartup<Startup>()
        .Build();

    host.Run();
}

This way, http://localhost:5001 configured on the server is used if no address is configured by the environment variable. However, if ASPNETCORE_URLS is set, then that will override the default configured on Kestrel (once the changes in Kestrel are made).

cesarblum · 2017-03-29T00:48:57Z

@natemcmaster @JunTaoLuo How about PreferWebHostBuilderUrls?

davidfowl · 2017-03-29T01:43:14Z

@natemcmaster @JunTaoLuo How about PreferWebHostBuilderUrls?

Downvotes are so passive aggressive. So here's some text describing the downvote. 👎

cesarblum · 2017-03-29T18:05:55Z

Well, I tried :P

natemcmaster · 2017-03-30T01:37:22Z

Add PreferHostingUrls to IServerAdressesFeature Add extension to IWebHostBuilder to set this flag

dnfclas added the cla-already-signed label Mar 27, 2017

cesarblum suggested changes Mar 27, 2017

View reviewed changes

cesarblum reviewed Mar 27, 2017

View reviewed changes

JunTaoLuo requested review from davidfowl and Tratcher March 27, 2017 21:57

davidfowl reviewed Mar 27, 2017

View reviewed changes

Tratcher approved these changes Mar 28, 2017

View reviewed changes

davidfowl reviewed Mar 28, 2017

View reviewed changes

natemcmaster reviewed Mar 28, 2017

View reviewed changes

JunTaoLuo force-pushed the johluo/prefer-hosting-urls branch from 1c7cb4e to 525ed34 Compare March 28, 2017 19:18

JunTaoLuo requested review from glennc and shirhatti March 28, 2017 23:09

shirhatti approved these changes Mar 30, 2017

View reviewed changes

Server addresses configuration enhancements

4cdc970

Add PreferHostingUrls to IServerAdressesFeature Add extension to IWebHostBuilder to set this flag

JunTaoLuo force-pushed the johluo/prefer-hosting-urls branch from 525ed34 to 4cdc970 Compare March 30, 2017 17:17

JunTaoLuo merged commit 4cdc970 into dev Mar 30, 2017

JunTaoLuo deleted the johluo/prefer-hosting-urls branch March 30, 2017 17:19

This was referenced Mar 30, 2017

More decoupling aspnet/KestrelHttpServer#1572

Merged

PreferHostingUrls added in IServerAddressesFeature should be honoured aspnet/HttpSysServer#336

Closed

Tratcher mentioned this pull request Apr 6, 2017

UseIISIntegration should set IServerAddresses.PreferHostingUrls aspnet/IISIntegration#352

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PreferHostingUrls to WebHostBuilder and IServerAddressesFeature #992

Add PreferHostingUrls to WebHostBuilder and IServerAddressesFeature #992

JunTaoLuo commented Mar 27, 2017

cesarblum Mar 27, 2017

cesarblum Mar 27, 2017 •

edited

Loading

JunTaoLuo Mar 27, 2017

davidfowl Mar 27, 2017

cesarblum Mar 27, 2017 •

edited

Loading

davidfowl Mar 27, 2017

cesarblum Mar 27, 2017

JunTaoLuo Mar 27, 2017

cesarblum Mar 27, 2017

cesarblum Mar 27, 2017 •

edited

Loading

cesarblum Mar 27, 2017

JunTaoLuo Mar 27, 2017

cesarblum Mar 27, 2017

davidfowl Mar 27, 2017 •

edited

Loading

JunTaoLuo Mar 27, 2017 •

edited

Loading

davidfowl Mar 28, 2017

JunTaoLuo Mar 28, 2017

davidfowl Mar 28, 2017

natemcmaster Mar 28, 2017

davidfowl Mar 28, 2017

natemcmaster Mar 28, 2017

JunTaoLuo Mar 28, 2017

natemcmaster Mar 28, 2017

JunTaoLuo commented Mar 28, 2017 •

edited

Loading

natemcmaster commented Mar 28, 2017

natemcmaster commented Mar 28, 2017

JunTaoLuo commented Mar 29, 2017

cesarblum commented Mar 29, 2017

davidfowl commented Mar 29, 2017 •

edited

Loading

cesarblum commented Mar 29, 2017

natemcmaster commented Mar 30, 2017

Add PreferHostingUrls to WebHostBuilder and IServerAddressesFeature #992

Add PreferHostingUrls to WebHostBuilder and IServerAddressesFeature #992

Conversation

JunTaoLuo commented Mar 27, 2017

Choose a reason for hiding this comment

cesarblum Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

JunTaoLuo Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo commented Mar 28, 2017 • edited Loading

natemcmaster commented Mar 28, 2017

natemcmaster commented Mar 28, 2017

JunTaoLuo commented Mar 29, 2017

cesarblum commented Mar 29, 2017

davidfowl commented Mar 29, 2017 • edited Loading

cesarblum commented Mar 29, 2017

natemcmaster commented Mar 30, 2017

cesarblum Mar 27, 2017 •

edited

Loading

cesarblum Mar 27, 2017 •

edited

Loading

cesarblum Mar 27, 2017 •

edited

Loading

davidfowl Mar 27, 2017 •

edited

Loading

JunTaoLuo Mar 27, 2017 •

edited

Loading

JunTaoLuo commented Mar 28, 2017 •

edited

Loading

davidfowl commented Mar 29, 2017 •

edited

Loading