Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow custom configuration via Options pattern #955

Closed
ljani opened this issue Sep 10, 2020 · 9 comments · Fixed by #978
Closed

Allow custom configuration via Options pattern #955

ljani opened this issue Sep 10, 2020 · 9 comments · Fixed by #978
Milestone

Comments

@ljani
Copy link

ljani commented Sep 10, 2020

Is your feature request related to a problem? Please describe.
"I'm always frustrated when" I cannot access IServiceProvider in the configure action of AddQuartz.

Describe the solution you'd like
AddQuartz should have an overload like AddQuartz(this IServiceCollection serviceCollection, Action<IServiceProvider, IServiceCollectionQuartzConfigurator>? configure = null) similar to eg. AddContext.

Describe alternatives you've considered
Alternatively Quartz could expose an options interface, and I could configure with eg. IConfigureOptions<QuartzOptions>. For example AddCors does this with CorsOptions. One can use IConfigureOptions<CorsOptions> to configure cors and access IServiceProvider.

As a workaround I can manually bootstrap the dependencies I'd like to access in AddQuartz.

Additional context
My use cases are similar to Andrew Lock's described in this blog post. Hope you find some inspiration there.

Many thanks!

EDIT: To be clear, this is a minor improvement on the configuration interface as feedback was requested.

@lahma
Copy link
Member

lahma commented Sep 10, 2020

I think the overload your described AddQuartz(this IServiceCollection serviceCollection, Action<IServiceProvider, IServiceCollectionQuartzConfigurator>? configure = null) makes quite a lot of sense. Would you like offer a PR to add one?

On second note if there's something that is hard to get configured in Quartz via the provided configuration API it might also make sense to add typed helpers there, not sure about your use case though.

@ljani
Copy link
Author

ljani commented Sep 10, 2020

I'm afraid I'm not familiar enough with Quartz to create a PR with a reasonable work estimate. So, sorry, not this time.

I accidentally linked wrong issue regarding the feedback, it's fixed now.

@lahma
Copy link
Member

lahma commented Sep 10, 2020

All fine, I'll see what I can do 👍

@lahma lahma added this to the 3.2 milestone Sep 10, 2020
@ljani
Copy link
Author

ljani commented Sep 11, 2020

To clarify, here's a simple use case with strongly typed settings:

.AddQuartz((serviceProvider, configurator) => {
    var cron = serviceProvider
        .GetRequiredService<IOptions<MyOptions>>()
        .Value
        .Cron;
    configurator.AddTrigger(trigger => trigger
        .ForJob("MyJob")
        .WithCronSchedule(cron));
});

@lahma
Copy link
Member

lahma commented Sep 11, 2020

For file-based, auto-refreshed, schedule configuration there's also the XML option, but you have probably already checked that out too. But I see the use case for retrieving things from own configuration/service infrastructure.

@kevinchalet
Copy link
Contributor

kevinchalet commented Sep 18, 2020

My 2-cent: adding such an overload will be very complicated, as the IServiceProvider is not available from ConfigureServices (unless you call services.BuildServiceProvider() to create a temporary DI, but it's a discouraged practice, which should even flag an alert in recent VS/.NET Core versions).

The use case described in @ljani's latest post would be easily solved if Quartz.NET used Microsoft.Extensions.Options (as suggested in the OP and discussed recently in the OpenIddict repo). In this case, doing it inline should be as simple as:

services.AddOptions<QuartzOptions>()
    .Configure<IOptions<MyOptions>>((options, dep) =>
    {
        options.AddTrigger(trigger => trigger
            .ForJob("MyJob")
            .WithCronSchedule(dep.Value.Cron));
    });

@lahma
Copy link
Member

lahma commented Sep 18, 2020

Thanks @kevinchalet for chiming in! I did a quick look earlier and this service provider indeed seemed quite hard to achieve. Now I need to study the options model 🙂

@lahma lahma changed the title AddQuartz should have access to IServiceProvider Allow custom configuration via Options pattern Sep 30, 2020
@lahma
Copy link
Member

lahma commented Sep 30, 2020

So I've done changes required to achieve @kevinchalet 's proposed idea with PR #978 . Hopefully this is nor more in line with expectations from options pattern and allows to do the Configure.

@ljani
Copy link
Author

ljani commented Oct 1, 2020

@lahma Looks great for my needs, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants