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

Revision history for this message
William Reade (fwereade) wrote :

LGTM, this is great. I'd love to see a followup for the local provider,
that should be quick and easy.

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

https://codereview.appspot.com/12333043/diff/1/container/lxc/instance.go#newcode30
container/lxc/instance.go:30: return string(state)
I see this, and it looks good, but I'm a bit confused as to why it
doesn't work with the local provider. (Well, I can see why, but I can't
understand the motivation -- can anyone think of a drawback to putting
the lxc.Instances into the local.Instances so that we can access this
method? I can understand differences in address handling, but I don't
think the same forces apply here.)

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

https://codereview.appspot.com/12333043/diff/1/environs/local/instance.go#newcode27
environs/local/instance.go:27: return ""
I'd like to do what I suggested earlier and expose the lxc instance
state here. But please do it in a followup, and run it by thumper first,
because if there's a reason not to do it he'll know what it is.

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
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.

https://codereview.appspot.com/12333043/diff/1/environs/polling_test.go
File environs/polling_test.go (left):

https://codereview.appspot.com/12333043/diff/1/environs/polling_test.go#oldcode63
environs/polling_test.go:63: func (*dnsNameFakeInstance) Ports(string)
([]instance.Port, error) { return nil, nil }
Complete side note: would it be useful for dnsNameFakeInstance to just
embed an instance.Instance, and thus panic on use of unwanted methods?
Feels a bit saner than just returning nonsense without error...

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

« Back to merge proposal