Code review comment for lp:~themue/juju-core/go-state-unit-status

On 2012/08/08 09:57:36, rog wrote:
> https://codereview.appspot.com/6454113/diff/3001/state/unit.go
> File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/3001/state/unit.go#newcode160
> state/unit.go:160: // Status returns the status of the unit.
> On 2012/08/08 07:57:00, fwereade wrote:
> > On 2012/08/07 17:14:58, niemeyer wrote:
> > > The whole thing is looking good, but it's missing more context of
what this
> > is.
> > > What does "status of the unit" actually mean? What are the set of
valid
> > values?
> > > Should these be constants? etc.
> >
> > How about:
> >
> > // Status returns the status of the unit's agent. Expected
> > // results include:
> > // pending: the agent has not started
> > // installed: the agent has installed the charm
> > // started: the agent is running and nothing is wrong
> > // stopped: the agent has done everything it ever will
> > // *-error: the agent failed to run the referenced hook
> > // down: the agent is not functioning correctly
> >
> > I don't *think* these should be constants, because the set of
*-error states
> is
> > not predictable.

> I might add to those:

> // restarting: the agent is restarting after an upgrade.

-0.5, would you expand on why this is where we should show this?

> Also, how about having error as a prefix, so that the actual
> error can be held in the status and it's easy to check for error
status in
> general?

> // error <reason>: the agent has stopped executing hooks for the given
reason.

Would SGTM in a green field, but I think we want to keep the states we
already had in python.

https://codereview.appspot.com/6454113/