state: added status data to status doc

state: added status data to status doc

This is the 2nd CL for lp:1224568. It adds status data to
the status doc and changes the API. Also the first infos
in ModeHookError() are added to the status. If this CL
is conceptual ok more tests have to be added and the exact
data of ModeHooError() has to be discussed with the GUI
team.

https://code.launchpad.net/~themue/juju-core/046-added-status-data/+merge/186130

(do not edit description out of merge proposal)

[mue, 2013-09-17 18:26:33]

Please take a look.

[mue, 2013-09-18 08:39:05]

Please take a look.

[mue, 2013-09-18 15:59:17]

Please take a look.

[fwereade, 2013-09-19 08:32:23]

Looking nice. Quickest way to get it landed will be to beef up the testing in
state and defer the actual behaviour changes to the next CL.

https://codereview.appspot.com/13430044/diff/7001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/13430044/diff/7001/state/machine.go#newcode703
state/machine.go:703: func (m *Machine) StatusData() (params.StatusData, error)
{
Not sure about this. Is it just for testing? If so, I'd put it in export_test
temporarily in preparation for a followup signature change in Status() so that
the sets and gets match.

https://codereview.appspot.com/13430044/diff/7001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/13430044/diff/7001/state/machine_test.go#newco...
state/machine_test.go:973: c.Assert(data["retries"], gc.Equals, 5)
Let's have a fresh test or two and cover this in a bit more detail -- for
example, check that dicts properly overwrite one another, and that the data dict
doesn't contain clever mongo syntax that could cause problems. (It *sucks* that
we don't have an automatic way of doing this, because I'm pretty sure we're
riddled with bson injection vulnerabilities already, but we have to start
somewhere.)

https://codereview.appspot.com/13430044/diff/7001/state/unit.go
File state/unit.go (left):

https://codereview.appspot.com/13430044/diff/7001/state/unit.go#oldcode508
state/unit.go:508: if err := doc.validateSet(); err != nil {
Yeah, I think we need a bit of sanity checking in here.

https://codereview.appspot.com/13430044/diff/7001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/13430044/diff/7001/state/unit.go#newcode503
state/unit.go:503: func (u *Unit) StatusData() (params.StatusData, error) {
Similarly, I think this should be in export_test until we add a data return to
Status()

https://codereview.appspot.com/13430044/diff/7001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/13430044/diff/7001/state/unit_test.go#newcode300
state/unit_test.go:300: c.Assert(data["remote-unit"], gc.Equals, "unit-mysql-0")
Similarly, it would be good to beef these tests up a bit as suggested for
machine.

https://codereview.appspot.com/13430044/diff/7001/worker/uniter/modes.go
File worker/uniter/modes.go (left):

https://codereview.appspot.com/13430044/diff/7001/worker/uniter/modes.go#oldc...
worker/uniter/modes.go:341: msg := fmt.Sprintf("hook failed: %q", u.s.Hook.Kind)
The Kind here is inadequate, I think. We really ought to name the hook, which
will take a small amount of looking up the relation. The relation should always
be known to the uniter if we're at this point, so it should be trivial to figure
out the actual hook name (as we do in the hook runner -- it may be smart to put
a helper method on uniter that can turn a hook.Info into an actual hook name?).

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

https://codereview.appspot.com/13430044/diff/7001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:346: "remote-unit": u.s.Hook.RemoteUnit,
I think we always want:

    "hook": <full-hook-name-as-described-above>,

...and that if it's a relation hook (RelationId != -1) we also want:

    "relation-id": <id>,

...and that if the remote unit is not "", we also want:

    "remote-unit": <name>,

https://codereview.appspot.com/13430044/diff/7001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:348: if err = u.unit.SetStatus(params.StatusError, msg,
data); err != nil {
...and, hey, I don't think this bit is tested.

I'd be more than happy to *not* change the functionality in this CL, though --
I'd just drop this change, get this one in as soon as possible, and then worry
about piping the data through.

https://codereview.appspot.com/13430044/diff/7001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:387: // TODO(mue) Add helpful data here too.
Great idea. Make it another CL though please.

[mue, 2013-09-24 09:01:48]

Please take a look.

https://codereview.appspot.com/13430044/diff/7001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/13430044/diff/7001/state/machine.go#newcode703
state/machine.go:703: func (m *Machine) StatusData() (params.StatusData, error)
{
On 2013/09/19 08:32:24, fwereade wrote:
> Not sure about this. Is it just for testing? If so, I'd put it in export_test
> temporarily in preparation for a followup signature change in Status() so that
> the sets and gets match.

Done.

https://codereview.appspot.com/13430044/diff/7001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/13430044/diff/7001/state/machine_test.go#newco...
state/machine_test.go:973: c.Assert(data["retries"], gc.Equals, 5)
On 2013/09/19 08:32:24, fwereade wrote:
> Let's have a fresh test or two and cover this in a bit more detail -- for
> example, check that dicts properly overwrite one another, and that the data
dict
> doesn't contain clever mongo syntax that could cause problems. (It *sucks*
that
> we don't have an automatic way of doing this, because I'm pretty sure we're
> riddled with bson injection vulnerabilities already, but we have to start
> somewhere.)

Done.

https://codereview.appspot.com/13430044/diff/7001/state/unit.go
File state/unit.go (left):

https://codereview.appspot.com/13430044/diff/7001/state/unit.go#oldcode508
state/unit.go:508: if err := doc.validateSet(); err != nil {
On 2013/09/19 08:32:24, fwereade wrote:
> Yeah, I think we need a bit of sanity checking in here.

Done.

https://codereview.appspot.com/13430044/diff/7001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/13430044/diff/7001/state/unit.go#newcode503
state/unit.go:503: func (u *Unit) StatusData() (params.StatusData, error) {
On 2013/09/19 08:32:24, fwereade wrote:
> Similarly, I think this should be in export_test until we add a data return to
> Status()

Done.

https://codereview.appspot.com/13430044/diff/7001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/13430044/diff/7001/state/unit_test.go#newcode300
state/unit_test.go:300: c.Assert(data["remote-unit"], gc.Equals, "unit-mysql-0")
On 2013/09/19 08:32:24, fwereade wrote:
> Similarly, it would be good to beef these tests up a bit as suggested for
> machine.

Done.

https://codereview.appspot.com/13430044/diff/7001/worker/uniter/modes.go
File worker/uniter/modes.go (left):

https://codereview.appspot.com/13430044/diff/7001/worker/uniter/modes.go#oldc...
worker/uniter/modes.go:341: msg := fmt.Sprintf("hook failed: %q", u.s.Hook.Kind)
On 2013/09/19 08:32:24, fwereade wrote:
> The Kind here is inadequate, I think. We really ought to name the hook, which
> will take a small amount of looking up the relation. The relation should
always
> be known to the uniter if we're at this point, so it should be trivial to
figure
> out the actual hook name (as we do in the hook runner -- it may be smart to
put
> a helper method on uniter that can turn a hook.Info into an actual hook
name?).

Done.

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

https://codereview.appspot.com/13430044/diff/7001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:346: "remote-unit": u.s.Hook.RemoteUnit,
On 2013/09/19 08:32:24, fwereade wrote:
> I think we always want:
> 
>     "hook": <full-hook-name-as-described-above>,
> 
> ...and that if it's a relation hook (RelationId != -1) we also want:
> 
>     "relation-id": <id>,
> 
> ...and that if the remote unit is not "", we also want:
> 
>     "remote-unit": <name>,

Done.

https://codereview.appspot.com/13430044/diff/7001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:348: if err = u.unit.SetStatus(params.StatusError, msg,
data); err != nil {
On 2013/09/19 08:32:24, fwereade wrote:
> ...and, hey, I don't think this bit is tested.
> 
> I'd be more than happy to *not* change the functionality in this CL, though --
> I'd just drop this change, get this one in as soon as possible, and then worry
> about piping the data through.

Done.

https://codereview.appspot.com/13430044/diff/7001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:387: // TODO(mue) Add helpful data here too.
On 2013/09/19 08:32:24, fwereade wrote:
> Great idea. Make it another CL though please.

Done.

[fwereade, 2013-09-24 15:06:28]

LGTM if you (1) tidy up the data that gets set in the tests so it's either
accurate or plainly nonsense, instead of something that looks sort of right but
is actually not realistic; and (2) add state tests for data-dict overwriting.

No need to extract CurrentHookName into its own CL, but do unexport it please.

https://codereview.appspot.com/13430044/diff/14001/state/megawatcher_internal...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/13430044/diff/14001/state/megawatcher_internal...
state/megawatcher_internal_test.go:667: "remote-unit": "unit-mysql-0",
Hmm. I feel like this probably ought to be a name.

https://codereview.appspot.com/13430044/diff/14001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/13430044/diff/14001/state/unit_test.go#newcode318
state/unit_test.go:318: "hook-kind":   "relation-joined",
I think this should be a name, not a kind. If I said otherwise before I'm an
idiot and I'm sorry.

https://codereview.appspot.com/13430044/diff/14001/state/unit_test.go#newcode320
state/unit_test.go:320: "remote-unit": "unit-mysql-0",
Idon't think this should be a tag, please fix the places it is.

https://codereview.appspot.com/13430044/diff/14001/state/unit_test.go#newcode360
state/unit_test.go:360: }
I don't see any tests for setting nil over the top of an existing dict, or
changing dict contents and avoiding cross-contamination.

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

https://codereview.appspot.com/13430044/diff/14001/worker/uniter/modes.go#new...
worker/uniter/modes.go:350: // TODO(mue) Add data in next CL together with
testing.
Delete this code please -- add it with the next CL. At the moment it's just
dead. But it is exactly what I'm looking for in the next CL :).

https://codereview.appspot.com/13430044/diff/14001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/13430044/diff/14001/worker/uniter/uniter.go#ne...
worker/uniter/uniter.go:386: func (u *Uniter) CurrentHookName() string {
This shouldn't be exported. It's only safe to call from mode funcs.

And it's nice, but this and the uniter change really make a perfect CL on their
own. If you spot opportunities to extract micro-CLs like this one please do so
:).

[mue, 2013-09-25 10:34:28]

Please take a look.

https://codereview.appspot.com/13430044/diff/14001/state/megawatcher_internal...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/13430044/diff/14001/state/megawatcher_internal...
state/megawatcher_internal_test.go:667: "remote-unit": "unit-mysql-0",
On 2013/09/24 15:06:29, fwereade wrote:
> Hmm. I feel like this probably ought to be a name.

Done.

https://codereview.appspot.com/13430044/diff/14001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/13430044/diff/14001/state/unit_test.go#newcode318
state/unit_test.go:318: "hook-kind":   "relation-joined",
On 2013/09/24 15:06:29, fwereade wrote:
> I think this should be a name, not a kind. If I said otherwise before I'm an
> idiot and I'm sorry.

Done.

https://codereview.appspot.com/13430044/diff/14001/state/unit_test.go#newcode320
state/unit_test.go:320: "remote-unit": "unit-mysql-0",
On 2013/09/24 15:06:29, fwereade wrote:
> Idon't think this should be a tag, please fix the places it is.

Done.

https://codereview.appspot.com/13430044/diff/14001/state/unit_test.go#newcode360
state/unit_test.go:360: }
On 2013/09/24 15:06:29, fwereade wrote:
> I don't see any tests for setting nil over the top of an existing dict, or
> changing dict contents and avoiding cross-contamination.

Done.

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

https://codereview.appspot.com/13430044/diff/14001/worker/uniter/modes.go#new...
worker/uniter/modes.go:350: // TODO(mue) Add data in next CL together with
testing.
On 2013/09/24 15:06:29, fwereade wrote:
> Delete this code please -- add it with the next CL. At the moment it's just
> dead. But it is exactly what I'm looking for in the next CL :).

Done.

https://codereview.appspot.com/13430044/diff/14001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/13430044/diff/14001/worker/uniter/uniter.go#ne...
worker/uniter/uniter.go:386: func (u *Uniter) CurrentHookName() string {
On 2013/09/24 15:06:29, fwereade wrote:
> This shouldn't be exported. It's only safe to call from mode funcs.
> 
> And it's nice, but this and the uniter change really make a perfect CL on
their
> own. If you spot opportunities to extract micro-CLs like this one please do so
> :).

Done.