Use OptionsBuilder to allow chaining Configure calls for providers #3696
Conversation
Configuring with just a delegate is probably the most common scenario, so add back overloads that take an `Action<TOptions>` and not just have `Action<OptionsBuilder<TOptions>>` which requires more ceremony for the end user. This allows this again: `services.UseAzureTableGatewayListProvider(gatewayOptions => gatewayOptions.ConnectionString = TestDefaultConfiguration.DataConnectionString)` Without the overload, users would have to do this: `services.UseAzureTableGatewayListProvider(ob => ob.Configure(gatewayOptions => gatewayOptions.ConnectionString = TestDefaultConfiguration.DataConnectionString))`
…the IConfiguration overload
| @@ -0,0 +1,74 @@ | |||
| // Copyright (c) .NET Foundation. All rights reserved. | |||
There was a problem hiding this comment.
| @@ -0,0 +1,33 @@ | |||
| // Copyright (c) .NET Foundation. All rights reserved. | |||
There was a problem hiding this comment.
| { | ||
| return builder.ConfigureServices(services => services.UseConsulGatewayListProvider(configuration)); | ||
| return builder.ConfigureServices(services => services.UseConsulGatewayListProvider(configureOptions)); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Configure DI container with ConsuleBasedMemebership |
There was a problem hiding this comment.
typo on Consul. Repeated below
| @@ -25,21 +22,15 @@ public static class ZooKeeperHostingExtensions | |||
| /// <summary> | |||
| /// Configure siloHostBuilder with ZooKeeperMembership | |||
There was a problem hiding this comment.
"Configures the silo to use ZooKeeper for cluster membership" - but it's perhaps out of scope for this pr to change the comment
| } | ||
|
|
||
| /// <summary> | ||
| /// Add an <see cref="StaticGatewayListProvider"/> and configure it using <paramref name="configuration"/> | ||
| /// Add an <see cref="StaticGatewayListProvider"/> and configure it using <paramref name="configureOptions"/> | ||
| /// </summary> | ||
| /// <param name="collection"></param> |
There was a problem hiding this comment.
In other files, empty params were removed. Could do the same here
| return collection | ||
| .Configure<StaticGatewayListProviderOptions>(configuration) | ||
| .AddSingleton<IGatewayListProvider, StaticGatewayListProvider>(); | ||
| configureOptions?.Invoke(collection.AddOptions<StaticGatewayListProviderOptions>()); |
There was a problem hiding this comment.
given this can be null, should the param have a default value of null? Fine either way to me
There was a problem hiding this comment.
Actually, scrap that - a default value would make overload resolution ugly.
There was a problem hiding this comment.
I'm not really making that call in this PR, just porting it. Although responding to this particular thing, it might be that we don't really want null, and potentially throw an argument exception... I don't really know at this point which behavior to choose, so I'll leave it undefined for now, unless you already thought through it and can recommend one
|
@dotnet-bot test bvt |
I temporarily copied the implementation of
OptionsBuilderinto the Orleans codebase, but the plan is to delete it onceMicrosoft.Extensions.Optionsv2.1 is released, and includes these. I put them in an Orleans namespace to avoid collisions if they co-exist for a short while.For an explanation why
OptionsBuilderis better than having extension method overloads for every variation of how to configure, see the PR itself: aspnet/Options#238This will even be a lot more useful once we start using named options for our named service providers configuration.
I migrated our current extension methods for the gateway list and membership providers to use this. I did preserve the overload that takes
Action<TOptions>, and the reasoning is explained and shown in the second commit in this PR (let me know if you are OK with this or prefer not to have that overload, which is less boilerplate code for us, but potentially a harder to use API for the common case).In the future, we can also port this PR that was merged today and builds on top of OptionsBuilder: aspnet/Options#239 We already do a very similar thing here)