state, cmd/juju: unit status changes, juju status

state, cmd/juju: unit status changes, juju status

Unit.Status() was changed not to return an error
and not to use AgentAlive() logic inside it, since
this is only meaningful inside juju status command
(the command was updated with the new correct logic).

Also the juju status tests were completely refactored
to be independent and more flexible. This is especially
important for the upcoming changes to the status cmd
to make it compatible with py-juju and will make adding
extra features and testing them easier.

https://code.launchpad.net/~dimitern/juju-core/020-state-fix-unit-status-juju-status/+merge/156181

(do not edit description out of merge proposal)

[dimitern, 2013-03-29 15:16:07]

Please take a look.

[fwereade, 2013-03-29 17:02:04]

This is awesome, thank you very much. I'd like one change to the tests, which I
think will enhance readability quite a bit (on top of everything else you've
already done :)).

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

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status.go#newcode231
cmd/juju/status.go:231: r["status"] = status
Shouldn't this be "agent-state", to match python as far as possible?

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status.go#newcode233
cmd/juju/status.go:233: r["status-info"] = info
agent-state-info

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

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status_test.go#newcode35
cmd/juju/status_test.go:35: func (s *StatusSuite) SetUpSuite(c *C) {
Just drop these 4 methods, you don't need them if you're not overriding

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status_test.go#newcode56
cmd/juju/status_test.go:56: output  M
It is most gratifying to see the uniter testing style replicated, and it looks
like a good fit, which is awesome.

Just one suggestion, spread through the file, which I think will make the test
cases read better: drop the M from here...

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status_test.go#newcode104
cmd/juju/status_test.go:104: M{
...specify all these as `expect{M{...}},` instead of just plain `M{...}`...

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status_test.go#newcode118
cmd/juju/status_test.go:118: addMachine{"0", state.JobManageEnviron},
...but put the expect steps after the setup steps...

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status_test.go#newcode410
cmd/juju/status_test.go:410: // Run command with the required format.
...and then pull everything from here down into:

type expect struct {
    output M
}

func (e expect) step(c *C, ctx *context) {
    // as below, but s/t.output/expect.output/
}

[dimitern, 2013-03-29 18:39:56]

Please take a look.

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

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status.go#newcode231
cmd/juju/status.go:231: r["status"] = status
On 2013/03/29 17:02:04, fwereade wrote:
> Shouldn't this be "agent-state", to match python as far as possible?

Done.

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status.go#newcode233
cmd/juju/status.go:233: r["status-info"] = info
On 2013/03/29 17:02:04, fwereade wrote:
> agent-state-info

Done.

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

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status_test.go#newcode35
cmd/juju/status_test.go:35: func (s *StatusSuite) SetUpSuite(c *C) {
On 2013/03/29 17:02:04, fwereade wrote:
> Just drop these 4 methods, you don't need them if you're not overriding

Done.

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status_test.go#newcode56
cmd/juju/status_test.go:56: output  M
On 2013/03/29 17:02:04, fwereade wrote:
> It is most gratifying to see the uniter testing style replicated, and it looks
> like a good fit, which is awesome.
> 
> Just one suggestion, spread through the file, which I think will make the test
> cases read better: drop the M from here...

Done.

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status_test.go#newcode104
cmd/juju/status_test.go:104: M{
On 2013/03/29 17:02:04, fwereade wrote:
> ...specify all these as `expect{M{...}},` instead of just plain `M{...}`...

Awesome idea, thanks! I was wondering how to improve this.

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status_test.go#newcode118
cmd/juju/status_test.go:118: addMachine{"0", state.JobManageEnviron},
On 2013/03/29 17:02:04, fwereade wrote:
> ...but put the expect steps after the setup steps...

Done.

https://codereview.appspot.com/8133045/diff/1/cmd/juju/status_test.go#newcode410
cmd/juju/status_test.go:410: // Run command with the required format.
On 2013/03/29 17:02:04, fwereade wrote:
> ...and then pull everything from here down into:
> 
> type expect struct {
>     output M
> }
> 
> func (e expect) step(c *C, ctx *context) {
>     // as below, but s/t.output/expect.output/
> }

Done.

[fwereade, 2013-03-29 18:56:29]

As stated online, I think that we could collapse the tests still further and
keep them readable by pulling the various repeated Ms out into vars. But this is
a great improvement already, so I'll leave that tweak to your judgment. So,
LGTM.

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

https://codereview.appspot.com/8133045/diff/6001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:100: expect{
I'd be fine rolling this one into the next test.

[dimitern, 2013-03-29 19:10:44]

Please take a look.

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

https://codereview.appspot.com/8133045/diff/6001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:100: expect{
On 2013/03/29 18:56:29, fwereade wrote:
> I'd be fine rolling this one into the next test.

Done.

[jameinel, 2013-03-31 12:03:08]

LGTM

Most of the patch seems just straightforward and mechanical of removing the
AgentAlive call internally (and thus the code "cannot fail").

I have a question in the tests, but it is more of a "why do it this way?" rather
than something that needs changing.

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

https://codereview.appspot.com/8133045/diff/12001/cmd/juju/status_test.go#new...
cmd/juju/status_test.go:22: func runStatus(c *C, args ...string) (code int,
stderr, stdout []byte) {
Offhand the ordering "code, stderr, stdout" looks a bit odd. It is fine if you
like it, but maybe code,out,err or out,err,code might be more obvious. (It is
just rare to have (stderr, stdout) rather than (stdout, stderr).

https://codereview.appspot.com/8133045/diff/12001/cmd/juju/status_test.go#new...
cmd/juju/status_test.go:271: 
I'm trusting that the above is mechanical. If there is a piece you specifically
want reviewed, just point it out. But it *looks* like just
s/map[string]interface{}/M/ which is great.

https://codereview.appspot.com/8133045/diff/12001/cmd/juju/status_test.go#new...
cmd/juju/status_test.go:441: }
I don't quite understand why these are started in a goroutine, only to have it
then work in lock-step because of the start channel.
It doesn't seem to add anything other than indirection vs just having the loop
inline with no goroutine.
Maybe it allows for some parallelism if GOMAXPROCS >= 1. However, it makes it a
bit hard to figure out what is going on. (Imagine both formats failing because
of a new attribute that ends up in both outputs, and then you have to unnest the
failing goroutine tracebacks, etc.)

Anyway, it isn't something you have to take out, but I'd like to understand why
it was useful.

[dimitern, 2013-04-01 09:12:23]

*** Submitted:

state, cmd/juju: unit status changes, juju status

Unit.Status() was changed not to return an error
and not to use AgentAlive() logic inside it, since
this is only meaningful inside juju status command
(the command was updated with the new correct logic).

Also the juju status tests were completely refactored
to be independent and more flexible. This is especially
important for the upcoming changes to the status cmd
to make it compatible with py-juju and will make adding
extra features and testing them easier.

R=fwereade, jameinel
CC=
https://codereview.appspot.com/8133045

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

https://codereview.appspot.com/8133045/diff/12001/cmd/juju/status_test.go#new...
cmd/juju/status_test.go:22: func runStatus(c *C, args ...string) (code int,
stderr, stdout []byte) {
On 2013/03/31 12:03:09, jameinel wrote:
> Offhand the ordering "code, stderr, stdout" looks a bit odd. It is fine if you
> like it, but maybe code,out,err or out,err,code might be more obvious. (It is
> just rare to have (stderr, stdout) rather than (stdout, stderr).
> 

Good point, done.

https://codereview.appspot.com/8133045/diff/12001/cmd/juju/status_test.go#new...
cmd/juju/status_test.go:271: 
On 2013/03/31 12:03:09, jameinel wrote:
> I'm trusting that the above is mechanical. If there is a piece you
specifically
> want reviewed, just point it out. But it *looks* like just
> s/map[string]interface{}/M/ which is great.

It mostly is, the only new test is the last case with agent-state "down", which
is now reported by the command, rather than the unit itself. Also renamed
"status", "info" -> "agent-state", "agent-state-info" to keep compatibility with
py-juju.

https://codereview.appspot.com/8133045/diff/12001/cmd/juju/status_test.go#new...
cmd/juju/status_test.go:441: }
On 2013/03/31 12:03:09, jameinel wrote:
> I don't quite understand why these are started in a goroutine, only to have it
> then work in lock-step because of the start channel.
> It doesn't seem to add anything other than indirection vs just having the loop
> inline with no goroutine.
> Maybe it allows for some parallelism if GOMAXPROCS >= 1. However, it makes it
a
> bit hard to figure out what is going on. (Imagine both formats failing because
> of a new attribute that ends up in both outputs, and then you have to unnest
the
> failing goroutine tracebacks, etc.)
> 
> Anyway, it isn't something you have to take out, but I'd like to understand
why
> it was useful.

The idea here is to test the already established scenario in state (previous
steps) with all supported formats concurrently. It was meant as a speed
improvement, but now I removed it, because the time difference between
sequential and concurrent approach as tested now is negligible.