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

As it stands, this does not lgtm. I think this needs to happen inside
state, based on the contents of state, at unit assignment time, and in a
txn that checks against the current service networks -- doing it that
way should make it possible to correctly handle both deploy and
add-unit.

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#newcode170
environs/interface.go:170: ValidateNetworksForInstance(includedNetworks,
excludedNetworks []string, instance_id string) error
Not 100%sure about this direction -- I'd rather not talk to the provider
if we have the info available in state. Dimitern, what's the status of
the pipeline that sets actual networks on machines?

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) {
Needs documentation, intent is not clear from the name IMO.

https://codereview.appspot.com/84880045/diff/1/juju/deploy.go#newcode49
juju/deploy.go:49: if containerType, err =
instance.ParseContainerType(firstPart); err == nil {
You surely didn't have the context to know this, but this is *not* a
container type -- the stuff in --to is a "placement directive", of which
the only one implemented is "lxc".

Haven't looked further; so, if this is consolidating the couple of bits
of code that already do this sort of thing, +1, because it reduces the
number of places we need to fix as we do placement directives properly;
if it's further duplication, -1.

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#newcode85
provider/maas/environ.go:85: return nil
This needs to return an error; but I have a pretty serious problem with
the existence of this exported method in the first place. We really
shouldn't be casting Environs like this...

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode572
provider/maas/environ.go:572: if len(maasObj_list) == 0 {
!= 1?

but... again, I don't think this should be an environ method anyway.

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#newcode315
state/apiserver/client/client.go:315: err =
env.ValidateNetworksForInstance(args.IncludeNetworks,
args.ExcludeNetworks, mid)
If I were happy with the environ method, I'd still rather keep this
check inside state, so we can keep ensuring db integrity as networks
become dynamic -- not directly relevant for *deploy*, I guess, but
trying to do the same job in different layers at different times should
generally be avoided ;).

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