NCronJob-Dev / NCronJob

A Job Scheduler sitting on top of IHostedService in dotnet.
https://docs.ncronjob.dev/
MIT License
158 stars 11 forks source link

feat: Startup Jobs are run before anything else is running #115

Closed linkdotnet closed 2 weeks ago

linkdotnet commented 3 weeks ago

This PR will hold all new features for v4

Additional

This PR:

coveralls commented 3 weeks ago

Pull Request Test Coverage Report for Build 11627735694

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/NCronJob/NCronJobExtensions.cs 12 13 92.31%
<!-- Total: 12 13 92.31% -->
Files with Coverage Reduction New Missed Lines %
src/NCronJob/Execution/StartupJobManager.cs 2 86.67%
src/NCronJob/Scheduler/JobProcessor.cs 3 73.08%
<!-- Total: 5 -->
Totals Coverage Status
Change from base Build 11627027565: 2.9%
Covered Lines: 1068
Relevant Lines: 1179

💛 - Coveralls
nulltoken commented 3 weeks ago

How about "driving the lib user into the pit of success :wink:" and making sure he/she doesn't forget to invoke UseNCronJob()?

There was a proposal about this in https://github.com/NCronJob-Dev/NCronJob/issues/37#issuecomment-2444157309

And to make sure the library consumer always invoke .UseNCronJob(), maybe make the QueueWorker
check some kind of marker flag that .UseNCronJob() would set.
Otherwise, throw an InvalidOperationException (".UseNCronJob() is expected to be called
once the application is built.").
linkdotnet commented 3 weeks ago

How about "driving the lib user into the pit of success 😉" and making sure he/she doesn't forget to invoke UseNCronJob()?

There was a proposal about this in #37 (comment)

And to make sure the library consumer always invoke .UseNCronJob(), maybe make the QueueWorker
check some kind of marker flag that .UseNCronJob() would set.
Otherwise, throw an InvalidOperationException (".UseNCronJob() is expected to be called
once the application is built.").

I like that. Currently that isn't really mandatory, ut as you said: "drive the lib user into the pit of success"

Edit: We can add a logger warning as first line of defense. I would not like to crash the application.

nulltoken commented 3 weeks ago

Edit: We can add a logger warning as first line of defense. I would not like to crash the application.

Why? If that becomes mandatory as part of v4, crashing the app would only occur on the developer computer or during the test executions. And that crash would come with a call to action explaining what to do in order to properly plug the lib.

There are some areas of asp.net that behaves in the same way to ensure the global coherence (for instance https://github.com/dotnet/aspnetcore/blob/7f62ae6455a90b1026bd1522f4af3e6de64ee2a9/src/Http/Authentication.Core/src/AuthenticationService.cs#L243)

nulltoken commented 2 weeks ago

Why? If that becomes mandatory as part of v4, crashing the app would only occur on the developer computer or during the test executions. And that crash would come with a call to action explaining what to do in order to properly plug the lib.

Another option could be to only trigger the exception if the jobRegistry contains any Startup jobs.

linkdotnet commented 2 weeks ago

Why? If that becomes mandatory as part of v4, crashing the app would only occur on the developer computer or during the test executions. And that crash would come with a call to action explaining what to do in order to properly plug the lib.

Another option could be to only trigger the exception if the jobRegistry contains any Startup jobs.

I assume we will have more stuff in there than just that (at least in the future).

linkdotnet commented 2 weeks ago

Just as an FYI - I will close this PR and reopen it and will target single features against v4. The v4 PR shouldn't have much "noise".