Merge lp://staging/~rogpeppe/juju-core/293-status-life into lp://staging/~juju/juju-core/trunk

Reviewers: mp+159617_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: implement life in status

https://code.launchpad.net/~rogpeppe/juju-core/293-status-life/+merge/159617

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8852043/

Affected files:
   A [revision details]
   M cmd/juju/status.go
   M cmd/juju/status_test.go

LGTM, although please merge trunk and change Series in machineStatus as
agreed (not omitempty).

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

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode137
cmd/juju/status.go:137: status.Life,
did gofmt do that? it looks surprisingly ugly..

https://codereview.appspot.com/8852043/

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go
File cmd/juju/status.go (left):

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#oldcode256
cmd/juju/status.go:256: if t, err := entity.AgentTools(); err == nil {
This should be a `, ok`, shouldn't it. Ah well, not for today.

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

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode140
cmd/juju/status.go:140: status.AgentStateInfo,
I don't really feel this is a win over the out params.

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode146
cmd/juju/status.go:146: status.InstanceId = instance.Id()
Why don't we set the instance id in the other branch?

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode157
cmd/juju/status.go:157: // if the instance hasn't even started yet. This
There is no instance. "if no instance has been provisioned" would be
correct, I think.

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode158
cmd/juju/status.go:158: // also gives a visually distinction to
unprovisioned machines
also makes unprovisioned machines visually distinct in the output.

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode275
cmd/juju/status.go:275: return
Why hide errors?

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode279
cmd/juju/status.go:279: // in enquiring about the agent liveness.
"We don't bother to check agent liveness, even though that is useful and
relevant information, because we haven't explicitly agreed what we
should do with it."

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode284
cmd/juju/status.go:284: return
I'd rather see 100x "watcher is dying" errors than to just skip it
entirely.

https://codereview.appspot.com/8852043/

Please take a look.

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

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode140
cmd/juju/status.go:140: status.AgentStateInfo,
On 2013/04/18 12:37:09, fwereade wrote:
> I don't really feel this is a win over the out params.

i think it wins *just*
better would be to have an embedded struct, but i'm not sure goyaml
supports that currently. this is one step towards that.

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode146
cmd/juju/status.go:146: status.InstanceId = instance.Id()
On 2013/04/18 12:37:09, fwereade wrote:
> Why don't we set the instance id in the other branch?

Done.

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode157
cmd/juju/status.go:157: // if the instance hasn't even started yet. This
On 2013/04/18 12:37:09, fwereade wrote:
> There is no instance. "if no instance has been provisioned" would be
correct, I
> think.

Done.

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode158
cmd/juju/status.go:158: // also gives a visually distinction to
unprovisioned machines
On 2013/04/18 12:37:09, fwereade wrote:
> also makes unprovisioned machines visually distinct in the output.

Done.

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode275
cmd/juju/status.go:275: return
On 2013/04/18 12:37:09, fwereade wrote:
> Why hide errors?

we don't. the error is returned.

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode279
cmd/juju/status.go:279: // in enquiring about the agent liveness.
On 2013/04/18 12:37:09, fwereade wrote:
> "We don't bother to check agent liveness, even though that is useful
and
> relevant information, because we haven't explicitly agreed what we
should do
> with it."

is it really useful and relevant?
if the status is pending, will we ever see a live agent, given that
changing the status is presumably the first thing an agent actually
does? (and if it doesn't, then it jolly well should)

https://codereview.appspot.com/8852043/diff/1/cmd/juju/status.go#newcode284
cmd/juju/status.go:284: return
On 2013/04/18 12:37:09, fwereade wrote:
> I'd rather see 100x "watcher is dying" errors than to just skip it
entirely.

we're not skipping it. it gets returned and put into the entity Err
field.

https://codereview.appspot.com/8852043/

Please add life to services/relations (obscuring Alive as for
units/machines). With that, LGTM.

https://codereview.appspot.com/8852043/

*** Submitted:

cmd/juju: implement life in status

R=dimitern, fwereade
CC=
https://codereview.appspot.com/8852043

https://codereview.appspot.com/8852043/