Code review comment for lp:~hduran-8/juju-core/add_network_check_on_specified_machine

Good direction, but I have some concerns and suggestions to make it
better.

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

https://codereview.appspot.com/84880045/diff/1/cmd/juju/deploy.go#newcode195
cmd/juju/deploy.go:195:
d

https://codereview.appspot.com/84880045/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/84880045/diff/1/environs/interface.go#newcode168
environs/interface.go:168: // ValidateNetworksForInstance returns nil if
the networks are
// ValidateNetworksForInstance returns and error if the given networks
// are not compatible for the given instance id.

s/instance_id string/instanceId instance.Id/

https://codereview.appspot.com/84880045/diff/1/juju/deploy.go
File juju/deploy.go (right):

https://codereview.appspot.com/84880045/diff/1/juju/deploy.go#newcode37
juju/deploy.go:37: func (parms *DeployServiceParams) ClearMachineSpecs()
(string, instance.ContainerType) {
s/parms/args/ ?

https://codereview.appspot.com/84880045/diff/1/juju/deploy.go#newcode38
juju/deploy.go:38: // TODO: Add this to AddUnit and replace wherever
required
// TODO(hduran-8) ...

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

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode568
provider/maas/environ.go:568: maasObj_list, err :=
e.instances([]instance.Id{instance.Id(instance_id)})
s/maasObj_list/maasObjList/ (no underscores please)

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode574
provider/maas/environ.go:574: return fmt.Errorf("Found unexpected amunt
of instances with the given id")
return fmt.Errorf("expected one result, got %d", len(maasObjList)) ?

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode580
provider/maas/environ.go:580: fmt.Print(networks)
logger.Debugf("got networks from MAAS: %v", networks) ?

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode586
provider/maas/environ.go:586: for _, i_network := range includedNetworks
{
s/i_network/network/

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode588
provider/maas/environ.go:588: fmt.Print(netmap)
logger.Debugf("got networks map from MAAS: %v", netmap)

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode589
provider/maas/environ.go:589: return fmt.Errorf("Ip %q is not present in
the requested unit", i_network)
Why IP ? We're getting network names in the command and we should check
for that in maas result list, not IPs.

return fmt.Errorf("network %q not found for instance %q in MAAS",
network, instanceId)

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode595
provider/maas/environ.go:595: return fmt.Errorf("Ip %q is set up in the
requested unit and can not be excluded", i_network)
Similarly, return fmt.Errorf("network %q is required for instance %q and
cannot be excluded", network, instanceId)

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

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ_whitebox_test.go#newcode549
provider/maas/environ_whitebox_test.go:549:
suite.getInstance("instance_for_network")
Something shorter? Like "i-node" ?

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ_whitebox_test.go#newcode560
provider/maas/environ_whitebox_test.go:560: c.Check(err, gc.DeepEquals,
expected_err)
When you're checking for error returns, better use Assert than Check.

https://codereview.appspot.com/84880045/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/84880045/diff/1/state/apiserver/client/client.go#newcode301
state/apiserver/client/client.go:301: // XXX Get the actual node
This is not right - both ServiceDeploy and ServiceDeployWithNetworks are
doing the same thing in the last release, so we should do these extra
steps inside ServiceDeploy instead.

https://codereview.appspot.com/84880045/