cmd/juju: implement life in status

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)

[rog, 2013-04-18 12:33:55]

Please take a look.

[dimitern, 2013-04-18 12:34:33]

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..

[fwereade, 2013-04-18 12:37:09]

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.

[rog, 2013-04-18 12:58:01]

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.

[fwereade, 2013-04-18 13:26:08]

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

[rog, 2013-04-18 14:34:30]

*** Submitted:

cmd/juju: implement life in status

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