Code review comment for lp://staging/~axwalk/juju-core/lp1167441-report-instance-state

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

LGTM with a couple of suggestions.

https://codereview.appspot.com/12333043/diff/1/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12333043/diff/1/container/lxc/lxc_test.go#newcode130
container/lxc/lxc_test.go:130: c.Assert(instance.State(), gc.Equals,
string(golxc.StateUnknown))
This is a bit confusing.. should it be "stopped" or something instead?

https://codereview.appspot.com/12333043/diff/1/environs/azure/instance_test.go
File environs/azure/instance_test.go (right):

https://codereview.appspot.com/12333043/diff/1/environs/azure/instance_test.go#newcode28
environs/azure/instance_test.go:28: func (*InstanceSuite) TestId(c *C) {
Thanks for fixing this. Maybe while you're at it, make it instanceState
instead - no need to be exported.

https://codereview.appspot.com/12333043/diff/1/environs/maas/instance.go
File environs/maas/instance.go (right):

https://codereview.appspot.com/12333043/diff/1/environs/maas/instance.go#newcode26
environs/maas/instance.go:26: // TODO determine MAAS instance state
On 2013/08/05 07:09:06, axw1 wrote:
> On 2013/08/02 17:30:32, fwereade wrote:
> > Add a bug in LP and note the number here please; and ping bigjools,
or someone
> > else who knows maas, to find out what'll be needed to implement it,
and note
> > that in the bug.

> I had a chat with bigjools on IRC about this. He tells me that the
MAAS server
> does not track the status of nodes after they're allocated, and so any
node that
> juju knows about will always show up as "allocated".

> The TODO should probably be replaced with a note explaining that
status isn't
> available. Sound reasonable?

+1

https://codereview.appspot.com/12333043/

« Back to merge proposal