cmd/juju: simplify status

cmd/juju: simplify status

After all Frank's hard work trying to work with the existing status
structure (thanks!), I still found it very hard to understand what was going on,
and while trying to do so, a cunning plan came to mind which I couldn't
resist.

This is the result.

The tests are unchanged from lp:~themue/juju-core/020-cmd-status-subordinates

https://code.launchpad.net/~rogpeppe/juju-core/292-simpler-status/+merge/159503

(do not edit description out of merge proposal)

[rog, 2013-04-17 21:43:09]

Please take a look.

[fwereade, 2013-04-17 22:04:07]

LGTM, this is very neat.

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

https://codereview.appspot.com/8842043/diff/1/cmd/juju/status.go#newcode78
cmd/juju/status.go:78: insts, err := env.AllInstances()
Zero instances is "sane" given eventual consistency and should probably be
allowed.

https://codereview.appspot.com/8842043/diff/1/cmd/juju/status.go#newcode144
cmd/juju/status.go:144: // find that id.
Not really an error; definitely not one that justifies barfing an error message
in place of all machine info.

https://codereview.appspot.com/8842043/diff/1/cmd/juju/status.go#newcode164
cmd/juju/status.go:164: ch, _, err := service.Charm()
I think you can just get CharmURL here

https://codereview.appspot.com/8842043/diff/1/cmd/juju/status.go#newcode304
cmd/juju/status.go:304: type machineStatusNoMarshal machineStatus
I have a great love for bug tests, which fail when upstream bugs are fixed and
allow us to drop the workarounds.

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

https://codereview.appspot.com/8842043/diff/1/cmd/juju/status_test.go#newcode782
cmd/juju/status_test.go:782: func (sua setUnitsAlive) step(c *C, ctx *context) {
Check we've got at least one test for a unit that's down please. Pretty sure we
do, but still.

https://codereview.appspot.com/8842043/diff/1/cmd/juju/status_test.go#newcode908
cmd/juju/status_test.go:908: func (s *StatusSuite) TestSubordinates(c *C) {
Not sure all the extra test methods give us much value; it's quite nice to have
all the tests in one list, given that the code accommodates that already.

https://codereview.appspot.com/8842043/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/8842043/diff/1/state/service.go#newcode266
state/service.go:266: func (s *Service) IsSubordinate() bool {
comment and test please

[rog, 2013-04-17 22:31:45]

Please take a look.

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

https://codereview.appspot.com/8842043/diff/1/cmd/juju/status.go#newcode78
cmd/juju/status.go:78: insts, err := env.AllInstances()
On 2013/04/17 22:04:08, fwereade wrote:
> Zero instances is "sane" given eventual consistency and should probably be
> allowed.

Done.

https://codereview.appspot.com/8842043/diff/1/cmd/juju/status.go#newcode144
cmd/juju/status.go:144: // find that id.
On 2013/04/17 22:04:08, fwereade wrote:
> Not really an error; definitely not one that justifies barfing an error
message
> in place of all machine info.

Done.

https://codereview.appspot.com/8842043/diff/1/cmd/juju/status.go#newcode164
cmd/juju/status.go:164: ch, _, err := service.Charm()
On 2013/04/17 22:04:08, fwereade wrote:
> I think you can just get CharmURL here

much nicer, yes. done (less round-trips, yay!)

https://codereview.appspot.com/8842043/diff/1/cmd/juju/status.go#newcode304
cmd/juju/status.go:304: type machineStatusNoMarshal machineStatus
On 2013/04/17 22:04:08, fwereade wrote:
> I have a great love for bug tests, which fail when upstream bugs are fixed and
> allow us to drop the workarounds.

upstream bugs *are* in fact fixed - but i don't want to force everyone to update
goyaml at this exact moment.

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

https://codereview.appspot.com/8842043/diff/1/cmd/juju/status_test.go#newcode782
cmd/juju/status_test.go:782: func (sua setUnitsAlive) step(c *C, ctx *context) {
On 2013/04/17 22:04:08, fwereade wrote:
> Check we've got at least one test for a unit that's down please. Pretty sure
we
> do, but still.

there is.

https://codereview.appspot.com/8842043/diff/1/cmd/juju/status_test.go#newcode908
cmd/juju/status_test.go:908: func (s *StatusSuite) TestSubordinates(c *C) {
On 2013/04/17 22:04:08, fwereade wrote:
> Not sure all the extra test methods give us much value; it's quite nice to
have
> all the tests in one list, given that the code accommodates that already.

agreed.
done.

[thumper, 2013-04-17 22:43:47]

LGTM

[fwereade, 2013-04-17 22:49:26]

still LGTM with trivials

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

https://codereview.appspot.com/8842043/diff/10001/cmd/juju/status.go#newcode140
cmd/juju/status.go:140: return
I wanted to say "oo! add m.Series() everywhere" but decided against it, we can
leave that and Life() for dimitern's branch.

https://codereview.appspot.com/8842043/diff/10001/cmd/juju/status.go#newcode146
cmd/juju/status.go:146: // find that id.
Epsilon ungood ;p.

https://codereview.appspot.com/8842043/diff/10001/cmd/juju/status.go#newcode254
cmd/juju/status.go:254: func processAgent(dstVersion *string, dstStatus
*params.Status, dstInfo *string, entity stateAgent) error {
I think these are the only funky out params now, so it'd be nice to put them
after the entity.

https://codereview.appspot.com/8842043/diff/10001/cmd/juju/status.go#newcode302
cmd/juju/status.go:302: return json.Marshal(errorStatus{s.Err.Error()})
Suggestion: add StatusError everywhere there's an Err, call Err err instead for
clarity of marshalling, and output error info alongside what we;ve managed to
get that's valid.

Might be crazy, YMMV, etc.

https://codereview.appspot.com/8842043/diff/14001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/8842043/diff/14001/state/service.go#newcode269
state/service.go:269: return !s.doc.Subordinate
I'd still like a test for this, absurd as that may sound.

[rog, 2013-04-17 22:50:25]

*** Submitted:

cmd/juju: simplify status

After all Frank's hard work trying to work with the existing status
structure (thanks!), I still found it very hard to understand what was going on,
and while trying to do so, a cunning plan came to mind which I couldn't
resist.

This is the result.

The tests are unchanged from lp:~themue/juju-core/020-cmd-status-subordinates

R=fwereade, thumper
CC=
https://codereview.appspot.com/8842043

[TheMue, 2013-04-18 06:50:50]

Nice refactoring, would have been the next step.

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

https://codereview.appspot.com/8842043/diff/14001/cmd/juju/status.go#newcode55
cmd/juju/status.go:55: var ctxt statusContext
What does the postfix "t" stand for? Would prefer "sctx" for a status context.

https://codereview.appspot.com/8842043/diff/14001/cmd/juju/status_test.go
File cmd/juju/status_test.go (right):

https://codereview.appspot.com/8842043/diff/14001/cmd/juju/status_test.go#new...
cmd/juju/status_test.go:536: "one service with one subordinate service",
Oops, has been wrong. Two services with one subordinate service.

[rog, 2013-04-18 07:28:19]

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

https://codereview.appspot.com/8842043/diff/14001/cmd/juju/status.go#newcode55
cmd/juju/status.go:55: var ctxt statusContext
On 2013/04/18 06:50:50, TheMue wrote:
> What does the postfix "t" stand for? Would prefer "sctx" for a status context.

sorry, i've always used "ctxt" as short form for "context". just habit.