Fix bad shorthand parse #34
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In crontab.go, we rely on https://github.com/gorhill/cronexpr to parse cron expressions, and to tell us when they're invalid. To choose what to feed into cronexpr, we pick out a number of tokens from the start of the cron line that might yield a valid cron expression, starting with the most tokens (we try 7, then 6, then 5, then 1). When we hit a valid cron expression, we stop there. Unfortunately, I overlooked the fact that cronexpr isn't exactly designed for this use case, and will not return an error when passed e.g. `@hourly foo` (it just ignores `foo` in this case). This is a problem if the user is using a shorthand cron expression and a command with 4 or more tokens: in this case, we'll attempt to parse the shorthand and the 4 first tokens in cronexpr, which will work (it'll ignore the tokens altogether), and we'll be left with a truncated command (missing the 4 first tokens). The ideal fix would be to have cronexpr reject this invalid expression for us, so I'm opening a PR there. In the meantime, I'm also opening this PR in Supercronic with a fix my fork of cronexpr.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In crontab.go, we rely on https://github.com/gorhill/cronexpr to parse
cron expressions, and to tell us when they're invalid.
To choose what to feed into cronexpr, we pick out a number of tokens
from the start of the cron line that might yield a valid cron
expression, starting with the most tokens (we try 7, then 6, then 5,
then 1). When we hit a valid cron expression, we stop there.
Unfortunately, I overlooked the fact that cronexpr isn't exactly
designed for this use case, and will not return an error when
passed e.g.
@hourly foo(it just ignoresfooin this case).This is a problem if the user is using a shorthand cron expression and a
command with 4 or more tokens: in this case, we'll attempt to parse the
shorthand and the 4 first tokens in cronexpr, which will work (it'll
ignore the tokens altogether), and we'll be left with a truncated
command (missing the 4 first tokens).
The ideal fix would be to have cronexpr reject this invalid expression
for us, so I'm opening a PR there. In the meantime, I'm also opening
this PR in Supercronic with a fix my fork of cronexpr.
cc @fancyremarker
fyi @sandersonet , this is what you hit today