Better cron expressions #5
Conversation
Summary of ChangesHello @gfdsa, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves cron expression support by updating the validation regex, adding more comprehensive examples, and including new tests. The changes are a solid step forward. I've provided a few suggestions to further enhance the code. My main feedback points relate to the new regex pattern, which is a bit too permissive, and a minor formatting issue in a Go file. I've also suggested adding more test cases to explicitly document the regex's known limitations.
| type: string | ||
| description: "Schedule in Cron format" | ||
| pattern: "^(\\d+|\\*)(/\\d+)?(\\s+(\\d+|\\*)(/\\d+)?){4}$" | ||
| pattern: "^((TZ=|CRON_TZ=)[A-Za-z/_]+(\\s+))?(([0-9]+|\\*)([-,/]([0-9]+|\\*))*\\s+){4}([0-9]+|\\*)([-,/]([0-9]+|\\*))*$" |
There was a problem hiding this comment.
The new regex pattern is a great improvement and covers many more valid cron expressions. However, it's a bit too permissive and may accept some invalid formats that the underlying robfig/cron parser will reject. For example, a field like 1-2-3 would be accepted by this regex, but is not a valid cron field part. While the controller will eventually reject this, tightening the regex would provide users with earlier feedback from kubectl apply. The part ([-,/]([0-9]+|\*))* allows for repeated separators like - or / in a single field.
| type RestartScheduleSpec struct { | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:Pattern=`^((TZ=|CRON_TZ=)[A-Za-z/_]+(\s+))?(([0-9]+|\*)([-,/]([0-9]+|\*))*\s+){4}([0-9]+|\*)([-,/]([0-9]+|\*))*$` | ||
| Schedule string `json:"schedule"` |
There was a problem hiding this comment.
There are a couple of issues on these lines:
- Formatting: Line 24 is indented with a tab, which is inconsistent with Go's standard formatting (
gofmt). Thetypekeyword should not be indented, and fields within the struct should be indented consistently. - Regex Permissiveness: The regex on line 26, while a good improvement, is a bit too permissive. It will accept some invalid cron strings (e.g.,
1-2-3in a field), which the controller's cron parser will later reject. This is the same issue as in the CRD definition.
| type RestartScheduleSpec struct { | |
| // +kubebuilder:validation:Required | |
| // +kubebuilder:validation:Pattern=`^((TZ=|CRON_TZ=)[A-Za-z/_]+(\s+))?(([0-9]+|\*)([-,/]([0-9]+|\*))*\s+){4}([0-9]+|\*)([-,/]([0-9]+|\*))*$` | |
| Schedule string `json:"schedule"` | |
| type RestartScheduleSpec struct { | |
| // +kubebuilder:validation:Required | |
| // +kubebuilder:validation:Pattern=`^((TZ=|CRON_TZ=)[A-Za-z/_]+(\s+))?(([0-9]+|\*)([-,/]([0-9]+|\*))* \s+){4}([0-9]+|\*)([-,/]([0-9]+|\*))*$` | |
| Schedule string `json:"schedule"` |
| // Invalid expressions | ||
| { | ||
| name: "Missing fields", | ||
| input: "0 * * *", | ||
| should: false, | ||
| }, | ||
| { | ||
| name: "Too many fields", | ||
| input: "0 * * * * *", | ||
| should: false, | ||
| }, | ||
| { | ||
| name: "Invalid text", | ||
| input: "invalid cron expression", | ||
| should: false, | ||
| }, |
There was a problem hiding this comment.
It's great that you've added tests for the new regex. To make this test suite even more robust and to document the known limitations of the regex, consider adding test cases for cron strings that are structurally invalid but are currently passed by the regex. This will make it clear what the CRD validation allows vs. what the controller's parser will ultimately reject.
For example:
// ... inside tests slice
{
name: "Structurally invalid: multiple hyphens in one field",
input: "0 1-2-3 * * *",
should: true, // The regex currently accepts this, though cron parsers will not.
},
{
name: "Structurally invalid: multiple slashes in one field",
input: "*/2/3 * * * *",
should: true, // The regex currently accepts this, though cron parsers will not.
},These tests would pass with the current regex, correctly demonstrating its permissiveness and serving as documentation.
1 participant
No description provided.