-
-
Notifications
You must be signed in to change notification settings - Fork 317
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
Conversation
@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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :
-
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 !
-
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 :)
There was a problem hiding this comment.
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!
- 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! - 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:
- Job options are applied, first global, then job specific so that they win -> here
- 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.
@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. |
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
andErrCronJobInvalid
i have change it toErrCronJobInvalid
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
example_test.go
README.md
Notes
This is very straight forward approach i have think this needs more refinements, suggestions appreciated