[maas] Filter out nodes that are not allocated.

[maas] Filter out nodes that are not allocated.

The maas provider should only return instances
that are in the allocated state.

https://code.launchpad.net/~thumper/juju-core/maas-allocated/+merge/191090

(do not edit description out of merge proposal)

[thumper, 2013-10-15 01:58:40]

Please take a look.

[axw, 2013-10-15 05:48:03]

https://codereview.appspot.com/14439060/diff/1/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/14439060/diff/1/provider/maas/environ.go#newco...
provider/maas/environ.go:333: // Preallocate enough spaces, but don't actually
put anything into the
IMHO the code explains itself. However, it's not right: you must leave nil slots
according to the doc comment of Environ.Instances.

https://codereview.appspot.com/14439060/diff/1/provider/maas/environ_test.go
File provider/maas/environ_test.go (left):

https://codereview.appspot.com/14439060/diff/1/provider/maas/environ_test.go#...
provider/maas/environ_test.go:191: func (suite *environSuite)
TestInstancesReturnsErrorIfPartialInstances(c *gc.C) {
This test is testing the wrong result (not your fault, it was already).

[thumper, 2013-11-04 01:21:17]

Please take a look.

https://codereview.appspot.com/14439060/diff/1/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/14439060/diff/1/provider/maas/environ.go#newco...
provider/maas/environ.go:333: // Preallocate enough spaces, but don't actually
put anything into the
On 2013/10/15 05:48:04, axw wrote:
> IMHO the code explains itself. However, it's not right: you must leave nil
slots
> according to the doc comment of Environ.Instances.

Changed behaviour to match expectations from the the interface comment.

https://codereview.appspot.com/14439060/diff/1/provider/maas/environ_test.go
File provider/maas/environ_test.go (left):

https://codereview.appspot.com/14439060/diff/1/provider/maas/environ_test.go#...
provider/maas/environ_test.go:191: func (suite *environSuite)
TestInstancesReturnsErrorIfPartialInstances(c *gc.C) {
On 2013/10/15 05:48:04, axw wrote:
> This test is testing the wrong result (not your fault, it was already).

Bugger! Fixing.

[axw, 2013-11-04 01:36:02]

Thanks, LGTM. One possible improvement, but fairly minor.

https://codereview.appspot.com/14439060/diff/5001/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/14439060/diff/5001/provider/maas/environ.go#ne...
provider/maas/environ.go:388: return nil, environs.ErrNoInstances
This could be made a bit nicer, but it's not that big a deal if you don't do it:

if len(instances)==0, return ErrNoInstances (you can check this up front).
Similarly, if len(instances)<len(ids), then environs.ErrPartialInstances. Then
you can do away with allNil, and errResult.

[thumper, 2013-11-04 02:43:47]

Please take a look.

[thumper, 2013-11-04 03:36:40]

Please take a look.

https://codereview.appspot.com/14439060/diff/5001/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/14439060/diff/5001/provider/maas/environ.go#ne...
provider/maas/environ.go:388: return nil, environs.ErrNoInstances
On 2013/11/04 01:36:02, axw wrote:
> This could be made a bit nicer, but it's not that big a deal if you don't do
it:
> 
> if len(instances)==0, return ErrNoInstances (you can check this up front).
> Similarly, if len(instances)<len(ids), then environs.ErrPartialInstances. Then
> you can do away with allNil, and errResult.

good call, making changes