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

Dynamic jobs #84

Closed linkdotnet closed 5 months ago

linkdotnet commented 5 months ago

Pull request description

Give the developer means to add and remove jobs dynamically.

PR meta checklist

Code PR specific checklist

falvarez1 commented 5 months ago

@linkdotnet

I appreciate the effort to provide a way to disable a CRON job. However, I don't think using an arbitrary CRON expression like "31st of February" is the right approach. Here are my concerns and suggestions:

  1. Misleading Intent: Using an unattainable date in the CRON expression to disable a job can be misleading and confusing for users. It doesn't clearly convey the intent to disable the job, and users may not understand why this specific date is chosen.
  2. Potential Issues: Relying on an invalid date could lead to unexpected behavior or bugs in the future, especially if the underlying CRON library updates its validation rules.
  3. Proper Solution: We should implement a dedicated feature to disable jobs. This would make the intent clear and the code more maintainable. Here’s a suggestion on how we could approach it:

Add a Disable Method: Add a method in the IRuntimeJobRegistry interface to disable jobs by name. Update Endpoint: Update the endpoint to use this new method.

public interface IRuntimeJobRegistry
{
    ...
    void DisableJob(string name);
}
app.MapPut("/disable-job", (IRuntimeJobRegistry registry) => 
{
    registry.DisableJob("MyName");
    return TypedResults.Ok();
});

We can use the February 31st CRON expression as a temporary internal solution, allowing us to evolve the feature later without changing the API. By following this approach, we make it explicit that a job is being disabled, which enhances code readability and maintainability. This ensures users have a clear and intuitive way to disable jobs, aligning with the principle of least astonishment.

linkdotnet commented 5 months ago

Yeah that is a good idea. Probably for now I just remove that thing entirely from the docs! We can add Disable / Enable later on, if someone really needs that.

Love the Point with upgrading the library and introducing some nasty bugs!

linkdotnet commented 5 months ago

ToDo:

Currenly I have the following API:

IReadOnlyCollection<RecurringJobSchedule> GetAllRecurringJobs();

public sealed record RecurringJobSchedule(Type JobType, string? JobName, string? CronExpression, TimeZoneInfo TimeZone);

@falvarez1 I am fighting to see if I like the name and if the information given is good enough. I would like to expose as least as possible.

falvarez1 commented 5 months ago

If you want we can just add a single method like void SetJobEnabled(string name, bool isEnabled); and manage it initially using this Feb 31st approach, then improve it in the state changes branch.

linkdotnet commented 5 months ago

If you want we can just add a single method like void SetJobEnabled(string name, bool isEnabled); and manage it initially using this Feb 31st approach, then improve it in the state changes branch.

Yeah good idea! Still thinking of tackling that in another PR. I still have some open thoughts, for example: Would a user like to create a scheduled job that is directly disabled?

falvarez1 commented 5 months ago

Would a user like to create a scheduled job that is directly disabled?

This seems like an edge case to me. Users can always create a job and then immediately disable it, assuming we expose the ability to disable jobs. This could be useful if they're creating a template and want to clone it later, but that's beyond our current scope.

linkdotnet commented 5 months ago

I guess if we merge this before #85 then you will have a quite some merge conflicts. @falvarez1

falvarez1 commented 5 months ago

@linkdotnet the PR looks good to me. Let me know if you want my help with resolving merge conflicts.

linkdotnet commented 5 months ago

Yeah I tried and there are many. Therefore I started from scratch: https://github.com/NCronJob-Dev/NCronJob/tree/dynamic-jobs-remastered

but here are a few things missing.

linkdotnet commented 5 months ago

@falvarez1 I dumped that into a chat message instead of posting here.