Remove seconds from scheduled jobs cron format #30227
Conversation
soltysh
added
area/batch
release-note-none
k8s-github-robot
added
the
size/M
| if err != nil { | ||
| allErrs = append(allErrs, field.Invalid(fldPath, schedule, err.Error())) | ||
| } | ||
| if ssObj, ok := obj.(*cron.SpecSchedule); ok { | ||
| if ssObj.Second != 1 { |
There was a problem hiding this comment.
move this to the end of the parent if
|
@janetkuo comments addressed, ptal |
| @@ -125,7 +125,11 @@ func getNextStartTimeAfter(schedule string, now time.Time) (time.Time, error) { | |||
| // If there were missed times prior to the last known start time, then those are not returned. | |||
| func getRecentUnmetScheduleTimes(sj batch.ScheduledJob, now time.Time) ([]time.Time, error) { | |||
| starts := []time.Time{} | |||
| sched, err := cron.Parse(sj.Spec.Schedule) | |||
| tmpSched := sj.Spec.Schedule | |||
There was a problem hiding this comment.
Shall we make this (L128-132) our own Parse function?
There was a problem hiding this comment.
This looks like only parsed data adjust, so maybe it should have other name, but still extracting this and adding a name for this block - could improve readability of this code.
There was a problem hiding this comment.
Yeah, I'll create one. I've already spoke with cron library author, I'll be addressing those issues in the library, when I'm back from my PTO in Sep.
|
Fix the last two nits and then this lgtm. |
|
Just found that |
I'll do once again a grep over the sources looking for that, just to make sure. Thanks for the review. |
|
Oh, and the example in |
soltysh
force-pushed
the
remove_seconds
branch
from
9a554a9 to
902ecd8
Compare
|
Nits addressed, the temporary method and places where we're adding seconds are commented with a pointer to robfig/cron#58 which when fixed, will replace them. |
soltysh
added
the
lgtm
|
GCE e2e build/test passed for commit 902ecd8. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit 902ecd8. |
|
Automatic merge from submit-queue |
Automatic merge from submit-queue Remove hacks from ScheduledJobs cron spec parsing Previusly `github.com/robfig/cron` library did not allow passing cron spec without seconds. First commit updates the library, which has additional method ParseStandard which follows the standard cron spec, iow. minute, hour, day of month, month, day of week. @janetkuo @erictune as promised in #30227 I've updated the library and now I'm updating it in k8s
@erictune @janetkuo as promised this removes the seconds from the cron format
This change is