Add overload to HealthChecks for configuring options #25238
Conversation
|
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
| throw new ArgumentNullException(nameof(configureOptions)); | ||
| } | ||
|
|
||
| services.AddOptions<HealthCheckServiceOptions>().Configure(configureOptions); |
There was a problem hiding this comment.
Isn't it sufficient to call services.Configure(configureOptions)?
There was a problem hiding this comment.
Could you add a unit test for this?
| /// <param name="services">The <see cref="IServiceCollection"/> to add the <see cref="HealthCheckService"/> to.</param> | ||
| /// <param name="configureOptions">A delegate to configure the <see cref="HealthCheckServiceOptions"/>.</param> | ||
| /// <returns>An instance of <see cref="IHealthChecksBuilder"/> from which health checks can be registered.</returns> | ||
| public static IHealthChecksBuilder AddHealthChecks<TService>(this IServiceCollection services, Action<HealthCheckServiceOptions, TService> configureOptions) where TService : class |
There was a problem hiding this comment.
This overload allows to resolve a service from IoC container to configure the options.
Several services now have both overloads. 5a375a7
There was a problem hiding this comment.
This doesn't seem worth adding, the underlying options builder api supports up to 5 services, and it's an uncommon enough scenario that it doesn't seem worth adopting as a general pattern to have all AddXyz methods add this overload. I think we should encourage people who want to configure options with services to do it via optionsbuilder
|
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
|
@dougbu do we need to do anything to make the PublicAPI files to be regened for this project? |
|
@pranavkm yes, this PR was created before the project even had public API baselines. You'll need to open the project or a containing solution in VS, build and use the analyzer's fixer to add the new API to the baselines. Do not merge before updating because it'll merge cleanly and break every subsequent build. There's more information in #24347 |
|
@dougbu I'm having no luck getting the analyzer to complain. Do I need to do enable something for it work? |
Nothing other than the steps I outlined should be necessary once you've built inside Visual Studio and looked at the errors and warnings for RS0016 (think that's the right one) |
|
Frankly, I don't think this PR fixes #8530 and the issue doesn't look valid. #8530 (comment) |
Fixes #8530