Start/restart stacks once. Skip failed pulls. Final summary notification.

[vorezal]

Attempting to address issues #249 and #260. Additionally, I skipped running docker compose up against stacks that it had already been performed for. Open to thoughts on that last bit, since no one actually asked for it but me.

I've tested a fair amount, but I don't think I hit every case. I wasn't able to "successfully" cause a docker pull to error or even think of how to force it to do so as a test. I'll keep testing more myself, but I wanted to get this up for review/thoughts before I went too far with a particular approach that someone else may know of a better way to handle.

[mag37]

Great work again @vorezal ! As I mentioned in the other PR, I'm a bit tight on time currently. I'll try to review this and get back to you asap, looks good at first glance! Though a compose.yaml might have snuck in from the other PR?

Maybe the pull error can be forced by hardcoding wrong $ContImage for a specific container during a test or something. I'll try to look at it when I find some time.

[yoyoma2]

Has the sourcing policy of v1 and v2 changed? Looks like v2 is now first and v1 is second. If v1 isn't there will v2 be sourced again?

I like that the summary notification is off by default.

The actions being logged are quite limited so introducing bugs as in my comment somewhere else aren't an issue.

The maintenance part however is still a concern. From the beginning notifications were considered outside the scope of dockcheck and deliberately separate. Now a new dockcheck feature needs to consider adding actions. Adding actions to the summary involves dockcheck version change instead of only a notify_v2 version change. This is a notable architectural change.

[vorezal]

No worries on the timing. Happy to keep this in the pipe as long as necessary for it to be properly reviewed. Thanks for the notice on the compose.yaml as well. I forgot that I made the .gitignore changes on the other branch and that made it through. I'll have to force push to remove it. No reason people need to see my compose.yaml.

I agree on the maintenance concern, though I'm also not too sure what to do about it. It could potentially be minimized by wrapping actions within another function that combines execution, error reporting, and storing the action, but no matter what tracking new actions would always require a version bump since we'd be modifying dockcheck.sh. It would be equivalent to adding new cli log output I suppose.

[mag37]

The maintenance part however is still a concern. From the beginning notifications were considered outside the scope of dockcheck and deliberately separate. Now a new dockcheck feature needs to consider adding actions.

Valid points to consider @yoyoma2 , thank you for raising them.
I've not reviewed the code properly yet so can't go into detail or alternatives, but I think - as @vorezal mention - to avoid touching dockcheck.sh we'd probably create a lot of extra hoops to jump through and messy dependencies/connections.

Hopefully the touching of dockcheck.sh won't be too often, and we could just decide to "lock" the scope to what we decide is complete with this feature change. Or decide to only push notification-changes within larger releases or something.

[mag37]

Line 690-699 is a bit hard to read and just from reading I can't say surely if the logic is sound, I'd need to dumb it down and create a test case for it to see that it does what intended.

        # Check if the whole stack should be restarted
        if [[ "$ContRestartStack" == true ]] || [[ "$ForceRestartStacks" == true ]]; then
          [[ "${RestartedStacks[@]}" != *"$ContPath"* ]] && { ${DockerBin} ${CompleteConfs} stop; ${DockerBin} ${CompleteConfs} ${ContEnvs} up -d && Actions+=("Recreate $i;Success;Full stack restart") || { printf "\n%bFailed to recreate $i, skipping.%b\n" "$c_red" "$c_reset" ; Actions+=("Recreate $i;Error;Failed to start stack") ; } }
	  RestartedStacks+=("$ContPath")
        else
          { [[ "${RestartedStacks[@]}" != *"$ContPath"* ]] || [[ -n "${SpecificContainer:-}" ]] } && { ${DockerBin} ${CompleteConfs} ${ContEnvs} up -d ${SpecificContainer} && Actions+=("Recreate $i;Success;${SpecificContainer:-Stack}") || { printf "\n%bFailed to recreate $i, skipping.%b\n" "$c_red" "$c_reset" ; Actions+=("Recreate $i;Error;Failed to start ${SpecificContainer:-Stack}") ; } }
	  if [[ -z "${SpecificContainer:-}" ]]; then
            RestartedStacks+=("$ContPath")
	  fi
        fi

Also looks like there's some indentation errors in the Recreate phase in general.

[vorezal]

Agreed. My first pass at that was pretty rough. I'll try to clean up the logic. My IDE also kept auto-indenting in a way that I didn't intend to and I didn't put in the effort to fix it at the time. I'll clean that up too.

[yoyoma2]

When the first notifications were introduced it was considered outside the scope of dockcheck. It turns out to have become a popular feature so the strict separation may not be a big deal any more. Recently someone fixed notify_v2 for unraid and I thought it was super cool that I got a notification for a new v2 but not for dockcheck.

Brainstorming:
What if logging calls were added to dockcheck with the usual verbosity levels like NOTICE, WARNING, ERROR and stored into a file and/or sent as a notification. Then the new logging system is sprinkled throughout dockcheck instead of the notification system sprinkled throughout dockcheck.

Again why the new sourcing of notify_v2 in line 163? Looks like it will get sourced again right after if you don't have a v1.

[vorezal]

That's a good thought. Building a logging system once within dockcheck and then having notification logic to parse through for log message types or verbosity levels would be more work, but it would be more segregated and potentially more maintainable. Let me think about how this could be done.

Apologies, I forgot to address that source change. At one point I was troubleshooting an error causing the source to fail. That was meant as a troubleshooting line and forgetting to remove it was a miss on my part. I'll make sure to get it out of there.

[yoyoma2]

After the v2 revamp stuff you've set the bar high. Curious to see your ideas.

Figures that sourcing thing was just forgotten troubleshooting code. Everything else made perfect sense.