Display machine hardware info in status

Display machine hardware info in status

Now that state stores the hardware characteristics of
provisioned machines, we can show that information in
juju status.

https://code.launchpad.net/~wallyworld/juju-core/machine-characteristics-in-status/+merge/171710

Requires: https://code.launchpad.net/~wallyworld/juju-core/instance-metadata-in-state/+merge/170740

(do not edit description out of merge proposal)

[wallyworld, 2013-06-27 04:23:27]

Please take a look.

[dimitern, 2013-06-27 14:43:53]

I'm a bit at a loss about how "hardware" is different from "constraints" in the
output? Shouldn't we just call it "constraints" instead?

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

https://codereview.appspot.com/10667043/diff/1/cmd/juju/status.go#newcode190
cmd/juju/status.go:190: inst, ok := context.instances[instid]
what's wrong with "instance"? :)

https://codereview.appspot.com/10667043/diff/1/cmd/juju/status.go#newcode206
cmd/juju/status.go:206: // if the machine hasn't been provisioned.  This
s/hasn't been provisioned/hasn't been provisioned or is in an error state./ ?

https://codereview.appspot.com/10667043/diff/1/cmd/juju/status.go#newcode209
cmd/juju/status.go:209: status.AgentState = ""
even better, how about status.AgentState = err.Error() in the else case above,
so we can see what the error was?

https://codereview.appspot.com/10667043/diff/1/cmd/juju/status.go#newcode214
cmd/juju/status.go:214: status.Hardware = "error"
how about status.Hardware = fmt.Sprintf("error: %v", err) ?

[wallyworld, 2013-06-27 20:46:49]

Hardware != constraints because the constraints specify the minimum requirements
for an instance; hardware is the actual characteristics. So we could ask for a
1GB machine and end up getting a 4GB instance started because that's what the
cloud offers. We want to record what the instance is actually running with so
that deployment constraints can be used sensibly.

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

https://codereview.appspot.com/10667043/diff/1/cmd/juju/status.go#newcode190
cmd/juju/status.go:190: inst, ok := context.instances[instid]
On 2013/06/27 14:43:53, dimitern wrote:
> what's wrong with "instance"? :)

We are importing a package called instance and so there's a name clash. This was
a drive by fix.

https://codereview.appspot.com/10667043/diff/1/cmd/juju/status.go#newcode206
cmd/juju/status.go:206: // if the machine hasn't been provisioned.  This
On 2013/06/27 14:43:53, dimitern wrote:
> s/hasn't been provisioned/hasn't been provisioned or is in an error state./ ?

I can update the comment

https://codereview.appspot.com/10667043/diff/1/cmd/juju/status.go#newcode209
cmd/juju/status.go:209: status.AgentState = ""
On 2013/06/27 14:43:53, dimitern wrote:
> even better, how about status.AgentState = err.Error() in the else case above,
> so we can see what the error was?

I've left the existing code alone. See the comment below about the possible
future use of a verbose flag.

https://codereview.appspot.com/10667043/diff/1/cmd/juju/status.go#newcode214
cmd/juju/status.go:214: status.Hardware = "error"
On 2013/06/27 14:43:53, dimitern wrote:
> how about status.Hardware = fmt.Sprintf("error: %v", err) ?

The error string can be very long and will mess up the concise output. We don't
expect the error condition often (hopefully not at all). If it happens, logs or
other means can be used to see what's happened. We are also considering a
--verbose option to juju status. We can print the full error if verbose is used.

[thumper, 2013-06-27 23:06:04]

LGTM

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

https://codereview.appspot.com/10667043/diff/1/cmd/juju/status.go#newcode190
cmd/juju/status.go:190: inst, ok := context.instances[instid]
On 2013/06/27 20:46:49, wallyworld wrote:
> On 2013/06/27 14:43:53, dimitern wrote:
> > what's wrong with "instance"? :)
> 
> We are importing a package called instance and so there's a name clash. This
was
> a drive by fix.

This is only a problem if you also want to access something in the instance
package while in the scope of this local variable.

[fwereade, 2013-06-28 12:46:00]

LGTM, nice

[fwereade, 2013-06-28 12:50:09]

On 2013/06/28 12:46:00, fwereade wrote:
> LGTM, nice

...except... I'm concerned it's a bit unkind to inject our own format into
supposed "json" or "yaml". Would you please export hardware characteristics as
dicts? As an ideal, I'd like us to be able to output everything in arbitrary
formats.

It's fine to do this in a followup, so long as we don't do any new releases in
between this landing and the followup doing so. Please coordinate with dave if
you land this as is.

[wallyworld, 2013-06-30 23:44:58]

On 2013/06/28 12:50:09, fwereade wrote:
> On 2013/06/28 12:46:00, fwereade wrote:
> > LGTM, nice
> 
> ...except... I'm concerned it's a bit unkind to inject our own format into
> supposed "json" or "yaml". Would you please export hardware characteristics as
> dicts? As an ideal, I'd like us to be able to output everything in arbitrary
> formats.
> 
> It's fine to do this in a followup, so long as we don't do any new releases in
> between this landing and the followup doing so. Please coordinate with dave if
> you land this as is.

I thought about a dict but wanted the output to be concise (ie all on one line).
I also made the output the same as we do for printing constraints since the
concepts are similar. I'd like to discuss this please. The landing is being done
just after a release so we will have time to bikeshed it before the next one.