-
Notifications
You must be signed in to change notification settings - Fork 482
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 duration to be used for scheduling every #607
base: master
Are you sure you want to change the base?
Conversation
0cdf0ae
to
0849a0b
Compare
) | ||
|
||
assert_equal(1, Resque::Scheduler.rufus_scheduler.jobs.size) | ||
assert_equal(1, Resque::Scheduler.scheduled_jobs.size) |
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.
Why not test the new job is well scheduled every 30s, and not 30m or 30ms ?
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.
agreed, we need to test the actual schedule that's configured in the scheduler here.
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.
@josephpage @iloveitaly definitely, updated!
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 is a great change! Left a couple comments, but we also need to:
- Update the changelog
- Rebase on master so CI runs properly
@@ -136,7 +136,7 @@ def load_schedule_job(name, config) | |||
interval_defined = false | |||
interval_types = %w(cron every) | |||
interval_types.each do |interval_type| | |||
next unless !config[interval_type].nil? && !config[interval_type].empty? | |||
next unless !config[interval_type].nil? && !config[interval_type].to_s.empty? |
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.
What is this fixing?
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.
@iloveitaly I believe this is since the interval type can now be Numeric
and empty?
is a String
only.
) | ||
|
||
assert_equal(1, Resque::Scheduler.rufus_scheduler.jobs.size) | ||
assert_equal(1, Resque::Scheduler.scheduled_jobs.size) |
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.
agreed, we need to test the actual schedule that's configured in the scheduler here.
Check for empty should be done on string Format numeric every for server Fix Rubocop complaint in test
7871ac1
to
5093a3e
Compare
Feature based on #605.
Since rufus-scheduler allows for numeric values for
every
, durations can be used like30.minutes
and1.hour
.