Success/error refactoring

[febbraro]

This allows us to centrally control how things get handled for success and error. It also means we don't need to config which commands provide notifications or not b/c we can just not call those or we can make cmd.Success and cmd.NotifySuccess and cmd.Error and cmd.NotifyError and that leaves it up to each command to control if it integrates with notifications or not?

[grayside]

I like a lot about this. If a given command wants a non-default handling of the notification they can replace the relevant function.

The downside is we will need to have the Run callback for each command do something like:

if err == nil {
  return cmd.Success(...)
} else {
  return cmd.Error(...)
}

The other thing is that we should have the command name in the message, and I'd prefer not to have that explicitly set by the command, it should just be part of the command calling the notification. Can BaseCommand access the calling command name? Maybe it needs to be passed the context as well, cmd.Success(ctx, message) and cmd.Error(ctx, message, code)

[febbraro]

I'm not sure I follow the comment about the Run callback, or really how it is different than what we do now which is cmd.out.Error.Fatal when something really bad happens, or return nil if we end in a good state.

[grayside]

In this case, it's a very minor point that if we switch to a model where we return an error or a nil to indicate success/failure, we will first need to evaluate that err state to take one or other action.

An alternative might be:

return cmd.Complete(ctx, err, messageOnError, codeOnError)

Then the evaluation of whether this was a success or failure can be done centrally. However, I think the finer-grained control of your existing approach is better.

[febbraro]

Did another pass, this needs to be tested, I only verified that it compiles

[grayside]

I thought we were loosely following:

• system packages (alphabetical)
• upstream packages (alphabetical)
• custom packages (alphabetical)

[febbraro]

oops, will fix.

[grayside]

How did this PR sweep up the recursive config loading?

[febbraro]

I think I rebased at some point, but this is weird. Maybe I will rebase on develop again just for good measure

[febbraro]

So my rebasing has now pulled everything we have put into develop into this PR even though it is already in the target branch. Apparently I have no idea how to rebase and manage git branches. sigh