- Notifications
You must be signed in to change notification settings - Fork928
fix: cli: prettify schedule when printing output#1440
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedMay 13, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #1440 +/- ##==========================================+ Coverage 67.10% 67.12% +0.01%========================================== Files 291 291 Lines 19527 19557 +30 Branches 258 258 ==========================================+ Hits 13104 13127 +23- Misses 5073 5081 +8+ Partials 1350 1349 -1
Continue to review full report at Codecov.
|
| @@ -49,9 +49,18 @@ func Weekly(spec string) (*Schedule, error) { | |||
| return nil, xerrors.Errorf("expected *cron.SpecSchedule but got %T", specSched) | |||
| } | |||
| tz := "UTC" | |||
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.
I think this is a bit brittle. The CLI will always pass a schedule string that starts withCRON_TZ=, but if someone sets a schedule through the API, it could be any valid string that the parser will accept. And if the schedule doesn't include a timezone, it will be interpreted using the server's timezone, which could be different from UTC.
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.
Good point. DefaultingCRON_TZ=UTC is probably the simplest way here.
| if strings.HasPrefix(raw, "CRON_TZ=") { | ||
| tz = strings.TrimPrefix(strings.Fields(raw)[0], "CRON_TZ=") | ||
| cron = strings.Join(strings.Fields(raw)[1:], " ") | ||
| } |
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.
It really bothers me that we have to do this using direct string manipulation instead of just looking at the parsed schedule, but it doesn't seem like this cron library provides a better way 😞
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 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.
Seems reasonable to me!
* Adds methods to schedule.Schedule to show the raw cron string and timezone* Uses these methods to clean up output of auto(start|stop) show or ls* Defaults CRON_TZ=UTC if not provided
Uh oh!
There was an error while loading.Please reload this page.
Ref:#1431 (comment)
This PR does the following:
schedule.Scheduleto show the raw cron string and timezoneauto(start|stop) showorlsCRON_TZ=UTCif not provided