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

feat:custom-cron interface for own custom cron implimentation #834

Merged
merged 7 commits into from
Feb 25, 2025

Conversation

Dojeto
Copy link
Contributor

@Dojeto Dojeto commented Feb 20, 2025

What does this do?

This pr for the new feature of custom cron job, added the cron interface for custom cron implementation

Which issue(s) does this PR fix/relate to?

Resolves #804

List any changes that modify/break current functionality

Errors will be change since from two different error ErrCronJobParse and ErrCronJobInvalid i have change it to ErrCronJobInvalid only one which leads to failure in test cases.

Have you included tests for your changes?

No

Did you document any new/modified functionality?

No

  • [⛌] Updated example_test.go
  • [⛌] Updated README.md

Notes

This is very straight forward approach i have think this needs more refinements, suggestions appreciated

@Dojeto
Copy link
Contributor Author

Dojeto commented Feb 20, 2025

@JohnRoesler what's your thoughts on this implementation, my test cases are getting failed due to combine the two different error into one let me know what kinda changes require in this implementation and what should we do for the error ? if you allow me to do with one error then i will change testcases and test and refine it

job.go Outdated
}

func (c cronJobDefinition) setup(j *internalJob, location *time.Location, now time.Time) error {
func (c *cronJobDefinition) IsValid() bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than having the JobDefinition implement the Cron interface, I was envisioning that the cronJobDefinition itself would include the Cron interface as a struct field (it will also need to be added to the internalJob struct). Then, in the setup method of the cronJobDefinition, could grab the Cron interface from the internalJob and utilize that.

The default Cron implementation would be the current one with robfig. Then, add a new JobOption that takes in a Cron interface and sets it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution keeps us to a single CronJob definition and instead allows altering the Cron implementation via a Job option which can be set for a single job, or globally using the WithGlobalJobOptions method on the scheduler.

Copy link
Contributor Author

@Dojeto Dojeto Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are few points that i want to discuss :

  1. Do i have to make a different Jobdefination function since it has extra field which might cause confusion on the Custom implementation via Job option !

  2. Where we are going to initialized the cron ?

func WithCronImplementation(c Cron) JobOption { return func(j *internalJob, _ time.Time) error { j.cron = c return nil } }

func (c cronJobDefinition) setup(j *internalJob, location *time.Location, _ time.Time) error { c.cron = j.cron

i hope this is exactly what you are trying to say

i am confused like initialize job and passing implementation is different thing Job initializing happening in job definition and implementation passing through job options how we can use them together. sorry if i am asking dumb question this pattern is new to me and i like this pattern :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dojeto No worries on asking questions!

  1. No, I don't think you need a different JobDefinition. if the implementation doesn't care about the withSeconds bool it can simply ignore it!
  2. Yeah, a little confusing maybe, I'll try and explain:

The Job itself is "initialized" in the NewJob method on the Scheduler, which in turn just calls the internal method addOrUpdateJob. If you look in that method, you'll see the following order of operations:

  1. Job options are applied, first global, then job specific so that they win -> here
  2. Then the jobDefinitions setup method is called https://github.com/go-co-op/gocron/blob/v2/scheduler.go#L742

So, in this case, the Cron interface will be initialized either by the caller and then passed in via the WithCronImplementation JobOption, which in turn sets the Cron onto the internalJob struct. OR, the Cron will be initialized inside the setup method as the default implementation.

Then, in the setup method, IsValid should be called.

@Dojeto
Copy link
Contributor Author

Dojeto commented Feb 21, 2025

@JohnRoesler how it is ? test cases getting failed due to change in error, i have check seems like it is working, let me know what's your thoughts on this ? let me know required changes.

@Dojeto Dojeto requested a review from JohnRoesler February 21, 2025 16:58
@JohnRoesler JohnRoesler marked this pull request as ready for review February 25, 2025 03:24
@JohnRoesler JohnRoesler merged commit 36cd85f into go-co-op:v2 Feb 25, 2025
5 checks passed
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 this pull request may close these issues.

[FEATURE] - run a job on the last weekday of the month
2 participants