Implement a consistent state

[croqaz]

This PR implements a consistent state for Cmd.

• if the respective command crashes on start, the states will be: initial, starting, fatal
• it it's a natural exit, the states will be: initial, starting, running, finished
• if it's Stopped after start, the states will be: initial, starting, running, stopping, interrupt
• there's also the case when it's killed externally, so in that case the "stopping" state will not be triggered
• the fatal, finished and interrupt states are final and the command can never be started again. This is where Clone() function is useful, so you can start a fresh Cmd instance.

Human readable description of the changes:

• after this PR gets merged, I'll create a git tag, probably "v1.1" because this is a breaking change - because of the Cmd and Status struct changes
• removed started, stopped, done, final from "Cmd" struct and replaced with State (is it a good idea to make the State a public variable ?)
• removed Complete variable from "Status" struct (is it a good idea to include the State instead ? In case it's a private variable, it would make sense)
• added private setState and public IsInitialState and IsFinalState because they are relevant to be public.

[coveralls]

Coverage decreased (-1.2%) to 98.78% when pulling 02fedc5 on new-state into 593c238 on master.

[coveralls]

Coverage decreased (-1.2%) to 98.78% when pulling 02fedc5 on new-state into 593c238 on master.

[coveralls]

Coverage remained the same at 100.0% when pulling a9258bb on new-state into 593c238 on master.

[croqaz]

I also have to update the readme, after we agree that this implementation is OK.

[daniel-nichter]

Have some feedback but first: what's this solving? What's the intended use vs. current API?

[croqaz]

Have some feedback but first: what's this solving? What's the intended use vs. current API?

tl;dr; This is not solving anything, it's simplifying and preparing for other features that would be hard to do if this PR wouldn't exist.

1. Funnel all the state changes in one place, in the function setState. This is required for next functionality of emitting events on each state change, via a channel. Without this funnel, it would be impossible to pinpoint 1 place in the code where the state changes are happening, so that would be hard to do.
2. Simplify the Cmd struct by squashing 4 variables into only 1. The "state" variable is much more semantic and much easier to reason about, for somebody new to the code, vs "started", "stopped", "done", "final" IMHO.
   I'm very much about "don't fix it if it's not broken" philosophy, so I understand this PR is a bit controversial, because the code as it is today is already deployed in production.

[croqaz]

I very much think the risks of this PR are mininal, if we don't consider the breaking changes.
If someone would use the NewCmd() to initialize a Cmd, there are no changes.
The only breaking change is in the Status. The open discussion is if we want to include the "state" in there, or just make it public as Cmd.State (like it is right now)

[daniel-nichter]

This is required for next functionality of emitting events on each state change, via a channel.

If I understand correctly, that's the goal? If so, then what's the use case look like? I.e. how do you envision this being used? I imagine you want the caller to be notified when the cmd starts and stops (for any reason, i.e. successful or not)?

Yes re "don't fix it if it's not broken". Also, I think it's beneficial to work backwards from the user experience to the tech which is why I'm more curious about the former than the latter at this point. Another philosophy: avoiding feature creep. This seems like a good feature. I want to keep it as minimally implemented as possible so it's less to test, document, and support.

[daniel-nichter]

Also looks like you want a full push model rather than a mixed push and pull/poll model wrt cmd status? With current API, there's one push: the final Status returned on Start() <-chan Status. But before and while running, caller has to poll with Status().

[croqaz]

This is required for next functionality of emitting events on each state change, via a channel.

If I understand correctly, that's the goal?

So the goal of this PR is only to simplify the state from many variables to only 1 and to funnel the state changes into a single function. Inside this function I will emit events on each state change, but this will happen in another PR.

This seems like a good feature. I want to keep it as minimally implemented as possible [..]

Do you have any specific sugestions? In my point of view, this is the smallest it can get.

Also looks like you want a full push model rather than a mixed push and pull/poll model wrt cmd status?

Inside the new setState function we can emit only the new state, which is just a uint8 (that's for the next PR)

With current API, there's one push: the final Status returned on Start() <-chan Status.

I don't think we need to change the Start() function (again, that's for the next PR). The state change could be in a separate function called "Watch" and should be optional.

But again, maybe it's better to move the discussion of the state watching in the next PR, when I actually write the code for that, just to keep this discussion easier.
You can take a look at what I have in mind here: https://github.com/ShinyTrinkets/overseer/blob/master/cmd.go

[croqaz]

I realize that we could implement this PR without breaking the compatibility of the Status struct.

We can keep the Complete boolean in there just like before and it can return "true" if the new function IsFinalState returns true, so it does exactly the same thing.

Not sure, is this a hard requirement? To maintain backward compatibility?

[daniel-nichter]

Closing due to merge conflicts and inactivity. I'm not sure this is the right direction for this pkg. It's a fair bit of additional logic (i.e. states); I'd rather keep the pkg very simple: just start, status, and stop a command. I think a change like this might warrant a fork. In any case, thank you for your work!