-
-
Notifications
You must be signed in to change notification settings - Fork 9k
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
CronTab.floor
/ .ceil
should return times at minute granularity
#10239
Conversation
private static void assertFloorCeil(CronTab tab, Calendar now, String expectedFloor, String expectedCeil) { | ||
var fmt = DateFormat.getDateTimeInstance(DateFormat.MEDIUM, DateFormat.MEDIUM); | ||
var nowFormatted = fmt.format(now.getTime()); | ||
var nowClone = (Calendar) now.clone(); |
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.
Beware that floor
and ceil
mutate their argument.
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 PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.
/label ready-for-merge
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.
@jglick LGTM.
f97b5b3 seems to have been calling previously unused utility methods.
jenkins/core/src/main/java/hudson/triggers/Trigger.java
Lines 223 to 224 in 630b6b6
jenkins/core/src/main/java/hudson/scheduler/CronTab.java
Lines 160 to 169 in 630b6b6
*/5 * * * *
would next run at 2:20:23, which is not really true: it would run at 2:20:00, or very shortly thereafter, and at any rate not specifically at 23 seconds past the minute.Testing done
Unit tests, and then some interactive sanity checks of form validation including of
@hourly
etc.Proposed changelog entries
Proposed upgrade guidelines
N/A
Desired reviewers
@Vlatombe
Before the changes are marked as
ready-for-merge
:Maintainer checklist