Add status cmd to api with state access fallback

Add status cmd to api with state access fallback

Current progress on having `juju status` use the api. Moves the
existing code to a resurrected state/statecmd file. This is
exposed both through the api, and as a fallback to accessing
state directly.

The status tests now pass, but moving some testing down a layer
or two would probably be a good plan.

Is there anything else required for a status first pass?

https://code.launchpad.net/~gz/juju-core/status_api/+merge/198444

(do not edit description out of merge proposal)

[rog, 2013-12-10 18:54:03]

This looks great in general. I'm not keen on the duplicated data structures
though. I'll take a look at the test failure.

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

https://codereview.appspot.com/40350043/diff/1/cmd/juju/status.go#newcode213
cmd/juju/status.go:213: func formatStatus(status *api.Status) formattedStatus {
I don't really see why we can't just use api.Status directly here.
Maintaining two versions of exactly the same thing seems to me to be an
unnecessary maintenance burden. Why can't we just add yaml tags to
the api status types and keep things simple?

As a (non-Go-based) client, I might well want to use the same code either to
parse a status from the Client.Status call or from calling juju status
--format=json.

https://codereview.appspot.com/40350043/diff/1/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/40350043/diff/1/environs/jujutest/livetests.go...
environs/jujutest/livetests.go:447: status, err :=
apiConn.State.Client().Status([]string{})
s/[]string{}/nil/ ?

https://codereview.appspot.com/40350043/diff/1/state/statecmd/status.go
File state/statecmd/status.go (right):

https://codereview.appspot.com/40350043/diff/1/state/statecmd/status.go#newco...
state/statecmd/status.go:24: func Status(conn *juju.Conn, patterns []string)
(api.Status, error) {
I'm presuming that almost no logic in this file has changed from the original,
so I'm skipping over it in my review. Please advise if that's not the case.

[gz, 2013-12-10 19:01:34]

Thanks! Any ideas on my test issue would be greatly appreciated.

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

https://codereview.appspot.com/40350043/diff/1/cmd/juju/status.go#newcode213
cmd/juju/status.go:213: func formatStatus(status *api.Status) formattedStatus {
On 2013/12/10 18:54:03, rog wrote:
> I don't really see why we can't just use api.Status directly here.
> Maintaining two versions of exactly the same thing seems to me to be an
> unnecessary maintenance burden. Why can't we just add yaml tags to
> the api status types and keep things simple?

That was my initial version, what I don't like is it's not just the json/yaml
annotations, there are also some custom methods on the structs I don't fully
understand, so split it out from the api copies for now.

My other reasoning is I would quite like to have the formatting not tied to what
the api exactly returns, so things like adding addresses can be done without
breakage on all sides.

I'm still after better ideas.

https://codereview.appspot.com/40350043/diff/1/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/40350043/diff/1/environs/jujutest/livetests.go...
environs/jujutest/livetests.go:447: status, err :=
apiConn.State.Client().Status([]string{})
On 2013/12/10 18:54:03, rog wrote:
> s/[]string{}/nil/ ?

Okay.

https://codereview.appspot.com/40350043/diff/1/state/statecmd/status.go
File state/statecmd/status.go (right):

https://codereview.appspot.com/40350043/diff/1/state/statecmd/status.go#newco...
state/statecmd/status.go:24: func Status(conn *juju.Conn, patterns []string)
(api.Status, error) {
On 2013/12/10 18:54:03, rog wrote:
> I'm presuming that almost no logic in this file has changed from the original,
> so I'm skipping over it in my review. Please advise if that's not the case.

That's the case. There are some minimal changes in this Status method, and some
renamings, but otherwise it should be exactly the current code.

[gz, 2013-12-10 20:48:46]

Please take a look.

[gz, 2013-12-10 20:53:29]

Please take a look.

[rog, 2013-12-10 21:44:25]

LGTM modulo the suggestions below.

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

https://codereview.appspot.com/40350043/diff/40001/cmd/juju/status.go#newcode78
cmd/juju/status.go:78: state, err := statecmd.Status(conn, c.patterns)
Why not make statecmd.Status return *Status? (alternatively make this method
return api.Status rather than *api.Status)

https://codereview.appspot.com/40350043/diff/40001/cmd/juju/status.go#newcode103
cmd/juju/status.go:103: // XXX: no early return, to allow for err+result output?
Why not just return the error here?

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

https://codereview.appspot.com/40350043/diff/40001/cmd/juju/status_test.go#ne...
cmd/juju/status_test.go:105: err = a.WaitAgentAlive(coretesting.LongWait)
This is almost right, but I'm not sure it's quite there.
We're still calling WaitAgentAlive on an object that's
looking at the non-API state presence package, so this
will still be potentially racy, I think.

To do it right, I think we should operate on an
object derived from the API state. One way of
going about this would be to use that State in
context.st .

https://codereview.appspot.com/40350043/diff/40001/cmd/juju/status_test.go#ne...
cmd/juju/status_test.go:1418: ctx.pingers[u.Name()] = ctx.setAgentAlive(c, u)
nice de-duplication of code, thanks.

[gz, 2013-12-11 14:19:13]

Please take a look.

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

https://codereview.appspot.com/40350043/diff/40001/cmd/juju/status.go#newcode78
cmd/juju/status.go:78: state, err := statecmd.Status(conn, c.patterns)
On 2013/12/10 21:44:25, rog wrote:
> Why not make statecmd.Status return *Status? (alternatively make this method
> return api.Status rather than *api.Status)

I fiddled around with these a bit. Basically, I wanted them to match up with
what you get back from the api call, which is a *Status. Should I change that? I
think having them all give the same thing makes life simpler. Maybe changing
statecmd.Status is the right option, I'll do that for now.

https://codereview.appspot.com/40350043/diff/40001/cmd/juju/status.go#newcode103
cmd/juju/status.go:103: // XXX: no early return, to allow for err+result output?
On 2013/12/10 21:44:25, rog wrote:
> Why not just return the error here?

Good XXX catch... So, this is standing in for the old logic which was:

-       context.instances, err = fetchAllInstances(conn.Environ)
-       if err != nil {
-               // We cannot see instances from the environment, but
-               // there's still lots of potentially useful info to print.
-               fmt.Fprintf(ctx.Stderr, "cannot retrieve instances from the
environment: %v\n", err)
-       }

In other words, in only one of the failure cases, show the error and give
partial status output. Seems worth adding a test or something to cover this case
so we know the output remains sane.

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

https://codereview.appspot.com/40350043/diff/40001/cmd/juju/status_test.go#ne...
cmd/juju/status_test.go:105: err = a.WaitAgentAlive(coretesting.LongWait)
On 2013/12/10 21:44:25, rog wrote:
> This is almost right, but I'm not sure it's quite there.
> We're still calling WaitAgentAlive on an object that's
> looking at the non-API state presence package, so this
> will still be potentially racy, I think.
> 
> To do it right, I think we should operate on an
> object derived from the API state. One way of
> going about this would be to use that State in
> context.st .

That appears to work, have made the change.

[fwereade, 2013-12-18 08:35:02]

LGTM

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

https://codereview.appspot.com/40350043/diff/60001/cmd/juju/status.go#newcode97
cmd/juju/status.go:97: "(direct DB access)")
This looks like it might be in common use and benefit from being factored out
into something like warnFallback("Status").