Additions, Improvements & Fixes #366
Conversation
Made significant changes in the menu option to set the cron schedule for automatic F/W Updates. 1) There are now 2 modes of operation to set the cron schedule: Guided & Custom. a) The "Menu-Guided Entry" is intended to help users select the MINUTES, HOUR and DAYS for the cron schedule. Useful for users who are not familiar with the cron syntax. b) The "Custom Input/Entry" is intended for users who already know the cron syntax and can enter the full "raw" schedule string without much help. 2) Added code to synchronize the cron schedule for automatic script updates with the schedule for automatic F/W updates. Currently, the check for automatic script updates will be performed daily but 15 minutes before the time when the cron for automatic F/W updates is scheduled. This ensures that the script is always be updated first before proceeding to check for automatic F/W updates. 3) Fixed code in the cron schedule that was allowing setting a cron job to run every minute and/or every hour of the day. Technically, this should not be allowed for the purposes of doing automatic F/W updates. 4) Various code improvements & fine-tuning.
|
Whenever you have a chance, please review and test the changes in this PR. Take care & have a good night, bud. |
Some minor code improvements & fine-tuning: - Modified the messaging when selecting to ENABLE the option for automatic script updates so that it does not appear as if it's a dangerous feature to activate. It's good to display a NOTICE about the option, but I think presenting it as a WARNING sends a misleading message that "bad" things can happen with updating the script automatically. Also, now the user is prompted to confirm the daily cron schedule on display to check for new versions. - Modified the color for ENABLED setting from red to magenta, for the same reasons stated above. - When ENABLED, the cron schedule setting for automatic script updates is shown below the menu option. This allows users to visually check & confirm when checks for new updates are made.
|
I had some time this morning while I waited for a firm decision from upper management about a few options I presented to implement for our final release of 2024 (deadline: Dec 18th). So I took the time to make the minor improvements that I previously mentioned I wanted to make.
Have a good day, bud. Talk to you in the evening. |
|
Wow you've been busy! Sorry for the delay it was a crazy day for me today between work and then I took a nap and then had to head out to get some groceries and errands done. All that to say I'm here! This all looks fantastic. I already have some ideas on things I want to test and validate with this solution. I'll test and report back. Also wanted to mention I'm testing and addressing some bugs and feature requests as well and will be submitting my own PR shortly. But I'll only submit it after this is merged to avoid conflicts. |
Yeah, I got 99% of the implementation & testing done yesterday, and I believe I put the final touches and fine-tuning today. Good thing I had some free time this morning at work before getting very busy for the next 10 days or so to finalize the library components I'm working on for our final release of the year by Dec-18th. That's why I wanted to get this PR done as soon as I could. Hopefully, the latest code will work well for your own testing. BTW, I just found a couple of bugs during my own testing this evening, and I'm able to duplicate them consistently. I'll write another message shortly with the details I'm gathering. |
Don't be stealing my bugs!!! :D ;) |
|
So far the testing is going well, will merge now and do final quick round of tests. Very nice work buddy around following the primary cron job!! I know we already did some work to calculate the next upcoming cron job, but decoding it to follow it slightly is a different bear I think :P |
|
One thing I noticed is these cron entries aren't detected as valid anymore when they were before:
Per per question 22 of our Wiki: |
MerlinAU.sh
Outdated
| printf "\nCurrent Schedule: ${GRNct}${scriptUpdateCronSched}${NOct}\n" | ||
| printf "[${GRNct}${cronSchedStrHR}${NOct}]\n" | ||
|
|
||
| if _WaitForYESorNO_ "\nConfirm daily check for automatic script updates?" |
There was a problem hiding this comment.
I would change the wording to this. I wasn't sure what it was asking me at first.
The "Proceed to enable" to me means it's being enabled, but really it seems to be the "Confirm daily check for automatic script updates?" that enables it. But it's asking that when it's showing the update schedule.
I would have it say: "Please confirm the above schedule for script updates looks correct"
There was a problem hiding this comment.
I would change the wording to this. I wasn't sure what it was asking me at first.
...The "Proceed to enable" to me means it's being enabled, but really it seems to be the "Confirm daily check for automatic script updates?" that enables it. But it's asking that when it's showing the update schedule.
I would have it say: "Please confirm the above schedule for script updates looks correct"
The intent of the 2nd prompt is for users to confirm that they want to enable the feature with a daily check using the cron schedule being displayed, giving them further context so they are more informed of the choice they're making. If they answer "NO" to the confirmation, it means they are canceling their initial request to enable.
I'll see if I can re-word the message to make it clearer.
There was a problem hiding this comment.
Another choice is remove my original "proceed to enable?" So it goes straight to your validation of the schedule to be enabled then, would help cause less confusion I think?
Any suggestions? A reword could work too, or we could remove one of the "confirm, double confirm" prompts.
There was a problem hiding this comment.
Another choice is remove my original "proceed to enable?" So it goes straight to your validation of the schedule to be enabled then, would help cause less confusion I think?
Any suggestions? A reword could work too, or we could remove one of the "confirm, double confirm" prompts.
Last night, I focused primarily on improving the validation error messages and moved them around to a single place for each case so any function calling for validation gets the same error messages. WRT to this message prompt, I decided to reword it for now. I suspect most users who are interested in having automatic script updates will simply set it once and then forget about it. I honestly like the prompt asking for confirmation based on the cron schedule that's displayed - it provides users with more info before committing to the decision. I suspect not everyone will want to have yet another cron job to update the script automatically.
|
While doing more testing around the latest cron schedule changes for F/W Updates, I found 2 bugs. Prerequisite:
And the same bunch of errors are repeated when going back to the Main Menu, before the menu header is displayed.
AFAICT, it looks like the current functions that calculate the next job run time don't handle any day intervals correctly. I'm not very familiar with that code so I'll let you take a look. You'll likely find the problem faster than I would. I'm going offline in about 5 minutes to have dinner so we'll talk later. |
Okay good, not the bugs I was looking at LOL! |
|
Just so your aware, the 2 bugs I found were again related to the post update emails detecting Gnuton versions as not flashed correctly. The reason is we remove the suffix specifically for thing like "ROG" or "tuf" but Gnuton always has a suffix in its version number "3004.388.8.2-Gnuton1" So the obvious fix is remove all suffixes, don't be picky that fix is lined up. And second bug is with the changelog verification, the regex is always failing to detect the current version in the changelog and it's a pretty simply fix I got already lined up. |
Yes, that's intentional. The new code does not allow any intervals for the minutes. They must be full numbers only. This prevents novice users from setting up cron schedules that are technically & syntactically correct in general but are not really valid for doing F/W Updates. Do people really want to check every 20 or 30 minutes all day long? P.S. The "boss" is calling me :>) We'll talk later... |
I think the use-case was mostly for remote users. That used to be how we recommended they update the router (schedule for 15 minutes ahead and logout of SSH so the terminal didn't close early and terminal the script) We can change the wiki, I think it still says that for the question 25 |
|
Also what does the code do if it's already set to let's say.. every half hour when this version is loaded? I'll review closer when I'm back from dinner. |
MerlinAU.sh
Outdated
| cronSchedStr="$(echo "$1" | awk -F ' ' '{print $1}')" | ||
| if ! _ValidateCronScheduleMINS_ "$cronSchedStr" | ||
| then | ||
| printf "${REDct}INVALID cron value for 'MINUTE' [$cronSchedStr].${NOct}\n" |
There was a problem hiding this comment.
I would clarify this, make the requirement clear that it needs to be a full minute value now
There was a problem hiding this comment.
I would clarify this, make the requirement clear that it needs to be a full minute value now
Good point. I'll review the messaging and modify it for more clarity.
There was a problem hiding this comment.
This is a change in behavior and if one thing my job as a workstation engineer taught me (even though it wasn't for long) is changes in behaviors need to be clearly communicated to users else they just go based on memory and get confused when it doesn't work like it used too. LOOL
I'm fine with documenting the changes to the cron behavior in the Wiki, but for users trying to set it live we should give them a bit of context here I think
There was a problem hiding this comment.
This is a change in behavior and if one thing my job as a workstation engineer taught me (even though it wasn't for long) is changes in behaviors need to be clearly communicated to users else they just go based on memory and get confused when it doesn't work like it used too. LOOL
I'm fine with documenting the changes to the cron behavior in the Wiki, but for users trying to set it live we should give them a bit of context here I think
When you get a chance, take a look at the new error messages getting generated when validation fails. They're more verbose and provide more details about the new restrictions on the cron schedule parameters.
Remember that the cron is intended for a very specific purpose so we don't want users to be able to set up a schedule that is counter-productive to do F/W Updates. For example, before we were allowing users to set a cron job to run "every minute in every hour" which would be the wrong thing to do. The intention now is to "guide" the users to make more reasonable choices, especially with MINUTE & HOUR parameters.
I think I got the fix. |
AFAIK, that scenario was resolved by adding the option to keep Tailscale & ZeroTier active when stopping all Entware services and by adding code to ignore the SIGHUP signal so that the script process can continue after the SSH session window is terminated. An alternative method would be to set the cron schedule with the HOUR & MINUTE parameters set to 15-20 minutes into the future. For example, if the current hour is 10:50 PM one can set the cron schedule to "
Yes, we'll need to document all these changes in the WIKI as well. |
Good question. Currently, upon initialization, the code simply creates the cron job using whatever schedule is stored in the configuration file - no validation is made. However, once users go to the CLI menu to change or simply check the cron job setting, the validation code will be triggered, checking for the new restrictions. |
Very true, it should technically be solved with that change! although I'm not sure how validated or tested that situation is, I think we had one user test, and ourselves. How successful it is in the wild hasn't been vetted much, that's why it's still mentioned in the Wiki.
I think this is an acceptable solution, again I'll document this in the Wiki and try to be clear around the new requirements and behaviors. I'll give an example similar to how you did here. |
Reworked the error messages when validating the cron schedule to provide users more information of the new restrictions to explain why some parameters fail to pass the check. Reworded confirmation prompt when enabling automatic script updates. The idea is to provide the user the schedule details of when the checks will be performed so they are more informed when making the choice. Some people may not want to enable if they don't agree with having a daily check.
|
Apologies for coming back online rather late yesterday. After dinner, my wife & I sat down to watch a TV show but I fell asleep in about 10 minutes and didn't wake up until about 1 hour later. Anyway, I just woke up to prepare for an early meeting this morning, but I wanted to submit my latest changes to the PR. Last night, I focused primarily on reworking the error messages generated when validating the cron schedule to provide users with as much information as possible on the screen and explain the new restrictions that may explain why some parameters are failing to pass the validation checks. When you get a chance, please take another look at the new error messages getting generated. These messages will likely be useful when documenting the changes on the Wiki page. |
|
Thanks Martinski! Will do a final review this morning after my work meeting and merge, then submit my PR with the requested fixes :) |
Take your time. I don't think the bugs found & fixed are very critical to the functionality. In fact, the 2 bugs I found have been there for quite a while, but I suspect users don't set up the cron schedule with any kind of day intervals; otherwise, we would heard about them by now on the SNBForums. Going offline now... Talk to you in the evening. |
I'll be online most of the day, meeting is just wrapped up. I'll review this and chat with you when you return buddy. Enjoy the day! |
|
Really liking this solution Martinski! |
Made significant changes in the menu option to set the cron schedule for automatic F/W Updates.
There are now 2 modes of operation to set the cron schedule: Guided & Custom.
a) The "Menu-Guided Entry" is intended to help users select the MINUTES, HOUR, and DAYS for the cron schedule. Useful for users who are not familiar with the cron scheduling syntax.
b) The "Custom Input/Entry" is intended for users who already know the cron scheduling syntax and can enter the full "raw" schedule string without much help.
Added code to synchronize the cron schedule for automatic script updates with the schedule for automatic F/W updates. Currently, the check for automatic script updates will be performed daily but 15 minutes before the time when the cron for automatic F/W updates is scheduled. This ensures that the script is always updated first before proceeding to check for automatic F/W updates.
Fixed code in the cron schedule that allowed setting a cron job to run every minute and/or every hour of the day. Technically, this should not be allowed for the purposes of doing automatic F/W updates.
Various code improvements & fine-tuning.