Fix Changelog and Email Notifications and Patch Uninstall

[ExtremeFiretop]

Patch Changelog and Email Notifications

[ExtremeFiretop]

@Martinski4GitHub

To deal with report from nlurker here for blank changelog URLs in the email notifications: https://www.snbforums.com/threads/merlinau-v1-4-1-the-ultimate-firmware-auto-updater-gnuton-support.91326/post-951011

Changelog check (pre-approval) not running before the email notification with the run_now parameter.
It's stopped by:

           if ! "$FlashStarted"
           then
               if "$isGNUtonFW"
               then
                   _ManageChangelogGnuton_ "download"
               else
                   _ManageChangelogMerlin_ "download" "$fwNewUpdateNotificationVers"
               fi
           fi

in CheckNewUpdateFirmwareNotification function due to PR:
#242

The problem is that for me I ran it interactively; which did this flow:

But nlurker followed this folow:

[ExtremeFiretop]

Solution is below:

Block flow as cron job:

Cron job approved flow:

Blocked flow interactively:

Actual update flow interactively:

[ExtremeFiretop]

Merged in an import fix as well for a report here: https://www.snbforums.com/threads/merlinau-v1-4-1-the-ultimate-firmware-auto-updater-gnuton-support.91326/post-951122

About being unable to uninstall

[Martinski4GitHub]

@ExtremeFiretop,

I would like to get some clarification. Is our new policy when submitting PRs to merge them right away into the development branch without waiting for a "2nd pair of eyes" to review the changes, just to double-check and make sure that we're not inadvertently introducing a bug or doing something that may be incorrect or unnecessary?

I'm asking because lately, you have merged your last few PRs into the development branch right away without waiting for an external review. Is that new policy in effect from now on?

[Martinski4GitHub]

Merged in an import fix as well for a report here: https://www.snbforums.com/threads/merlinau-v1-4-1-the-ultimate-firmware-auto-updater-gnuton-support.91326/post-951122

About being unable to uninstall

How was MerlinAU installed in the first place?
Were they using AMTM? Or just manually typing the curl command?
What is the router model?
Did the user actually install MerlinAU? Or did they just attempt to install it, but the installation failed?
There are many unknowns, and there is absolutely no context for what the user reported, but you didn't ask any such questions.

What do you mean by "an import fix"?

In any case, the "fix" that you made introduced a bug. I'm going to submit a PR shortly to fix it in addition to some improvements.

[ExtremeFiretop]

Redacted

[ExtremeFiretop]

@Martinski4GitHub

Sorry for being so sharp. Upon reflection you didn't deserve that original message.
I think I'm just having a bad day and you poked a nerve by making my contribution feel like a mistake.

I didn't intend on going on a venting rampage. Sorry again. I am still open to discussing the changes in your PR and will redact messages.

[Martinski4GitHub]

@ExtremeFiretop,

I would like to get some clarification. Is our new policy when submitting PRs to merge them right away into the development branch without waiting for a "2nd pair of eyes" to review the changes, just to double-check and make sure that we're not inadvertently introducing a bug or doing something that may be incorrect or unnecessary?

I'm asking because lately, you have merged your last few PRs into the development branch right away without waiting for an external review. Is that new policy in effect from now on?

Just to be clear, I'm not necessarily opposed to a policy change WRT a PR submission to the development branch. Either way, I can still handle the PR changes & review them. I was just wondering whether the policy had indeed changed lately so that I could adjust accordingly.

The PR #453 being merged without an external review is what finally prompted my question because, IMO, there was nothing critical or urgent in the PR that required sending it out to users for immediate testing & validation. The reported problems were not crashing the script or preventing users from performing a F/W Update - IOW, no showstoppers - so I didn't see the urgency to get the fixes out to users ASAP, hence my message asking for clarification.

When there's a serious bug that prevents users from running the script (e.g. "Parameter not found" error/crash), or a showstopper bug affecting critical functionality (e.g. not being able to run a F/W update check), then yes, of course, I see the need to fix and merge the PR right away to get it "out the door" ASAP for testing and validation purposes. Those scenarios were not in question here.

I guess, ultimately, the decision to wait for a PR review or to merge the PR right way will depend on our own definition of what constitutes a "critical" or a "showstopper" bug.

Anyway, I just wanted to clear the air and understand whether we were on the same page about this specific subject. I believe we are except for some "threshold" bug reports where we might agree or disagree on whether the bugs are considered "critical" (i.e. high severity) or perhaps just simply "annoying" (i.e. low severity).

[Martinski4GitHub]

@Martinski4GitHub

Sorry for being so sharp. Upon reflection you didn't deserve that original message. I think I'm just having a bad day and you poked a nerve by making my contribution feel like a mistake.

I didn't intend on going on a venting rampage. Sorry again. I am still open to discussing the changes in your PR and will redact messages.

I believe I understand. The triggering phrase was my saying that "the fix that you made introduced a bug." I can see how that can be taken as an offense, and I apologize for that. And I accept your apology as well.

Now, just to clear the air, let me explain where I'm coming from. In S/W development, when we find a regression issue, we may say things like "The bug was introduced in commit #567" or "Developer XYZ introduced a bug when fixing the issue in commit #567 to address problem report #987." This is just how we communicate among ourselves, and everyone understands that it's not meant to be a personal attack or personal offense. It's just a declarative statement indicating where or how a regression bug was introduced into the S/W. That's SOP and normal in our engineering, development environment.

We certainly give praise and credit where credit is due, but during our "Lessons Learned" sessions, we also may point out where possible or actual mistakes have been made so we can all learn from them and avoid them in the future. That's part of how we learn to be better at our jobs as developers.

But again, I can see where if you're not used to that kind of working environment, day in and day out, 40+ hours per week, it can be taken out of context as an "attack." But, to be absolutely clear, that was not my intention or purpose at all.

[ExtremeFiretop]

@ExtremeFiretop,
I would like to get some clarification. Is our new policy when submitting PRs to merge them right away into the development branch without waiting for a "2nd pair of eyes" to review the changes, just to double-check and make sure that we're not inadvertently introducing a bug or doing something that may be incorrect or unnecessary?
I'm asking because lately, you have merged your last few PRs into the development branch right away without waiting for an external review. Is that new policy in effect from now on?

Just to be clear, I'm not necessarily opposed to a policy change WRT a PR submission to the development branch. Either way, I can still handle the PR changes & review them. I was just wondering whether the policy had indeed changed lately so that I could adjust accordingly.

The PR #453 being merged without an external review is what finally prompted my question because, IMO, there was nothing critical or urgent in the PR that required sending it out to users for immediate testing & validation. The reported problems were not crashing the script or preventing users from performing a F/W Update - IOW, no showstoppers - so I didn't see the urgency to get the fixes out to users ASAP, hence my message asking for clarification.

When there's a serious bug that prevents users from running the script (e.g. "Parameter not found" error/crash), or a showstopper bug affecting critical functionality (e.g. not being able to run a F/W update check), then yes, of course, I see the need to fix and merge the PR right away to get it "out the door" ASAP for testing and validation purposes. Those scenarios were not in question here.

I guess, ultimately, the decision to wait for a PR review or to merge the PR right way will depend on our own definition of what constitutes a "critical" or a "showstopper" bug.

Anyway, I just wanted to clear the air and understand whether we were on the same page about this specific subject. I believe we are except for some "threshold" bug reports where we might agree or disagree on whether the bugs are considered "critical" (i.e. high severity) or perhaps just simply "annoying" (i.e. low severity).

No generally the policy is the same as we originally stated over a year ago. Submit to dev for review and testing, anything urgent can be committed merged right away.

Generally we are talking about dev here, but the same applies to production. We've done that in the past as well and just blasted out a fix almost instantly. If it's called for.

I would be more skeptical about doing that though. But for dev I don't really mind merging something in right away to test. Giving a link to a random branch I created isn't ideal and it's much easier to provide the curl or the develop command to have them test right away.

With this PR it's an example of going straight to dev to get that guy to uninstall and move on. He can't use develop command since the script won't run. So it basically becomes a show stopper for him. Now I realize that's not a big deal for a single unsupported user where the script isn't going to run by design.

But I would personally be extremely unhappy and unimpressed as that user to install it and find out it's stuck there now and I can't even use it. That's why in my eyes it was ultimately worth while to go straight to dev for testing.

But I say that like I didn't spend an hour testing it and making sure it worked before hand. Which I did. I checked it worked as a cron, I checked it worked interactively, I checked it worked on the parent and the nodes, I checked it up and down before I gave the user the URL to test

[ExtremeFiretop]

@Martinski4GitHub
Sorry for being so sharp. Upon reflection you didn't deserve that original message. I think I'm just having a bad day and you poked a nerve by making my contribution feel like a mistake.
I didn't intend on going on a venting rampage. Sorry again. I am still open to discussing the changes in your PR and will redact messages.

I believe I understand. The triggering phrase was my saying that "the fix that you made introduced a bug." I can see how that can be taken as an offense, and I apologize for that. And I accept your apology as well.

Now, just to clear the air, let me explain where I'm coming from. In S/W development, when we find a regression issue, we may say things like "The bug was introduced in commit #567" or "Developer XYZ introduced a bug when fixing the issue in commit #567 to address problem report #987." This is just how we communicate among ourselves, and everyone understands that it's not meant to be a personal attack or personal offense. It's just a declarative statement indicating where or how a regression bug was introduced into the S/W. That's SOP and normal in our engineering, development environment.

We certainly give praise and credit where credit is due, but during our "Lessons Learned" sessions, we also may point out where possible or actual mistakes have been made so we can all learn from them and avoid them in the future. That's part of how we learn to be better at our jobs as developers.

But again, I can see where if you're not used to that kind of working environment, day in and day out, 40+ hours per week, it can be taken out of context as an "attack." But, to be absolutely clear, that was not my intention or purpose at all.

Honestly that may be part of it yes, but it's not the first time you've told me I introduced a bug. It's not my first rodeo dealing with development and devs either. I think I had more frustration built in for other reasons and I mistakenly redirected some of it towards you this morning.

To save you the long story I'm dealing with the car dealership over my car for warranty repairs, and I'm not sure if it's the same for you but this is the only dealership for my Genesis within 300km and their being assholes and j was ready for a fight this morning dealing with them and I think I inadvertently unloaded some towards you and I'm sorry and that's why I say you didn't deserve that.

Usually I would let this type of thing roll off me like it's nothing and not take it personally and just ask for clarification and deal with it logically. But today it felt personal for some reason and maybe your wording had part in it, I think the other part is completely unrelated to you I just lashed out ready for escalation.

[ExtremeFiretop]

@ExtremeFiretop,
I would like to get some clarification. Is our new policy when submitting PRs to merge them right away into the development branch without waiting for a "2nd pair of eyes" to review the changes, just to double-check and make sure that we're not inadvertently introducing a bug or doing something that may be incorrect or unnecessary?
I'm asking because lately, you have merged your last few PRs into the development branch right away without waiting for an external review. Is that new policy in effect from now on?

Just to be clear, I'm not necessarily opposed to a policy change WRT a PR submission to the development branch. Either way, I can still handle the PR changes & review them. I was just wondering whether the policy had indeed changed lately so that I could adjust accordingly.
The PR #453 being merged without an external review is what finally prompted my question because, IMO, there was nothing critical or urgent in the PR that required sending it out to users for immediate testing & validation. The reported problems were not crashing the script or preventing users from performing a F/W Update - IOW, no showstoppers - so I didn't see the urgency to get the fixes out to users ASAP, hence my message asking for clarification.
When there's a serious bug that prevents users from running the script (e.g. "Parameter not found" error/crash), or a showstopper bug affecting critical functionality (e.g. not being able to run a F/W update check), then yes, of course, I see the need to fix and merge the PR right away to get it "out the door" ASAP for testing and validation purposes. Those scenarios were not in question here.
I guess, ultimately, the decision to wait for a PR review or to merge the PR right way will depend on our own definition of what constitutes a "critical" or a "showstopper" bug.
Anyway, I just wanted to clear the air and understand whether we were on the same page about this specific subject. I believe we are except for some "threshold" bug reports where we might agree or disagree on whether the bugs are considered "critical" (i.e. high severity) or perhaps just simply "annoying" (i.e. low severity).

No generally the policy is the same as we originally stated over a year ago. Submit to dev for review and testing, anything urgent can be committed merged right away.

Generally we are talking about dev here, but the same applies to production. We've done that in the past as well and just blasted out a fix almost instantly. If it's called for.

I would be more skeptical about doing that though. But for dev I don't really mind merging something in right away to test. Giving a link to a random branch I created isn't ideal and it's much easier to provide the curl or the develop command to have them test right away.

With this PR it's an example of going straight to dev to get that guy to uninstall and move on. He can't use develop command since the script won't run. So it basically becomes a show stopper for him. Now I realize that's not a big deal for a single unsupported user where the script isn't going to run by design.

But I would personally be extremely unhappy and unimpressed as that user to install it and find out it's stuck there now and I can't even use it. That's why in my eyes it was ultimately worth while to go straight to dev for testing.

But I say that like I didn't spend an hour testing it and making sure it worked before hand. Which I did. I checked it worked as a cron, I checked it worked interactively, I checked it worked on the parent and the nodes, I checked it up and down before I gave the user the URL to test

And to clarify, it's the same with you. If I'm sleeping and you merge something straight to dev I won't be mad. As long as it's for user testing of a reported issue and it's only going to dev, we can always roll it back it tune it and refine after the fact.

[Martinski4GitHub]

@ExtremeFiretop,
I would like to get some clarification. Is our new policy when submitting PRs to merge them right away into the development branch without waiting for a "2nd pair of eyes" to review the changes, just to double-check and make sure that we're not inadvertently introducing a bug or doing something that may be incorrect or unnecessary?
I'm asking because lately, you have merged your last few PRs into the development branch right away without waiting for an external review. Is that new policy in effect from now on?

Just to be clear, I'm not necessarily opposed to a policy change WRT a PR submission to the development branch. Either way, I can still handle the PR changes & review them. I was just wondering whether the policy had indeed changed lately so that I could adjust accordingly.

The PR #453 being merged without an external review is what finally prompted my question because, IMO, there was nothing critical or urgent in the PR that required sending it out to users for immediate testing & validation. The reported problems were not crashing the script or preventing users from performing a F/W Update - IOW, no showstoppers - so I didn't see the urgency to get the fixes out to users ASAP, hence my message asking for clarification.

When there's a serious bug that prevents users from running the script (e.g. "Parameter not found" error/crash), or a showstopper bug affecting critical functionality (e.g. not being able to run a F/W update check), then yes, of course, I see the need to fix and merge the PR right away to get it "out the door" ASAP for testing and validation purposes. Those scenarios were not in question here.

I guess, ultimately, the decision to wait for a PR review or to merge the PR right way will depend on our own definition of what constitutes a "critical" or a "showstopper" bug.

Anyway, I just wanted to clear the air and understand whether we were on the same page about this specific subject. I believe we are except for some "threshold" bug reports where we might agree or disagree on whether the bugs are considered "critical" (i.e. high severity) or perhaps just simply "annoying" (i.e. low severity).

No generally the policy is the same as we originally stated over a year ago. Submit to dev for review and testing, anything urgent can be committed merged right away.

Generally we are talking about dev here, but the same applies to production. We've done that in the past as well and just blasted out a fix almost instantly. If it's called for.

I would be more skeptical about doing that though. But for dev I don't really mind merging something in right away to test. Giving a link to a random branch I created isn't ideal and it's much easier to provide the curl or the develop command to have them test right away.

With this PR it's an example of going straight to dev to get that guy to uninstall and move on. He can't use develop command since the script won't run. So it basically becomes a show stopper for him. Now I realize that's not a big deal for a single unsupported user where the script isn't going to run by design.

Yeah, I get it. That's what I meant by saying:

I guess, ultimately, the decision to wait for a PR review or to merge the PR right way will depend on our own definition of what constitutes a "critical" or a "showstopper" bug.

In any case, things are much clearer now.

But I would personally be extremely unhappy and unimpressed as that user to install it and find out it's stuck there now and I can't even use it. That's why in my eyes it was ultimately worth while to go straight to dev for testing.

We look at things differently in that regard. If I have paid money for the S/W, I do expect "more and better," and get annoyed or frustrated by little bugs. However, if it's free S/W that someone is developing in their own spare time without any compensation at all, I'm much more forgiving and don't get annoyed or frustrated when a bug is found because I fully understand the amount of time and effort it takes to develop, build & test good S/W. Some non-tech users might not fully understand that, but that's due to ignorance more than anything else.

But I say that like I didn't spend an hour testing it and making sure it worked before hand. Which I did. I checked it worked as a cron, I checked it worked interactively, I checked it worked on the parent and the nodes, I checked it up and down before I gave the user the URL to test

[Martinski4GitHub]

@ExtremeFiretop,
I would like to get some clarification. Is our new policy when submitting PRs to merge them right away into the development branch without waiting for a "2nd pair of eyes" to review the changes, just to double-check and make sure that we're not inadvertently introducing a bug or doing something that may be incorrect or unnecessary?
I'm asking because lately, you have merged your last few PRs into the development branch right away without waiting for an external review. Is that new policy in effect from now on?

Just to be clear, I'm not necessarily opposed to a policy change WRT a PR submission to the development branch. Either way, I can still handle the PR changes & review them. I was just wondering whether the policy had indeed changed lately so that I could adjust accordingly.
The PR #453 being merged without an external review is what finally prompted my question because, IMO, there was nothing critical or urgent in the PR that required sending it out to users for immediate testing & validation. The reported problems were not crashing the script or preventing users from performing a F/W Update - IOW, no showstoppers - so I didn't see the urgency to get the fixes out to users ASAP, hence my message asking for clarification.
When there's a serious bug that prevents users from running the script (e.g. "Parameter not found" error/crash), or a showstopper bug affecting critical functionality (e.g. not being able to run a F/W update check), then yes, of course, I see the need to fix and merge the PR right away to get it "out the door" ASAP for testing and validation purposes. Those scenarios were not in question here.
I guess, ultimately, the decision to wait for a PR review or to merge the PR right way will depend on our own definition of what constitutes a "critical" or a "showstopper" bug.
Anyway, I just wanted to clear the air and understand whether we were on the same page about this specific subject. I believe we are except for some "threshold" bug reports where we might agree or disagree on whether the bugs are considered "critical" (i.e. high severity) or perhaps just simply "annoying" (i.e. low severity).

No generally the policy is the same as we originally stated over a year ago. Submit to dev for review and testing, anything urgent can be committed merged right away.
Generally we are talking about dev here, but the same applies to production. We've done that in the past as well and just blasted out a fix almost instantly. If it's called for.
I would be more skeptical about doing that though. But for dev I don't really mind merging something in right away to test. Giving a link to a random branch I created isn't ideal and it's much easier to provide the curl or the develop command to have them test right away.
With this PR it's an example of going straight to dev to get that guy to uninstall and move on. He can't use develop command since the script won't run. So it basically becomes a show stopper for him. Now I realize that's not a big deal for a single unsupported user where the script isn't going to run by design.
But I would personally be extremely unhappy and unimpressed as that user to install it and find out it's stuck there now and I can't even use it. That's why in my eyes it was ultimately worth while to go straight to dev for testing.
But I say that like I didn't spend an hour testing it and making sure it worked before hand. Which I did. I checked it worked as a cron, I checked it worked interactively, I checked it worked on the parent and the nodes, I checked it up and down before I gave the user the URL to test

And to clarify, it's the same with you. If I'm sleeping and you merge something straight to dev I won't be mad. As long as it's for user testing of a reported issue and it's only going to dev, we can always roll it back it tune it and refine after the fact.

Yep, understood.

[Martinski4GitHub]

@Martinski4GitHub
Sorry for being so sharp. Upon reflection you didn't deserve that original message. I think I'm just having a bad day and you poked a nerve by making my contribution feel like a mistake.
I didn't intend on going on a venting rampage. Sorry again. I am still open to discussing the changes in your PR and will redact messages.

I believe I understand. The triggering phrase was my saying that "the fix that you made introduced a bug." I can see how that can be taken as an offense, and I apologize for that. And I accept your apology as well.
Now, just to clear the air, let me explain where I'm coming from. In S/W development, when we find a regression issue, we may say things like "The bug was introduced in commit #567" or "Developer XYZ introduced a bug when fixing the issue in commit #567 to address problem report #987." This is just how we communicate among ourselves, and everyone understands that it's not meant to be a personal attack or personal offense. It's just a declarative statement indicating where or how a regression bug was introduced into the S/W. That's SOP and normal in our engineering, development environment.
We certainly give praise and credit where credit is due, but during our "Lessons Learned" sessions, we also may point out where possible or actual mistakes have been made so we can all learn from them and avoid them in the future. That's part of how we learn to be better at our jobs as developers.
But again, I can see where if you're not used to that kind of working environment, day in and day out, 40+ hours per week, it can be taken out of context as an "attack." But, to be absolutely clear, that was not my intention or purpose at all.

Honestly that may be part of it yes, but it's not the first time you've told me I introduced a bug. It's not my first rodeo dealing with development and devs either. I think I had more frustration built in for other reasons and I mistakenly redirected some of it towards you this morning.

To save you the long story I'm dealing with the car dealership over my car for warranty repairs, and I'm not sure if it's the same for you but this is the only dealership for my Genesis within 300km and their being assholes and j was ready for a fight this morning dealing with them and I think I inadvertently unloaded some towards you and I'm sorry and that's why I say you didn't deserve that.

Usually I would let this type of thing roll off me like it's nothing and not take it personally and just ask for clarification and deal with it logically. But today it felt personal for some reason and maybe your wording had part in it, I think the other part is completely unrelated to you I just lashed out ready for escalation.

I understand. Sometimes, we let our emotions override our good, common sense, and end up doing things we shouldn't have. We've all "been there, done that."

But no worries: "It's water under the bridge."