uniter: add hook error status data

uniter: add hook error status data

With this CL information about a failing hook and,
if possible, the relation id and the remote unit are
written into the status data of the status doc.

https://code.launchpad.net/~themue/juju-core/047-write-hook-error-to-status-data/+merge/187992

(do not edit description out of merge proposal)

[mue, 2013-09-27 08:42:29]

Please take a look.

[dimitern, 2013-09-27 09:11:49]

I don't really like having FullStatus and Status, but I guess there will be a
follow-up to clean this up soon. One suggestion below, otherwise LGTM.

https://codereview.appspot.com/14030043/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/14030043/diff/1/state/unit.go#newcode502
state/unit.go:502: // FullStatus returns the full status of the unit including
the data. It
s/the data/the additional data/ ?

Also, please TODO the second sentence.

[mue, 2013-09-27 11:01:34]

Please take a look.

[fwereade, 2013-09-27 11:11:12]

NOT LGTM on the basis of these. Haven't checked the other bits yet.

https://codereview.appspot.com/14030043/diff/5001/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/14030043/diff/5001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:344: if u.s.Hook.RelationId != -1 {
ha, looks like that was wrong. We should probably be doing this with IsRelation
or something -- if it's a relation hook, RelationId is definitely valid,
otherwise it's ignored, I guess.

https://codereview.appspot.com/14030043/diff/5001/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (left):

https://codereview.appspot.com/14030043/diff/5001/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:1294: resolved state.ResolvedMode
there are no tests for the relation hooks at all

https://codereview.appspot.com/14030043/diff/5001/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/14030043/diff/5001/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:305: "relation-id": 0,
Huh? This is not a relation hook, and should not have a relation id.

https://codereview.appspot.com/14030043/diff/5001/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:347: "relation-id": 0,
ditto

[mue, 2013-09-27 12:49:13]

Please take a look.

[fwereade, 2013-09-27 14:29:57]

I'd really like to see a lot more checks for the actual value of the statusData
result -- maybe not everywhere there's a ditto, but definitely in the unit tests
for state.Machine and state.Unit (other places I'm a little less bothered by...
but really, if we're testing some return values we should test the others).

Apart from that, though, this LGTM. I'd be very happy to see it land with just
the unit tests beefed up, and to fill in the rest later...

...because I think we're actually at the point where we should export
state.StatusDoc (distinct from statusDoc, please) and use that in the state
methods. Would you be amenable to a pure-refactoring followup that did that?

https://codereview.appspot.com/14030043/diff/12001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/14030043/diff/12001/cmd/juju/status.go#newcode507
cmd/juju/status.go:507: status, info, _, err = entity.Status()
add a TODO here and a bug for maybe exposing this usefully to people. Not now
though.

https://codereview.appspot.com/14030043/diff/12001/cmd/jujud/unit_test.go
File cmd/jujud/unit_test.go (right):

https://codereview.appspot.com/14030043/diff/12001/cmd/jujud/unit_test.go#new...
cmd/jujud/unit_test.go:118: c.Fatalf("unexpected status %s %s", st, info)
nice to stick the dict in here maybe?

https://codereview.appspot.com/14030043/diff/12001/state/api/machiner/machine...
File state/api/machiner/machiner_test.go (right):

https://codereview.appspot.com/14030043/diff/12001/state/api/machiner/machine...
state/api/machiner/machiner_test.go:59: status, info, _, err :=
s.machine.Status()
we should be testing this, shouldn't we?

https://codereview.appspot.com/14030043/diff/12001/state/api/machiner/machine...
state/api/machiner/machiner_test.go:67: status, info, _, err =
s.machine.Status()
ditto

https://codereview.appspot.com/14030043/diff/12001/state/api/uniter/unit_test.go
File state/api/uniter/unit_test.go (right):

https://codereview.appspot.com/14030043/diff/12001/state/api/uniter/unit_test...
state/api/uniter/unit_test.go:49: status, info, _, err :=
s.wordpressUnit.Status()
ditto

https://codereview.appspot.com/14030043/diff/12001/state/api/uniter/unit_test...
state/api/uniter/unit_test.go:57: status, info, _, err =
s.wordpressUnit.Status()
ditto

https://codereview.appspot.com/14030043/diff/12001/state/apiserver/machine/ma...
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/14030043/diff/12001/state/apiserver/machine/ma...
state/apiserver/machine/machiner_test.go:74: // Verify machine 0 - no change.
assume more dittos

https://codereview.appspot.com/14030043/diff/12001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/14030043/diff/12001/state/machine.go#newcode708
state/machine.go:708: return "", "", params.StatusData{}, err
nil?

https://codereview.appspot.com/14030043/diff/12001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/14030043/diff/12001/state/machine_test.go#newc...
state/machine_test.go:953: status, info, _, err := s.machine.Status()
more dittos

https://codereview.appspot.com/14030043/diff/12001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/14030043/diff/12001/state/unit.go#newcode495
state/unit.go:495: return "", "", params.StatusData{}, err
nil?

https://codereview.appspot.com/14030043/diff/12001/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/14030043/diff/12001/worker/uniter/modes.go#new...
worker/uniter/modes.go:349: }
Please put the RemoteUnit check inside the IsRelation check, no point looking
for a remote unit if not in a relation hook.

https://codereview.appspot.com/14030043/diff/12001/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/14030043/diff/12001/worker/uniter/uniter_test....
worker/uniter/uniter_test.go:987: }
Perfect, thanks.

[mue, 2013-09-27 15:34:44]

Please take a look.

https://codereview.appspot.com/14030043/diff/12001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/14030043/diff/12001/cmd/juju/status.go#newcode507
cmd/juju/status.go:507: status, info, _, err = entity.Status()
On 2013/09/27 14:29:57, fwereade wrote:
> add a TODO here and a bug for maybe exposing this usefully to people. Not now
> though.

Done. Filing a bug still has to be done.

https://codereview.appspot.com/14030043/diff/12001/cmd/jujud/unit_test.go
File cmd/jujud/unit_test.go (right):

https://codereview.appspot.com/14030043/diff/12001/cmd/jujud/unit_test.go#new...
cmd/jujud/unit_test.go:118: c.Fatalf("unexpected status %s %s", st, info)
On 2013/09/27 14:29:57, fwereade wrote:
> nice to stick the dict in here maybe?

Done.

https://codereview.appspot.com/14030043/diff/12001/state/api/machiner/machine...
File state/api/machiner/machiner_test.go (right):

https://codereview.appspot.com/14030043/diff/12001/state/api/machiner/machine...
state/api/machiner/machiner_test.go:59: status, info, _, err :=
s.machine.Status()
On 2013/09/27 14:29:57, fwereade wrote:
> we should be testing this, shouldn't we?

Done.

https://codereview.appspot.com/14030043/diff/12001/state/api/machiner/machine...
state/api/machiner/machiner_test.go:67: status, info, _, err =
s.machine.Status()
On 2013/09/27 14:29:57, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/14030043/diff/12001/state/api/uniter/unit_test.go
File state/api/uniter/unit_test.go (right):

https://codereview.appspot.com/14030043/diff/12001/state/api/uniter/unit_test...
state/api/uniter/unit_test.go:49: status, info, _, err :=
s.wordpressUnit.Status()
On 2013/09/27 14:29:57, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/14030043/diff/12001/state/api/uniter/unit_test...
state/api/uniter/unit_test.go:57: status, info, _, err =
s.wordpressUnit.Status()
On 2013/09/27 14:29:57, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/14030043/diff/12001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/14030043/diff/12001/state/machine.go#newcode708
state/machine.go:708: return "", "", params.StatusData{}, err
On 2013/09/27 14:29:57, fwereade wrote:
> nil?

Done.

https://codereview.appspot.com/14030043/diff/12001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/14030043/diff/12001/state/unit.go#newcode495
state/unit.go:495: return "", "", params.StatusData{}, err
On 2013/09/27 14:29:57, fwereade wrote:
> nil?

Done.

https://codereview.appspot.com/14030043/diff/12001/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/14030043/diff/12001/worker/uniter/modes.go#new...
worker/uniter/modes.go:349: }
On 2013/09/27 14:29:57, fwereade wrote:
> Please put the RemoteUnit check inside the IsRelation check, no point looking
> for a remote unit if not in a relation hook.

Done.

[fwereade, 2013-09-27 15:37:55]

LGTM, just some droppings

https://codereview.appspot.com/14030043/diff/17001/state/api/uniter/unit_test.go
File state/api/uniter/unit_test.go (right):

https://codereview.appspot.com/14030043/diff/17001/state/api/uniter/unit_test...
state/api/uniter/unit_test.go:65: c.Assert(err, gc.IsNil)
drop this bit