Skip to content

Improvements to "Cron Job Schedule" validation & translation.#142

Merged
ExtremeFiretop merged 2 commits intoExtremeFiretop:devfrom
Martinski4GitHub:dev
Feb 23, 2024
Merged

Improvements to "Cron Job Schedule" validation & translation.#142
ExtremeFiretop merged 2 commits intoExtremeFiretop:devfrom
Martinski4GitHub:dev

Conversation

@Martinski4GitHub
Copy link
Collaborator

  • Added a validator for the "Cron Job Schedule" user input to try to prevent users from setting an invalid check schedule and then wondering why it didn't work. It's not an exhaustive validation but it flags the most common user errors.

  • Improve translation of the "Cron Job Schedule" string to English.
    Now it handles abbreviated short names for "month" values ("Jan", "Feb", "Mar", etc.) and for "day of the week" values ("Sun", "Mon", "Tue", etc.) and outputs the full names.

- Added a validator for the "Cron Job Schedule" user input to try to prevent users from setting an invalid check schedule and then wondering why it didn't work. It's not an exhaustive validation but it flags the most common user errors.

- Improve translation of "Cron Job Schedule" string to English.
Now it handles abbreviated short names for month ("Jan", "Feb", etc.) and day of the week ("Sun", "Mon", etc.).
@Martinski4GitHub
Copy link
Collaborator Author

@ExtremeFiretop,

OK, it's all yours now. Once you're happy that everything is working well and no issues are found, you can merge into the "main" branch and issue a new release.

@ExtremeFiretop ExtremeFiretop added the enhancement New feature or request label Feb 23, 2024
@ExtremeFiretop
Copy link
Owner

@ExtremeFiretop,

OK, it's all yours now. Once you're happy that everything is working well and no issues are found, you can merge into the "main" branch and issue a new release.

Starting my review now :)

@Martinski4GitHub
Copy link
Collaborator Author

@ExtremeFiretop,
OK, it's all yours now. Once you're happy that everything is working well and no issues are found, you can merge into the "main" branch and issue a new release.

Starting my review now :)

I'll be around checking for any problems reported. I was just informed that dinner will be ready in about 15 minutes so I'll take a break then.

## Modified by Martinski W. [2024-Feb-22] ##
##----------------------------------------##
readonly FW_Update_CRON_DefaultSchedule="0 0 * * 0"
readonly FW_Update_CRON_DefaultSchedule="0 0 * * Sun"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving changes to default values


# Special handling for "month" to map short abbreviations to long full names #
month_text="$(echo "$month_text" | tr [A-Z] [a-z])"
month_map1="jan:January feb:February mar:March apr:April may:May jun:June jul:July aug:August sep:September oct:October nov:November dec:December"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, yes I see, this is smart, having a map1 and map2, one for each depending what is used. (short form or number)

then
# Special handling for "day of the week" to map short abbreviations to long full names #
day_of_week_text="$(echo "$day_of_week_text" | tr [A-Z] [a-z])"
dow_map1="sun:Sunday mon:Monday tue:Tuesday wed:Wednesday thu:Thursday fri:Friday sat:Saturday"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this is smart, I see what you mean by exhaustive lol

readonly FW_Update_CRON_DefaultSchedule="0 0 * * Sun"

readonly CRON_MINS_RegEx="([*0-9]|[1-5][0-9])([\/,-]([0-9]|[1-5][0-9]))*"
readonly CRON_HOUR_RegEx="([*0-9]|1[0-9]|2[0-3])([\/,-]([0-9]|1[0-9]|2[0-3]))*"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved the newly added regular expressions

##-------------------------------------##
## Added by Martinski W. [2024-Feb-22] ##
##-------------------------------------##
_ValidateCronJobSchedule_()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so basically every input now is validated against these new regular expressions.
This addition really enhances the robustness of the cron section of the script by preventing incorrect cron schedule entries :) I like it!


readonly CRON_DAYofWEEK_NAMES="(Sun|Mon|Tue|Wed|Thu|Fri|Sat)"
readonly CRON_DAYofWEEK_RegEx="$CRON_DAYofWEEK_NAMES([\/,-]$CRON_DAYofWEEK_NAMES)*|[0-6*]([\/,-][0-6])*"
readonly CRON_DAYofWEEK_RegEx="$CRON_DAYofWEEK_NAMES([\/,-]$CRON_DAYofWEEK_NAMES)*|[*0-6]([\/,-][0-6])*"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved the newly added regular expressions

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Feb 23, 2024

Okay all changes understood and approved so far!. Running some tests now against the changes!

printf "${REDct}INVALID cron schedule string [$1]. Incorrect number of parameters.${NOct}\n"
return 1
fi
cronSchedsStr="$(echo "$1" | awk -F ' ' '{print $1}')"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exhaustive validation alright lol!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exhaustive validation alright lol!

Well, it's not quite an exhaustive check & validation; it's more like a syntax parser validator so it will flag the most common user errors WRT syntax, but a user can still set a "syntactically correct" schedule that it will not work like:

10 3 31 2 0
10 3 31 Feb Sun

The above entries are obviously wrong and will never actually work. Adding checks & validation for semantic errors is much more complicated and would require dozens of small functions for each possible case. Not something I'm willing to spend time on just for this one user input. My take is that users should make some effort to learn how to set a semantically valid cron schedule and take responsibility for making such errors.

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Feb 23, 2024

So far all the tests pass perfect!

Here is a complete list of valid crons I've tested to try and break the validation without success:

0 0 1 1 * 
0 0 1 Jan *
0 0 1 * Sun
*/5 0 29 Feb *
0 22 * * Fri
59 23 31 Dec *
0 0 * * */2
0 4 1 Jan,Feb,Mar * 
15,30,45 * 10 * Mon-Fri

A small list of some invalid crons below I tried to use to trick the validation without success:

0 12 25 Dec ? 
0 0 L * *
0 0 * * 5L
0 0 * * 3#2
* * * * Jan-Feb
* 60 0 * * *
* 24:00 * * * *
* * 25 * * *
* 0 0 -1 * *
* 0 0 * 13 *
* */61 * * * *
* 1-60 * * * *
* 0 0 32 * *
* * 0 0 * * sun-mon
* * * * * Jan-Feb

@ExtremeFiretop ExtremeFiretop merged commit 711c05f into ExtremeFiretop:dev Feb 23, 2024
@Martinski4GitHub
Copy link
Collaborator Author

So far all the tests pass perfect!

Here is a complete list of valid crons I've tested to try and break the validation without success::

0 0 1 1 * 
0 0 1 Jan *
0 0 1 * Sun
*/5 0 29 Feb *
0 22 * * Fri
59 23 31 Dec *
0 0 * * */2
0 4 1 Jan,Feb,Mar * 
15,30,45 * 10 * Mon-Fri

A small list of some invalid crons below I tried to use to trick the validation without success:

0 12 25 Dec ? 
0 0 L * *
0 0 * * 5L
0 0 * * 3#2
* * * * Jan-Feb
* 60 0 * * *
* 24:00 * * * *
* * 25 * * *
* 0 0 -1 * *
* 0 0 * 13 *
* */61 * * * *
* 1-60 * * * *
* 0 0 32 * *
* * 0 0 * * sun-mon
* * * * * Jan-Feb

Excellent!! I went through a similar long list of valid & invalid entries to verify functionality and my final test results went perfectly well. Thank you for testing & verifying the code.

@Martinski4GitHub
Copy link
Collaborator Author

OK, I'm being summoned by a "higher power." :>) Be back in 30 or so...

Repository owner deleted a comment from Martinski4GitHub Feb 23, 2024
@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Feb 23, 2024

OK, I'm being summoned by a "higher power." :>) Be back in 30 or so...

Enjoy dinner!
Your right on the crons, Idk what I was thinking, I thought it was case sensitive but it clearly isn't when you do a cru l in the system :) (That's embarrassing!)

Ready to release when your back!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants