Add a configuration validator and options formatter for EndpointOptions #4024
Conversation
| @@ -76,31 +104,29 @@ public static class EndpointOptionsExtensions | |||
|
|
|||
| internal static IPEndPoint GetPublicSiloEndpoint(this EndpointOptions options) | |||
| { | |||
| return new IPEndPoint(options.AdvertisedIPAddress, options.SiloPort); | |||
| return options?.AdvertisedIPAddress != null | |||
There was a problem hiding this comment.
I don't think this check is needed since the validator should throw before that.
We could also throw an exception here just in case, currently the silo cannot work without setting the AdvertisedIPAddress. Same comment for GetPublicProxyEndpoint
I think we should do like in 1.5, by default take one non-localhost ip address that we found, with |
| @@ -62,13 +63,15 @@ public class SiloHostBuilderTests | |||
| [Fact] | |||
| public void SiloHostBuilder_AssembliesTest() | |||
| { | |||
| var builder = (ISiloHostBuilder) new SiloHostBuilder() | |||
| .ConfigureServices(services => services.AddSingleton<IMembershipTable, NoOpMembershipTable>()); | |||
| var builder = new SiloHostBuilder().ConfigureEndpoints(IPAddress.Any, 9999, 0) | |||
There was a problem hiding this comment.
Sorry to have missed that, but I think we should throws at validation when trying to use IPAddress.Any for published ip address. I don't think it make sense?
|
LGTM as is, I can add some checks and add default value in another PR if you want |
|
@benjaminpetit sounds good to me. This is just a drive-by PR, since I hit the issue of not having a default endpoint value in another PR |
|
@ReubenBond Please rebase. |
ReubenBond
force-pushed
the
validate-endpoints
branch
from
e7875fd
to
6fcd181
Compare
@benjaminpetit this might also need a default value for
AdvertisedIPAddress, but I'm not sure if the default should beAny,Localhost, or something else.