Sorry, what I meant by "follow-up" is actually a prerequisite:
https://codereview.appspot.com/93670046

What I've said below is done, it's there.

I'll repropose this later with the changes to cmd/juju mostly.


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
On 2014/05/29 18:22:36, fwereade wrote:
> --constraints networks=^logging,db

Done in a follow-up.

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())
On 2014/05/29 18:22:36, fwereade wrote:
> 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.

Taking your suggestion, I'm dropping cmd/juju changes from this branch
and will propose it as a follow-up, fixing these issues.

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

Done in a follow-up.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode195
cmd/juju/deploy.go:195: env, err := environs.New(conf)
On 2014/05/29 18:22:36, fwereade wrote:
> 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).

Check the follow-up. Doing this check here, in addition to a check in
the API server (sorry, I thought I had that there already - will add it
there as well) will allow us to bail out early, before the API call with
a nicer CLI-specific message, like "cannot use --networks: not supported
by the environmen" instead of "cannot deploy "wordpress" with networks:
not supported by the environment". I'm with having only the latter, less
specific error message.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode240
cmd/juju/deploy.go:240: excludeNetworks,
On 2014/05/29 18:22:36, fwereade wrote:
> 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

Good point. In the follow-up, I'll drop that and modify
service.Networks() to return include/excludeNetworks as before, but
taking both requested networks and constraints into account. That's the
least painful way to do it I found.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode297
cmd/juju/deploy.go:297: parts := strings.Split(networksValue, ",")
On 2014/05/29 18:22:36, fwereade wrote:
> cmd.StringsValue

Didn't know about this one! Will use it in the follow-up, thanks!

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

The complication here is because I'm using it to process both networks
coming from constraints and the argument. The whole point is to always
send tags yes, but I'll simplify it in the follow-up.

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
On 2014/05/29 18:22:36, fwereade wrote:
> 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.

Added validation for network names and tests.

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,
On 2014/05/29 18:22:36, fwereade wrote:
> Please make the order of fields here (and above) consistent with the
order in
> the struct definition.

Done.

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