If you dropped the changes to cmd/juju I'd approve this (with just a
little bit of extra checking of the values when you parse network
constraints).


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

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode95
cmd/juju/deploy.go:95: juju deploy mysql --networks=storage,mynet
--constraints ^logging,db
--constraints networks=^logging,db

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode191
cmd/juju/deploy.go:191: includeNetworks :=
mergeNetworks(parseNetworks(c.Networks),
c.Constraints.IncludeNetworks())
This is definitely wrong. This'd put the ones in constraints into the
set that got configured on the machine, instead of the ones used *only*
for machine *selection*, as they should be.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode192
cmd/juju/deploy.go:192: excludeNetworks := mergeNetworks(nil,
c.Constraints.ExcludeNetworks())
And I'm pretty sure this is wrong too. The information's already in
constraints and doesn't need to be duplicated.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode195
cmd/juju/deploy.go:195: env, err := environs.New(conf)
I'm a bit concerned about this. I know we currently need the environ to
talk to the charm store, but I think this will change as we move the
charm store into the environment; I'd prefer not to add more
dependencies on it at this layer (apart from everything else, we can't
assume that the average user has permission to look at the env config).
Just do the checking in the API server (which you have to do anyway, you
might get any request from the GUI/other client).

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode240
cmd/juju/deploy.go:240: excludeNetworks,
includeNetworks could stay, if it had the right information in it, but
I'm certain we don't want this. If we haven't released a stable version
with --include/exclude-networks, we should just drop this field. What
are dev versions for, if not for making mistakes? ;p

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode297
cmd/juju/deploy.go:297: parts := strings.Split(networksValue, ",")
cmd.StringsValue

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode314
cmd/juju/deploy.go:314: // It's not a tag, convert it.
We should *always* turn the user's strings into tags. If they try to
specify network-foo, send network-network-foo over the API.

https://codereview.appspot.com/102830044/diff/1/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/102830044/diff/1/constraints/constraints.go#newcode514
constraints/constraints.go:514: return &items, nil
I'm wondering whether we should be restricting the values here, at least
in the networks case. Remember these are meant to refer to juju network
names, not provider ones (where we can't control validity).

Tags may be a bit different; not sure if we can sanely restrict them,
even if we'll get surprising results if they ever contain ",", but I
think we can and should keep network names locked down.

https://codereview.appspot.com/102830044/diff/1/state/constraints.go
File state/constraints.go (right):

https://codereview.appspot.com/102830044/diff/1/state/constraints.go#newcode55
state/constraints.go:55: InstanceType: cons.InstanceType,
Please make the order of fields here (and above) consistent with the
order in the struct definition.

https://codereview.appspot.com/102830044/diff/1/state/constraintsvalidation_test.go
File state/constraintsvalidation_test.go (right):

https://codereview.appspot.com/102830044/diff/1/state/constraintsvalidation_test.go#newcode43
state/constraintsvalidation_test.go:43: }{{
Thanks :).

https://codereview.appspot.com/102830044/