cmd/juju: Add --(exclude-)networks args to deploy

cmd/juju: Add --(exclude-)networks args to deploy

Implemented two new arguments to juju deploy:
--networks and --exclude-networks. They allow specifying
networks to enable/disable when deploying new machines
hosting units of the service.

If the API server is older than 1.17.7 and does not
support ServiceDeployWithNetworks, and networks are
specified, an error is returned. Otherwise (without
networks specified), the command falls back to using
ServiceDeploy API.

https://code.launchpad.net/~dimitern/juju-core/370-cli-deploy-networks/+merge/212860

Requires: https://code.launchpad.net/~dimitern/juju-core/350-api-service-deploy-with-networks/+merge/211773

(do not edit description out of merge proposal)

[dimitern, 2014-03-26 14:43:59]

Please take a look.

[fwereade, 2014-03-26 15:48:29]

Couple of questions

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

https://codereview.appspot.com/80600044/diff/1/cmd/juju/deploy.go#newcode70
cmd/juju/deploy.go:70: --no-networks. Only supported when using the MaaS
provider.
How do we enforce this? I hope we don't let mis-specification of networks bleed
through into provisioning errors...

https://codereview.appspot.com/80600044/diff/1/cmd/juju/deploy.go#newcode217
cmd/juju/deploy.go:217: ctx.Infof("ignoring --networks/--no-networks as it is
not supported by the API server")
I think we should fail here if any network was specified.

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

https://codereview.appspot.com/80600044/diff/1/cmd/juju/deploy_test.go#newcod...
cmd/juju/deploy_test.go:169: c.Assert(excludeNetworks, gc.DeepEquals,
[]string{"net3", "net4"})
Didn't jam do some work on mocking at this layer, that we could use to check
against the old behaviour? I'm concerned that we DTRT given all possible valid
version combinations.

[dimitern, 2014-03-28 10:25:20]

This is on hold for a short time until I land a related branch, which will get
integrated with this after that.

My replies below:

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

https://codereview.appspot.com/80600044/diff/1/cmd/juju/deploy.go#newcode70
cmd/juju/deploy.go:70: --no-networks. Only supported when using the MaaS
provider.
On 2014/03/26 15:48:30, fwereade wrote:
> How do we enforce this? I hope we don't let mis-specification of networks
bleed
> through into provisioning errors...

We'll have both early and late checks. I'm working on a separate branch that
will provide EnvironCapability.SupportNetworks() for all providers, so we can
fail early if it's not supported. Later, as we do the provisioning work, we'll
have a separate check to ensure the specified networks indeed exist on the
provider. I'll integrate this CL with the early check as it lands.

We'll still have the possibility of provisioning-time errors when you specify
networks which do not exist, but this can be improved in the future.

https://codereview.appspot.com/80600044/diff/1/cmd/juju/deploy.go#newcode217
cmd/juju/deploy.go:217: ctx.Infof("ignoring --networks/--no-networks as it is
not supported by the API server")
On 2014/03/26 15:48:30, fwereade wrote:
> I think we should fail here if any network was specified.

Will do.

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

https://codereview.appspot.com/80600044/diff/1/cmd/juju/deploy_test.go#newcod...
cmd/juju/deploy_test.go:169: c.Assert(excludeNetworks, gc.DeepEquals,
[]string{"net3", "net4"})
On 2014/03/26 15:48:30, fwereade wrote:
> Didn't jam do some work on mocking at this layer, that we could use to check
> against the old behaviour? I'm concerned that we DTRT given all possible valid
> version combinations.

I'm not aware of that. I'll ask. It's actually surprisingly difficult to mock
that and test it, given so many layers of concrete types.

[dimitern, 2014-03-28 17:34:19]

Please take a look.

[jameinel, 2014-03-31 06:14:00]

I thought the CLI was --networks/--exclude-networks, and I think William had
asked that it be singular? (I feel that we use --constraints, so it should be
plural.)

Otherwise, LGTM.

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

https://codereview.appspot.com/80600044/diff/20001/cmd/juju/deploy.go#newcode71
cmd/juju/deploy.go:71: --no-networks. Only supported when using the MaaS
provider.
I'm not sure if you want to explicitly list only MaaS here or not. You could say
"not supported on all providers."
We can keep it and just make sure to update it as we get support on EC2, etc.
Though *there* it isn't so much that it "must be on all networks" as much as it
is "add these networks to all the machines".

https://codereview.appspot.com/80600044/diff/20001/cmd/juju/deploy.go#newcode80
cmd/juju/deploy.go:80: juju deploy mysql --networks=storage,mynet
--no-networks=logging
I thought we wanted the commandline to be "--networks=X,Y,Z
--exclude-networks=Q"

[dimitern, 2014-03-31 08:54:58]

Please take a look.

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

https://codereview.appspot.com/80600044/diff/20001/cmd/juju/deploy.go#newcode71
cmd/juju/deploy.go:71: --no-networks. Only supported when using the MaaS
provider.
On 2014/03/31 06:14:01, jameinel wrote:
> I'm not sure if you want to explicitly list only MaaS here or not. You could
say
> "not supported on all providers."
> We can keep it and just make sure to update it as we get support on EC2, etc.
> Though *there* it isn't so much that it "must be on all networks" as much as
it
> is "add these networks to all the machines".

Done.

https://codereview.appspot.com/80600044/diff/20001/cmd/juju/deploy.go#newcode80
cmd/juju/deploy.go:80: juju deploy mysql --networks=storage,mynet
--no-networks=logging
On 2014/03/31 06:14:01, jameinel wrote:
> I thought we wanted the commandline to be "--networks=X,Y,Z
> --exclude-networks=Q"

Done.

[dimitern, 2014-03-31 08:59:55]

Please take a look.

[jameinel, 2014-03-31 09:05:45]

LGTM