-
Notifications
You must be signed in to change notification settings - Fork 527
Conversation
Easier to review with https://github.com/aspnet/KestrelHttpServer/pull/1655/files?w=1 |
This is going to conflict with my Start/Stop PR that I'm merging tomorrow. |
Go ahead and merge your changes for startasync/stopasync. I'll rework this PR. |
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.
One minor nit, then
} | ||
|
||
[ConditionalFact] | ||
[PortSupportedCondition(5000)] |
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.
The test uses 5001, not 5000.
840ddd2
to
2398330
Compare
if (hasListenOptions) | ||
{ | ||
var joined = string.Join(", ", _serverAddresses.Addresses); | ||
_logger.LogWarning($"Overriding endpoints defined in UseKestrel() since {nameof(IServerAddressesFeature.PreferHostingUrls)} is set to true. Binding to address(es) '{joined}' instead."); |
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.
This is something that we expect to happen when PreferHostingUrls is set to true. Is this worth a warning? Or should we log as info?
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.
I'll log as info.
Rebase ofc. |
88f7795
to
08f9056
Compare
6b676d8
to
0723d46
Compare
Addresses #1575. Alternative to #1639
We want to override the endpoints configured in the listen options when PreferHostingUrls is set to true with the addresses in the IServerAddressesFeature if it's not empty.
Will squash with a meaningful commit message once ready to merge.