feat(snapshots): added ability to use cron expressions to schedule snapshots #3149
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3149 +/- ##
==========================================
- Coverage 75.40% 75.40% -0.01%
==========================================
Files 459 459
Lines 36341 36409 +68
==========================================
+ Hits 27404 27453 +49
- Misses 7024 7039 +15
- Partials 1913 1917 +4
☔ View full report in Codecov by Sentry. |
jkowalski
force-pushed
the
cron-schedule
branch
5 times, most recently
from
aa24fe9
to
ab057fb
Compare
…apshots We use `github.com/hashicorp/cronexpr` to parse and evaluate expressions, as documented in https://github.com/hashicorp/cronexpr#implementation
There was a problem hiding this comment.
Overall LG. Just a couple of minor comments and questions.
@redgoat650 or @Shrekster should provide input as well.
| expChangeCount: 1, | ||
| }, | ||
| } { | ||
| t.Log(tc.name) | ||
| tc := tc |
There was a problem hiding this comment.
nit: why not move the declaration for psf here, inside the closure, so it is a new variable every time?
| for _, e := range p.Cron { | ||
| ce, err := cronexpr.Parse(stripCronComment(e)) | ||
| if err != nil { | ||
| // ignore invalid crontab entries, nothing we can do at this point |
There was a problem hiding this comment.
Should this at least be logged?
There was a problem hiding this comment.
...and/or maybe a note pointing the reader to ValidateSchedulingPolicy, where an initial parsing check takes place before the cron strings can be added to the policy
There was a problem hiding this comment.
i don't want to log here since it happens in a tight loop, so if we ever log anything that's gonna flood the log file, added comment
snapshot/policy/scheduling_policy.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func stripCronComment(s string) string { | ||
| return strings.TrimSpace(strings.Split(s, "#")[0]) |
There was a problem hiding this comment.
nit: ?
| return strings.TrimSpace(strings.Split(s, "#")[0]) | |
| return strings.TrimSpace(strings.SplitN(s, "#", 2)[0]) |
| pol: policy.SchedulingPolicy{ | ||
| Cron: []string{"0 23 * * *"}, | ||
| }, | ||
| previousSnapshotTime: time.Date(2020, time.January, 1, 19, 0, 0, 0, time.Local), |
There was a problem hiding this comment.
If I'm reading these cases correctly, now is before the previous snapshot, is that right?
Does it make sense to also have cases where now is after the previous snapshot? Both for cases where (a) now is before the time the next snapshot is supposed to be, and (b) now is after when the snapshot was supposed to have been taken.
What is the expected behavior for (b)? Would a new snapshot be taken right away?
There was a problem hiding this comment.
removed previousSnapshotTime since it was not essential here and confusing.
| for _, e := range p.Cron { | ||
| ce, err := cronexpr.Parse(stripCronComment(e)) | ||
| if err != nil { | ||
| // ignore invalid crontab entries, nothing we can do at this point |
There was a problem hiding this comment.
...and/or maybe a note pointing the reader to ValidateSchedulingPolicy, where an initial parsing check takes place before the cron strings can be added to the policy
We use
github.com/hashicorp/cronexprto parse and evaluate expressions, as documented in https://github.com/hashicorp/cronexpr#implementationKapture.2023-07-16.at.11.24.13.mp4
Fixes #671
Related #1723