ExtremeFiretop / MerlinAutoUpdate-Router

Merlin(A)uto(U)pdate is a Merlin router script which allows you to remotely identify a stable firmware update for an ASUS Merlin router, and automatically download and update via an unattended method directly from the router.
https://www.snbforums.com/threads/merlinau-v1-2-7-the-ultimate-firmware-auto-updater-amtm-addon.91326/
GNU General Public License v3.0
21 stars 1 forks source link

Include "ManualUpdate" switch and Other Small Corrections v2 #275

Closed ExtremeFiretop closed 3 months ago

ExtremeFiretop commented 3 months ago

Include "ManualUpdate" switch and Other Small Corrections v2 (Accidently closed the previous one by merging it)

This change includes the following:

Improved Messaging around Tailscale Setting Improved Messaging around ZIP Path Setting Reworded Cron Schedule Setting Include the requested feature to allow manual updates of a specific file as requested here: https://www.snbforums.com/threads/merlinau-v1-2-6-the-ultimate-firmware-auto-updater-now-available-in-amtm.88577/post-915243 Considering this will be a "advanced" user option with less safeties involved, this will be a switch only and no menu option. Screenshots of the new flow below:

File name/version Auto-detected: image

File name/version detection failed: image

ExtremeFiretop commented 3 months ago

@ExtremeFiretop,

I have not yet had a chance to review the latest changes in this PR (I was busy this evening rebuilding the main drive from my wife's laptop & reinstalling all her apps from scratch - it's all good now). But from the SNB Forums post you referenced asking for the "Manual Update" feature, I still have the same concerns you listed in your reply about allowing the user to do a "manual update" using a F/W ZIP file that was previously downloaded locally.

My main concern is that the local ZIP file containing the F/W image can be of unknown origin, from an unknown source, even a Beta or Alpha build, and this opens a potential "can of worms" where the router can be accidentally bricked by trying to flash an image that may or may not have been "vetted & blessed" by RMerlin. Currently, our script downloads the image directly from RMerlin's website so we are extremely confident that the image is "good" and we are now verifying its authenticity & integrity by checking the SHA256 signature against RMerlin's official firmware signature.

Now, from a developer's point of view, I can see where this "manual update" feature would be useful if you wanted to build your own F/W images and then flash them into the target router. But this is essentially a "backdoor" for development purposes only; I would not want normal, regular users (many of whom lack the necessary technical skills to know better) to have the ability to flash a potentially unknown & unvetted F/W image file using our script, and if something goes wrong, blame our script for unforeseen consequences.

IMO, if a user really wants to flash a F/W image that can potentially be from an "unknown" source, they can always use the built-in webGUI. I understand that the OP from the SNBForum post was using TailScale to connect remotely and his concern was that the connection was terminated before the F/W flash was fully completed. I think this has been already addressed in our latest script release (BTW, I'll submit a PR that should improve this feature, based on our recent discussions in PR #271).

I am very much in a similar frame of mind. However, I feel the need to advocate for the user, and work on a solution to display to you, as he even messaged me again on the side lines via PM to discuss his use-case to remotely flash betas "more safely"

Now he did mention that he does not want this feature being abused, and a I quote below:

The folks who will use this will be advanced users want to use it to ensure a remote Router will "take" the FW without a disruption. If beta or alphas bork their router, tough, they knew the risks; it was not MerlinAUs fault.

I was thinking about this the other day and was postulating whether on FIRST run you tell them a messaging about: You have been warned. And sell it for Betas only ... the risk-takers and propellor heads will use it for Alphas anyway.

Now I'm thinking, we don't need to sell it for betas only, we just do not sell it at all. Someone smart enough might know to check the help commands, and find the command.

Or we don't even include it in the help like it is now, and we say not a word, but include it for people like you or me from the development standpoint, or a user like him trying to do the betas on remote routers as safely as possible and benefit still from the advanced features MerlinAU brings (such as shutting down services, unmount the USB, sha256 check, etc).

This is the summery of the discussion in TL;DR: image

jksmurf commented 3 months ago

I still think it’s a case of caveat emptor.

Everything we do to our routers outside of stock, including Asus-Merlin, amtm and all the Addons it facilitates, are outwith the boundaries of the Asus model, which doesn’t work a lot of the time anyway. So you’re selling to a captive audience who by and large know what they are doing.

The only thing you might be buying is additional headaches in supporting it, but at no time should you ever feel responsible. That is my 2c. I think a menu option with sufficient “are you sure” warnings is enough.

The recent round of people updating 388.7 to 388.8 and not having internet nor access to their routers strongly suggests to me that this is no worse; and from what I have seen of the safeguards you folks are building in, you care. You care that it works as well as it should but at the end of the day there’s always a risk. Nobody died. Thanks for all you do.

Martinski4GitHub commented 3 months ago

I still think it’s a case of caveat emptor.

Everything we do to our routers outside of stock, including Asus-Merlin, amtm and all the Addons it facilitates, are outwith the boundaries of the Asus model, which doesn’t work a lot of the time anyway. So you’re selling to a captive audience who by and large know what they are doing.

The only thing you might be buying is additional headaches in supporting it, but at no time should you ever feel responsible. That is my 2c. I think a menu option with sufficient “are you sure” warnings is enough.

The recent round of people updating 388.7 to 388.8 and not having internet nor access to their routers strongly suggests to me that this is no worse; and from what I have seen of the safeguards you folks are building in, you care. You care that it works as well as it should but at the end of the day there’s always a risk. Nobody died. Thanks for all you do.

Can you elaborate on the reason(s) for not using the built-in webGUI to flash non-production or previous production versions of the F/W on the router?

Essentially, the webGUI allows you to flash any F/W image built for the specific router model: alphas, betas, the latest as well as previous production versions (in case you want to revert to a previous release that works better). Yes, the webGUI does not do the SHA256 signature verification, but everything else is done "behind the scenes" to ensure a safe F/W flash once the F/W image is fully downloaded & locally available on the router.

Frankly, I’m not seeing the benefits or the added value of trying to flash non-production F/W images with the MerlinAU script.

You said this is a feature for "advanced users" but once such a feature is released and available in the wild, potentially anyone can use it, and we have no way to prevent "novice users" from trying it. And yes, at some point, someone with very little technical know-how will try to use it and when things go wrong, they’ll blame it on the script tool.

Based on many posts that I’ve read on the SNB Forums over the years, there are many users who are not aware that they don’t know enough to realize that they are still "novice users" but believe themselves to be "advanced users who know what they’re doing."

My current position is that the MerlinAU script should stay focused on flashing the latest production-level F/W releases only, and we should let the built-in webGUI handle alphas, betas, previous releases, and everything else in between.

Yes, there are always potential risks involved with flashing F/W, but allowing MerlinAU to handle only production-level releases minimizes the risks and our exposure to being "blamed" for bricking a router.

My 2 cents.

jksmurf commented 3 months ago

Thank you very much for the comments; all understood.

The reason I think that MerlinAU would work better vs the WebGUI is primarily because when I select the file to upload, I do so locally, but the file is then sent over the VPN to the remote router, so that is a concern; especially if the VPN carks out. The risk appears greater.

I’m not sure if it’s possible to WinSCP the file to the remote router then WebGui upload from there, if so then yes, that might be a solution, but still lacks the ease of use and checks made by MerlinAU.

My thinking was that MerlinAU has the structures and framework in place to upload and update FW, restart the router with a local file. This is my reasoning. I trust that clarifies. Please understand I’m coming from a position as end user without anywhere near the level of technical expertise as yourself and ExtremeFiretop.

k.

ExtremeFiretop commented 3 months ago

I think this discussion is very productive, and helps clear the air on different viewpoints. Now just staying neutral for now and playing devils advocate, heres a proposed solution?

Implement a strict warning and "Opt-In" system

Make the feature an opt-in hidden command, not visible in the main menu or help commands. Implement the feature but include a clear and strong warning about the risks. Require users to acknowledge the warning before they can proceed.

Basically I'm thinking of a way to Implement the feature but keep it disabled by default. Developers and advanced users can enable it through a configuration file or environment variable.

This approach ensures regular users do not accidentally stumble upon it? Based on the arguments presented, I see a course of action could potentially be to implement the feature with strong safeguards in place. (If that is possible).

This includes strict warnings, making it an opt-in hidden feature. This approach allows advanced users to benefit from the feature while minimizing risks and support issues.

Thoughts? If your still aren't comfortable with the idea @Martinski4GitHub I am ready to drop and close the PR. To be fair your initial thoughts is exactly what I posted in the forums, I fully agree that the risks are greater here with unvetted firmware.

Again this was mostly to discuss the considerations of both views. I do see the argument made about everything being done is all outside of asus model, but it is not outside of the "Merlin support model", as we have been very active at making sure that features we implement stay within his approved and supported "model"

Martinski4GitHub commented 3 months ago

Thank you very much for the comments; all understood.

The reason I think that MerlinAU would work better vs the WebGUI is primarily because when I select the file to upload, I do so locally, but the file is then sent over the VPN to the remote router, so that is a concern; especially if the VPN carks out. The risk appears greater.

Note that when using the built-in WebGUI to flash the F/W, the process does not actually start until the complete image file is received & stored on the router's filesystem. Also, RMerlin has stated many times that an image file integrity & a router model check are performed before flashing can start (to avoid the situation where a user has selected the wrong file for the router model). This means that if you have a flaky connection to a remote router, and the image file is corrupted in transit, the file integrity check will fail and the process is then canceled automatically. So the risk of bricking the router with a "bad" image file due to a flaky connection is essentially nil. IOW, the risk you're worrying about is extremely low, highly unlikely, and certainly not greater.

I’m not sure if it’s possible to WinSCP the file to the remote router then WebGui upload from there, if so then yes, that might be a solution, but still lacks the ease of use and checks made by MerlinAU.

Currently, the "Upload" button from the WebGUI searches for files on the connected PC's filesystem, not from the router's filesystem so that's not possible, AFAIK.

ExtremeFiretop commented 3 months ago

Currently, the "Upload" button from the WebGUI searches for files on the connected PC's filesystem, not from the router's filesystem so that's not possible, AFAIK.

You know correctly. There is no way for the WebUI to use local files on the router.

Martinski4GitHub commented 3 months ago

I think this discussion is very productive, and helps clear the air on different viewpoints. Now just staying neutral for now and playing devils advocate, heres a proposed solution?

Implement a strict warning and "Opt-In" system

Make the feature an opt-in hidden command, not visible in the main menu or help commands. Implement the feature but include a clear and strong warning about the risks. Require users to acknowledge the warning before they can proceed.

Basically I'm thinking of a way to Implement the feature but keep it disabled by default. Developers and advanced users can enable it through a configuration file or environment variable.

This approach ensures regular users do not accidentally stumble upon it? Based on the arguments presented, I see a course of action could potentially be to implement the feature with strong safeguards in place. (If that is possible).

This includes strict warnings, making it an opt-in hidden feature. This approach allows advanced users to benefit from the feature while minimizing risks and support issues.

Thoughts? If your still aren't comfortable with the idea @Martinski4GitHub I am ready to drop and close the PR. To be fair your initial thoughts is exactly what I posted in the forums, I fully agree that the risks are greater here with unvetted firmware.

Again this was mostly to discuss the considerations of both views. I do see the argument made about everything being done is all outside of asus model, but it is not outside of the "Merlin support model", as we have been very active at making sure that features we implement stay within his approved and supported "model"

Let me sleep on your proposal and will respond tomorrow. I think I got the gist of it and saw some possibilities, although I didn't understand your last paragraph. My brain is starting to feel like it's running on fumes. It was a very long workday for me and I'm pretty much ready to go to bed and have a long, good sleep.

Talk to you tomorrow evening.

ExtremeFiretop commented 3 months ago

I won't lie, because I'm building nightly imagines right now, I didn't see this as a bad idea because I also saw a way I could benefit from this.

That's why I was willing advocate for it as a "hidden" feature. But I understand the risks involved which is why I wouldn't ever want this to be front and center.

jksmurf commented 3 months ago

Note that when using the built-in WebGUI to flash the F/W, the process does not actually start until the complete image file is received & stored on the router's filesystem. Also, RMerlin has stated many times that an image file integrity & a router model check are performed before flashing can start (to avoid the situation where a user has selected the wrong file for the router model). This means that if you have a flaky connection to a remote router, and the image file is corrupted in transit, the file integrity check will fail and the process is then canceled automatically. So the risk of bricking the router with a "bad" image file due to a flaky connection is essentially nil. IOW, the risk you're worrying about is extremely low, highly unlikely, and certainly not greater.

Hi and thanks again for giving it some further thought, although I see it not having a great deal of support, which is totally understood. It's probably introducing big headaches for small gain, however you limit the risk and user-base.

So, fair enough, I guess that gives me some comfort and food for thought, for the occasions where I wanted to udpate a FW with mostly a beta fix (and sometimes alpha fix, the current issue with 388.8 and VPNs is a case in point) to trust my VPN and just keep using the WebGUI.

You raised valid concerns about non-production FW, but my original thoughts on this developed from a need (desire?) to update a remote router with am AsusMerlin Beta FW that IIRC had a feature or even a fix I thought was necessary. So if we are talking only about concerns regarding self-compiled FW or non-sanctioned (non Merlin, non Stock Asus) etc would there be any mechanism to limit a manual update only to FW that contain a specific signature (if that mechanism exists) that denotes the FW is sanctioned by RMerlin (including his Betas and Alphas), or by ASUS?

Currently, the "Upload" button from the WebGUI searches for files on the connected PC's filesystem, not from the router's filesystem so that's not possible, AFAIK.

Ah, OK, I was thinking you could create some sort of share that you select on the remote router (or USB-SSD), having uploaded the file into that share via e.g. WInSCP. Was just a thought, but if that's not technically possible, no problem. All good.

ExtremeFiretop commented 3 months ago

So if we are talking only about concerns regarding self-compiled FW or non-sanctioned (non Merlin, non Stock Asus) etc would there be any mechanism to limit a manual update only to FW that contain a specific signature (if that mechanism exists) that denotes the FW is sanctioned by RMerlin (including his Betas and Alphas), or by ASUS?

Let me grab this one... This is a more complicated question than you might think.

The short answer is yes, off the top of my head, there is specific markers we can use to identify RMerlin firmware.

However, due to the open nature of the firmware (and this project), basically any check we put in can easily be faked. It would become a bit of apple ve jailbreaks wack-a-mole situation where I'm not really interested in constantly finding a way to block unsanctioned firmware.

Security by obscurity doesn't work very well on open source projects unfortunately. Anyone smart enough can easily add a file, craft a commit to bypass our checks and now we face the exact same problems again from a bad actor and unvetted malicious firmware.

Ah, OK, I was thinking you could create some sort of share that you select on the remote router (or USB-SSD), having uploaded the file into that share via e.g. WInSCP. Was just a thought, but if that's not technically possible, no problem. All good.

We upload the file to the WebUI locally, but by crafting a very specific custom web request, which essentially tricks the router into thinking the request is coming from a local PC.

Is that sense it's possible, but not by using the actual webUI, by crafting a packet which mimicks what the WebUI would usually receive.

jksmurf commented 3 months ago

Thank you for the explanation all good, I sort of expected the response about folks being able to circumvent it! Also on the WebGUI trick :-).

Martinski4GitHub commented 3 months ago

So if we are talking only about concerns regarding self-compiled FW or non-sanctioned (non Merlin, non Stock Asus) etc would there be any mechanism to limit a manual update only to FW that contain a specific signature (if that mechanism exists) that denotes the FW is sanctioned by RMerlin (including his Betas and Alphas), or by ASUS?

The technology & mechanism to sign & authenticate any software package to make sure the original file has not been tampered with currently exist: digital signatures using public key certificates. The same can be used for any document, email, etc.

Essentially, the entity issuing the original file creates a unique digital signature using its own private key that certifies and timestamps the file. The signature payload is embedded in the file itself. The entity then makes the corresponding public key available so anyone wanting to verify the authenticity & integrity of the file can do so with publicly available tools. This method is pretty much foolproof (so far) as long as you use very strong encryption methods and keep the private key completely & absolutely secured & air-gapped so that only a few selected & trusted individuals have access to it.

However, I doubt that ASUS & RMerlin are willing to spend the additional time & resources needed to put this mechanism in place, especially given the number of F/W images that are issued in each release cycle for all router models. The process would require a small team dedicated to implementing, maintaining, and then applying the tools that create the digital signature for every single F/W image file before it gets packaged & released "into the wild."

ExtremeFiretop commented 3 months ago

Would be interesting to see, if a hypothetical digital signature was used for such a project for each firmware release.

I mean obviously once the infrastructure is setup, using it is easy for each firmware release by RMerlin. Signing an email with a digital signature isn't hard for example, It's setting up the infrastructure to allow for that signature to be used and read which is complex.

At my work we use digital signatures for the code we send out in the department, I can pump out 20 versions easy with the same signature, but setting that up is complex to have that signature be trusted by all endpoints requires infrastructure as Martinski mentioned.

Entrust is an example of a commercial example of this digital signature solution. As Martinski said I doubt anyone would spend time or money on such a solution.

If that changes then we would need to seriously revise a few things anyways. Short of that, everything I said is with today's solutions. 😉 I'm not interested in looking for specific markers in RMerlin firmware just to keep having to update the method the second it no longer works, or someone fakes it.

jksmurf commented 3 months ago

I’m sorry guys, I really regret asking the question about a digital signature in the FW to avoid non-approved variants.

If we’re going to entertain this manual upload, the advantages of which seem less apparent than I thought, it will need to be ex-main menu with warnings or not at all ie folks can just use the WeGUI approach discussed above.

ExtremeFiretop commented 3 months ago

I’m sorry guys, I really regret asking the question about a digital signature in the FW to avoid non-approved variants.

If we’re going to entertain this manual upload, the advantages of which seem less apparent than I thought, it will need to be ex-main menu with warnings or not at all ie folks can just use the WeGUI approach discussed above.

No need to apologize, we are just running through the examples of the limitations with your suggestions. 😉

If RMerlin is convinced to start using digital signatures when he pumps out firmware releases, then we can consider what your asking, but using today's solutions, nothing we could do would be "sufficient" so to speak.

I do believe the only way to safely implement it would be to basically hide it from the available options, as you mentioned. It becomes a developer feature. "If you know, you know."

It's not trying to block anything, as much as limit unintentionally use.

Martinski4GitHub commented 3 months ago

INFO WAS MODIFIED.

jksmurf commented 3 months ago

How does the above sound

How does it sound? Perfect. Absolutely perfect.

If you "know", you set it once in the required config, you upload your FW file to the remote router or router's USB, you run the script from the CLI only, you acknowledge the warnings, MerlinAU does the FW flash, done.

My thanks to you both for considering this, my apologies again if it caused (will cause) a few headaches for you, but I genuinely think it will be a very, very nice (albeit hidden) addition to a great Add-on.

ExtremeFiretop commented 3 months ago

I think this discussion is very productive, and helps clear the air on different viewpoints. Now just staying neutral for now and playing devils advocate, heres a proposed solution? Implement a strict warning and "Opt-In" system Make the feature an opt-in hidden command, not visible in the main menu or help commands. Implement the feature but include a clear and strong warning about the risks. Require users to acknowledge the warning before they can proceed. Basically I'm thinking of a way to Implement the feature but keep it disabled by default. Developers and advanced users can enable it through a configuration file or environment variable. This approach ensures regular users do not accidentally stumble upon it? Based on the arguments presented, I see a course of action could potentially be to implement the feature with strong safeguards in place. (If that is possible). This includes strict warnings, making it an opt-in hidden feature. This approach allows advanced users to benefit from the feature while minimizing risks and support issues. Thoughts? If your still aren't comfortable with the idea @Martinski4GitHub I am ready to drop and close the PR. To be fair your initial thoughts is exactly what I posted in the forums, I fully agree that the risks are greater here with unvetted firmware. Again this was mostly to discuss the considerations of both views. I do see the argument made about everything being done is all outside of asus model, but it is not outside of the "Merlin support model", as we have been very active at making sure that features we implement stay within his approved and supported "model"

I think your proposal to make the feature inaccessible to casual, regular users would be the way to go. If someone really wants to use such a feature, he must "jump through some hoops" every time he runs the script with some specific argument(s). Here's a proposed summary combining your ideas & mine:

INFO WAS MODIFIED

How does the above sound? Further ideas & comments are welcome, of course.

I think all of this is a fair compromise. If @jksmurf is happy with it, I am as well. 😜

Since it could be setup on my node for testing manual builds for example.

it would require a bit more work to get ready though. In this PR now, I have your points; 1, 3 and 6 already considered to some degree, we would be missing points 2, 4 and 5.

Was there a specific way you wanted to deal with point 7? Other than making it interactive only?

Martinski4GitHub commented 3 months ago

How does the above sound? Further ideas & comments are welcome, of course.

I think all of this is a fair compromise. If @jksmurf is happy with it, I am as well. 😜

Since it could be setup on my node for testing manual builds for example.

it would require a bit more work to get ready though. In this PR now, I have your points; 1, 3 and 6 already considered to some degree, we would be missing points 2, 4 and 5.

Was there a specific way you wanted to deal with point 7? Other than making it interactive only?

Point 7 was explicitly mentioned just to get the point across that automating the feature in any way would not be an acceptable compromise, IMO.

ExtremeFiretop commented 3 months ago

How does the above sound? Further ideas & comments are welcome, of course.

I think all of this is a fair compromise. If @jksmurf is happy with it, I am as well. 😜 Since it could be setup on my node for testing manual builds for example. it would require a bit more work to get ready though. In this PR now, I have your points; 1, 3 and 6 already considered to some degree, we would be missing points 2, 4 and 5. Was there a specific way you wanted to deal with point 7? Other than making it interactive only?

Point 7 was explicitly mentioned just to get the point across that automating the feature in any way would not be an acceptable compromise, IMO.

Fully agreed on that front!! 💪 It's pretty much the end of the night for me but I'll look into this tomorrow 😉

Have a goodnight folks. We will pickup the discussion tomorrow once the solution is touched up some more

Martinski4GitHub commented 3 months ago

How does the above sound? Further ideas & comments are welcome, of course.

I think all of this is a fair compromise. If @jksmurf is happy with it, I am as well. 😜 Since it could be setup on my node for testing manual builds for example. it would require a bit more work to get ready though. In this PR now, I have your points; 1, 3 and 6 already considered to some degree, we would be missing points 2, 4 and 5. Was there a specific way you wanted to deal with point 7? Other than making it interactive only?

Point 7 was explicitly mentioned just to get the point across that automating the feature in any way would not be an acceptable compromise, IMO.

Fully agreed on that front!! 💪 It's pretty much the end of the night for me but I'll look into this tomorrow 😉

Have a goodnight folks. We will pickup the discussion tomorrow once the solution is touched up some more

INFO WAS MODIFIED,

Have a good night's sleep, bud.

ExtremeFiretop commented 3 months ago

Added commit: https://github.com/ExtremeFiretop/MerlinAutoUpdate-Router/pull/275/commits/c99afde108c70733aebb27bef924b9d58b244569

INFO WAS MODIFIED.

ExtremeFiretop commented 3 months ago

Final time to discuss before we merge or close this. Any last concerns? Any other considerations? Any adjustments?

If we aren't happy with the solution we can still close it out, otherwise it seems to work for me :)

jksmurf commented 3 months ago

Looking good but two questions from me:

INFO WAS MODIFIED.

Second (excuse my ignorance here) is just how come you don’t need the “-“ in your switch in the CLI command ?

ExtremeFiretop commented 3 months ago

INFO WAS MODIFIED.

Correct, as per the third screenshot.

Second (excuse my ignorance here) is just how come you don’t need the “-“ in your switch in the CLI command ?

We are following the set standard for other addons. Such as:

/jffs/scripts/spdmerlin develop
/jffs/scripts/spdmerlin forceupdate
/jffs/scripts/connmon develop
/jffs/scripts/connmon forceupdate
/jffs/scripts/ntpmerlin develop
/jffs/scripts/ntpmerlin forceupdate
/jffs/scripts/YazDHCP develop
/jffs/scripts/YazDHCP forceupdate
jksmurf commented 3 months ago

OK, no problem. So:

INFO WAS MODIFIED.

We are following the set standard for other addons.

OK, no worries, just curious.

ExtremeFiretop commented 3 months ago

INFO WAS MODIFIED.

We are following the set standard for other addons.

OK, no worries, just curious.

Correct, everything you said is how I pictured Martinskis instructions and how the current flow to enable this feature works.

ExtremeFiretop commented 3 months ago

And to clarify, I believe the extra config file being created and populated manually the one time (one off manual step) Is again to limit unintentional use from someone not fully understanding what they are doing.

Someone can quickly browse all the switches in the code to find the offlineupdate switch, but the rest of the requirements aren't so clear. Also word can go around about the switch alone, but the switch alone is now not enough without the correctly populated config file to enable it.

Martinski4GitHub commented 3 months ago

Final time to discuss before we merge or close this. Any last concerns? Any other considerations? Any adjustments?

If we aren't happy with the solution we can still close it out, otherwise it seems to work for me :)

Very good job, bud! Well done!!

Martinski4GitHub commented 3 months ago

Second (excuse my ignorance here) is just how come you don’t need the “-“ in your switch in the CLI command ?

We are following the set standard for other addons. Such as:

/jffs/scripts/spdmerlin develop
/jffs/scripts/spdmerlin forceupdate
/jffs/scripts/connmon develop
/jffs/scripts/connmon forceupdate
/jffs/scripts/ntpmerlin develop
/jffs/scripts/ntpmerlin forceupdate
/jffs/scripts/YazDHCP develop
/jffs/scripts/YazDHCP forceupdate

Actually, given that this new feature is in fact "non-standard" in terms of being "for developers & advanced users only," and is not officially documented or supported for regular users, I propose that it gets reflected in the required argument by having a hyphen: -offlineupdate

ExtremeFiretop commented 3 months ago

Second (excuse my ignorance here) is just how come you don’t need the “-“ in your switch in the CLI command ?

We are following the set standard for other addons. Such as:

/jffs/scripts/spdmerlin develop
/jffs/scripts/spdmerlin forceupdate
/jffs/scripts/connmon develop
/jffs/scripts/connmon forceupdate
/jffs/scripts/ntpmerlin develop
/jffs/scripts/ntpmerlin forceupdate
/jffs/scripts/YazDHCP develop
/jffs/scripts/YazDHCP forceupdate

Actually, given that this new feature is in fact "non-standard" in terms of being "for developers & advanced users only," and is not officially documented or supported for regular users, I propose that it gets reflected in the required argument by having a hyphen: -offlineupdate

Are you saying you want a hyphen for this single feature only?

Martinski4GitHub commented 3 months ago

Actually, given that this new feature is in fact "non-standard" in terms of being "for developers & advanced users only," and is not officially documented or supported for regular users, I propose that it gets reflected in the required argument by having a hyphen: -offlineupdate

Are you saying you want a hyphen for this single feature only?

Correct. Again, to reflect that it's a "non-standard" feature & inaccessible unless "you're in the know."

ExtremeFiretop commented 3 months ago

Well, I did say it was the last chance to speak up before this got merged in, so I'd feel I'd be doing everyone a disservice if I didn't even follow my own advice.

I personally think that looks a bit "gross" (for lack of a better word) to have a difference in the method we call the specific actions. But I guess for the sake of safety it makes sense.

I understand the thinking, it's just not what I'm accustomed too myself at work. I guess we should prioritize the safety over the looks though 😂

ExtremeFiretop commented 3 months ago

Updated in commit: https://github.com/ExtremeFiretop/MerlinAutoUpdate-Router/pull/275/commits/d436daccc45bc10bcffeacad54aade3cbf8718ed

Martinski4GitHub commented 3 months ago

Well, I did say it was the last chance to speak up before this got merged in, so I'd feel I'd be doing everyone a disservice if I didn't even follow my own advice.

I personally think that looks a bit "gross" (for lack of a better word) to have a difference in the method we call the specific actions. But I guess for the sake of safety it makes sense.

I understand the thinking, it's just not what I'm accustomed too myself at work. I guess we should prioritize the safety over the looks though 😂

Sorry for not responding sooner. Last night, after my last message, my wife & I decided to watch a movie and after it ended we got very sleepy and just went to bed. I just woke up to go to the bathroom so this is just to let you know that I'm going to review the PR code this evening and run it through the Linter tool. From my first look at it, I think it will pass the Linter checks without much problem.

ExtremeFiretop commented 3 months ago

Well, I did say it was the last chance to speak up before this got merged in, so I'd feel I'd be doing everyone a disservice if I didn't even follow my own advice. I personally think that looks a bit "gross" (for lack of a better word) to have a difference in the method we call the specific actions. But I guess for the sake of safety it makes sense. I understand the thinking, it's just not what I'm accustomed too myself at work. I guess we should prioritize the safety over the looks though 😂

Sorry for not responding sooner. Last night, after my last message, my wife & I decided to watch a movie and after it ended we got very sleepy and just went to bed. I just woke up to go to the bathroom so this is just to let you know that I'm going to review the PR code this evening and run it through the Linter tool. From my first look at it, I think it will pass the Linter checks without much problem.

No worries buddy! It's like 5:30am your time, get back to bed!

Message us when you REALLY wake up 😉 If I've learned anything about my girlfriend it's that you gotta back in the bed sooner rather than later if you want to avoid the death glare.

Martinski4GitHub commented 3 months ago

@ExtremeFiretop,

The Linter tool checks were mostly fine, but I just found a few issues when doing testing & verification. I'll merge the PR now and then submit a separate PR to address the issues found. Give me about one hour or so.

ExtremeFiretop commented 3 months ago

I'll get to syncing Gnuton once your PR comes in :)

Was just touching up RTRMON for Viktor, take your time.

Martinski4GitHub commented 3 months ago

Well, I did say it was the last chance to speak up before this got merged in, so I'd feel I'd be doing everyone a disservice if I didn't even follow my own advice.

I personally think that looks a bit "gross" (for lack of a better word) to have a difference in the method we call the specific actions. But I guess for the sake of safety it makes sense.

I understand the thinking, it's just not what I'm accustomed too myself at work. I guess we should prioritize the safety over the looks though 😂

In the current implementation, to invoke the new "Offline Update" feature it requires a CLI parameter to start with.

INFO WAS MODIFIED

Any thoughts or comments?

BTW, about 70%-75% of the necessary code is already there, so it would not be a big effort at all to change the behavior as described above (about one hour or so plus testing & verification).

ExtremeFiretop commented 3 months ago

I think it's a great idea and I bet @jksmurf would approve as well if he had a way to call it from the Main Menu.

Question, is there a specific reason we couldn't keep the CLI parameter? I still think having a difference in the method we call those CLI parameters a bit "ugh".

But I'll live with it. I think I'd end up using the menu option more often than the CLI if it existed myself.

ExtremeFiretop commented 3 months ago

Sorry I'm saying main menu, but maybe the advanced menu is better

ExtremeFiretop commented 3 months ago

Question, would you still require the special config file be populated with the special variable, as well as the key combination and special key word while hitting enter?

jksmurf commented 3 months ago

I think it's a great idea and I bet @jksmurf would approve as well if he had a way to call it from the Main Menu.

It’s certainly an option and being in the advanced menu means you are there anyway. I’m sure most advanced users (and I don’t categorise myself as such) would be equally comfortable with the CLI sequence; as would I.

So yes, it’s a nice option for sure but if it’s a lot of extra work, no biggie.

Martinski4GitHub commented 3 months ago

I think it's a great idea and I bet @jksmurf would approve as well if he had a way to call it from the Main Menu.

Question, is there a specific reason we couldn't keep the CLI parameter? I still think having a difference in the method we call those CLI parameters a bit "ugh".

INFO WAS MODIFIED.

Martinski4GitHub commented 3 months ago

Sorry I'm saying main menu, but maybe the advanced menu is better

Yes, I agree. Initially, I had no real preference but yes, the "Advanced Menu Options" would be the better place for it since the new feature is an advanced option.

Martinski4GitHub commented 3 months ago

Question, would you still require the special config file be populated with the special variable, as well as the key combination and special key word while hitting enter?

Yes, that's still a requirement as I explicitly stated here:

INFO WAS MODIFIED.

Martinski4GitHub commented 3 months ago

OK, guys, I'm going to start implementation now. INFO WAS MODIFIED. If not, speak up now and give me your ideas; otherwise, you can "forever hold your peace." :>).

JK... LOL :>) Those can be easily changed later on if anyone has objections or better ideas.

jksmurf commented 3 months ago

INFO WAS MODIFIED.

ExtremeFireTop …

But I’m easy :-)

ExtremeFiretop commented 3 months ago

I have no preference, once I remember it the combination won't matter much hahaha