state;constraints: Networks constraints added

state;constraints: Networks constraints added

This adds "networks=..." constraint in the constraints
and state packages. The ability to specify networks
to include/exclude when selecting a machine to deploy
a service's unit with specified constraints. Both
positive and negative entries are supported and the
value of "networks=" is a comma-delimited list of
(juju) network names. Negative entries have "^" prefix.

Examples: networks=logging,^db,storage,^dmz

Means: use machines (if possible) on the "logging" and
"storage" networks and not on "db" or "dmz" networks.

No changes to cmd/juju/deploy or other commands yet,
this will come in a follow-up.

https://code.launchpad.net/~dimitern/juju-core/459-networks-constraints/+merge/221681

(do not edit description out of merge proposal)

[dimitern, 2014-06-02 09:21:34]

Please take a look.

[jameinel, 2014-06-02 10:33:42]

I think the changes LGTM, I have some comments to tweak, though.

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

https://codereview.appspot.com/93670046/diff/1/constraints/constraints.go#new...
constraints/constraints.go:121: func (v *Value) extractNetworks(toInclude,
toExclude bool) []string {
I don't think it makes sense to have both 'toInclude' and 'toExclude', because
it would be really strange to want both. This also seems like it would benefit
from looking more  like a simple Filter. So maybe:

func isIncluded(name string) string {
  if !strings.HasPrefix(name, "^") {
    return name
  }
  return ""
}

func isExcluded(name string) string {
  if strings.HasPrefix(name, "^"^) {
    return name[1:]
  }
  return ""
}

func filterNetworks(filter func(string) string) []string {
  for _, name := range *v.Networks 
    if name = filter(name); name != "" {
      nets = append(nets, name)
    }
  }
  return nets
}

func (v *Value) IncludeNetworks() []string {
  return v.filterNetworks(isIncluded)
}

Thoughts?

https://codereview.appspot.com/93670046/diff/1/constraints/constraints.go#new...
constraints/constraints.go:454: v.Tags = parseCommaDelimited(str)
It seems odd that here we call "parseCommaDelimited" and earlier we call
"parseYamlStrings" I suppose it is because we're just using different source
data each time?

https://codereview.appspot.com/93670046/diff/1/constraints/constraints.go#new...
constraints/constraints.go:475: netName = strings.TrimPrefix(netName, "^")
I would tend to say that here we should use "netName = netName[1:]" because I
*don't* think we want to support:
  juju deploy --constraints=networks=^^^^foo

Whether we use it in the other case, meh. But the validation code should allow
"foo" or "^foo" but not "^^^^foo".

https://codereview.appspot.com/93670046/diff/1/constraints/constraints.go#new...
constraints/constraints.go:517: func parseCommaDelimited(s string) *[]string {
returning a pointer to a slice seems like an odd construction. Is there a reason
it can't just return a []string? Maybe its because our structs have to use
*[]string to distinguish being an empty value from not being there at all?

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

https://codereview.appspot.com/93670046/diff/1/constraints/constraints_test.g...
constraints/constraints_test.go:345: invalidNames := []string{"%net", "^net#2",
"_", "tcp:ip", "-foo", "net/3", "^net_4", "&#!"}
we may want a test with "net^2". And probably "^^net".

https://codereview.appspot.com/93670046/diff/1/constraints/constraints_test.g...
constraints/constraints_test.go:560: }}
If all of them take the same initial, should we pull it out of the table and
just have it as generic inside the test case itself?

I like this change, but it feels like we could go a step further.

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

https://codereview.appspot.com/93670046/diff/1/state/constraintsvalidation_te...
state/constraintsvalidation_test.go:43: }{{
Is this actually changing anything, or is it just reindenting this?

[hduran, 2014-06-02 14:47:17]

LGTM except a few things that I pointed out in the comments.

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

https://codereview.appspot.com/93670046/diff/1/constraints/constraints.go#new...
constraints/constraints.go:121: func (v *Value) extractNetworks(toInclude,
toExclude bool) []string {
On 2014/06/02 10:33:41, jameinel wrote:
> I don't think it makes sense to have both 'toInclude' and 'toExclude', because
> it would be really strange to want both. This also seems like it would benefit
> from looking more  like a simple Filter. So maybe:
> 
> func isIncluded(name string) string {
>   if !strings.HasPrefix(name, "^") {
>     return name
>   }
>   return ""
> }
> 
> func isExcluded(name string) string {
>   if strings.HasPrefix(name, "^"^) {
>     return name[1:]
>   }
>   return ""
> }
> 
> func filterNetworks(filter func(string) string) []string {
>   for _, name := range *v.Networks 
>     if name = filter(name); name != "" {
>       nets = append(nets, name)
>     }
>   }
>   return nets
> }
> 
> func (v *Value) IncludeNetworks() []string {
>   return v.filterNetworks(isIncluded)
> }
> 
> Thoughts?
I would fancy a bit more something like:
func (v *Value) extractNetworks() excluded, included []string {
    if strings.HasPrefix(name, "^") {
        excluded = append(excluded, name[1:])
    } else {
        included = append(included, name)
    }
}
and then you use the slice you need, or both if there really is a case for it,
even if you dont like this approach, I think the passing of incl/excl as
parameters add clutter with no real benefit.

https://codereview.appspot.com/93670046/diff/1/constraints/constraints.go#new...
constraints/constraints.go:475: netName = strings.TrimPrefix(netName, "^")
On 2014/06/02 10:33:41, jameinel wrote:
> I would tend to say that here we should use "netName = netName[1:]" because I
> *don't* think we want to support:
>   juju deploy --constraints=networks=^^^^foo
> 
> Whether we use it in the other case, meh. But the validation code should allow
> "foo" or "^foo" but not "^^^^foo".
Also, since we seem to do this a couple of times, perhaps a helper? if we ever
decide to chane ^ as the exclude char we better have to track it the least
possible in the code.

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

https://codereview.appspot.com/93670046/diff/1/constraints/constraints_test.g...
constraints/constraints_test.go:345: invalidNames := []string{"%net", "^net#2",
"_", "tcp:ip", "-foo", "net/3", "^net_4", "&#!"}
On 2014/06/02 10:33:41, jameinel wrote:
> we may want a test with "net^2". And probably "^^net".
And also the very odd case of ^^^^^^^^ which might pass if you dont check that
there are no subsequent ^ after the first and that there are other chars besides
^

[dimitern, 2014-06-04 09:26:27]

This got migrated to github at:
https://github.com/juju/juju/pull/9

Changes suggested were applied there.

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

https://codereview.appspot.com/93670046/diff/1/constraints/constraints.go#new...
constraints/constraints.go:121: func (v *Value) extractNetworks(toInclude,
toExclude bool) []string {
On 2014/06/02 14:47:17, hduran wrote:
> On 2014/06/02 10:33:41, jameinel wrote:
> > I don't think it makes sense to have both 'toInclude' and 'toExclude',
because
> > it would be really strange to want both. This also seems like it would
benefit
> > from looking more  like a simple Filter. So maybe:
> > 
> > func isIncluded(name string) string {
> >   if !strings.HasPrefix(name, "^") {
> >     return name
> >   }
> >   return ""
> > }
> > 
> > func isExcluded(name string) string {
> >   if strings.HasPrefix(name, "^"^) {
> >     return name[1:]
> >   }
> >   return ""
> > }
> > 
> > func filterNetworks(filter func(string) string) []string {
> >   for _, name := range *v.Networks 
> >     if name = filter(name); name != "" {
> >       nets = append(nets, name)
> >     }
> >   }
> >   return nets
> > }
> > 
> > func (v *Value) IncludeNetworks() []string {
> >   return v.filterNetworks(isIncluded)
> > }
> > 
> > Thoughts?
> I would fancy a bit more something like:
> func (v *Value) extractNetworks() excluded, included []string {
>     if strings.HasPrefix(name, "^") {
>         excluded = append(excluded, name[1:])
>     } else {
>         included = append(included, name)
>     }
> }
> and then you use the slice you need, or both if there really is a case for it,
> even if you dont like this approach, I think the passing of incl/excl as
> parameters add clutter with no real benefit.

That's a good idea! Done.

https://codereview.appspot.com/93670046/diff/1/constraints/constraints.go#new...
constraints/constraints.go:121: func (v *Value) extractNetworks(toInclude,
toExclude bool) []string {
On 2014/06/02 10:33:41, jameinel wrote:
> I don't think it makes sense to have both 'toInclude' and 'toExclude', because
> it would be really strange to want both. This also seems like it would benefit
> from looking more  like a simple Filter. So maybe:
> 
> func isIncluded(name string) string {
>   if !strings.HasPrefix(name, "^") {
>     return name
>   }
>   return ""
> }
> 
> func isExcluded(name string) string {
>   if strings.HasPrefix(name, "^"^) {
>     return name[1:]
>   }
>   return ""
> }
> 
> func filterNetworks(filter func(string) string) []string {
>   for _, name := range *v.Networks 
>     if name = filter(name); name != "" {
>       nets = append(nets, name)
>     }
>   }
>   return nets
> }
> 
> func (v *Value) IncludeNetworks() []string {
>   return v.filterNetworks(isIncluded)
> }
> 
> Thoughts?

Since it's a trivial case, this looks like way too much code for it, so I did
what Horacio suggested, if you don't mind.

https://codereview.appspot.com/93670046/diff/1/constraints/constraints.go#new...
constraints/constraints.go:454: v.Tags = parseCommaDelimited(str)
On 2014/06/02 10:33:41, jameinel wrote:
> It seems odd that here we call "parseCommaDelimited" and earlier we call
> "parseYamlStrings" I suppose it is because we're just using different source
> data each time?

Exactly, parseYamlStrings is called from SetYAML (in turn called by
goyaml.Unmarshal), while parseCommaDelimited here is called ultimately from
Parse, when dealing with strings.

https://codereview.appspot.com/93670046/diff/1/constraints/constraints.go#new...
constraints/constraints.go:475: netName = strings.TrimPrefix(netName, "^")
On 2014/06/02 10:33:41, jameinel wrote:
> I would tend to say that here we should use "netName = netName[1:]" because I
> *don't* think we want to support:
>   juju deploy --constraints=networks=^^^^foo
> 
> Whether we use it in the other case, meh. But the validation code should allow
> "foo" or "^foo" but not "^^^^foo".

strings.TrimPrefix("^^^net", "^") == "^^net", so it's fine.

https://codereview.appspot.com/93670046/diff/1/constraints/constraints.go#new...
constraints/constraints.go:517: func parseCommaDelimited(s string) *[]string {
On 2014/06/02 10:33:41, jameinel wrote:
> returning a pointer to a slice seems like an odd construction. Is there a
reason
> it can't just return a []string? Maybe its because our structs have to use
> *[]string to distinguish being an empty value from not being there at all?

It's so that the result can be directly assigned to v.Tags or v.Networks, which
are *[]string. And AFAIK both []string{} and []string(nil) are valid values,
whereas nil is not (i.e. empty vs. not set).

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

https://codereview.appspot.com/93670046/diff/1/constraints/constraints_test.g...
constraints/constraints_test.go:345: invalidNames := []string{"%net", "^net#2",
"_", "tcp:ip", "-foo", "net/3", "^net_4", "&#!"}
On 2014/06/02 10:33:41, jameinel wrote:
> we may want a test with "net^2". And probably "^^net".

Done.

https://codereview.appspot.com/93670046/diff/1/constraints/constraints_test.g...
constraints/constraints_test.go:345: invalidNames := []string{"%net", "^net#2",
"_", "tcp:ip", "-foo", "net/3", "^net_4", "&#!"}
On 2014/06/02 14:47:17, hduran wrote:
> On 2014/06/02 10:33:41, jameinel wrote:
> > we may want a test with "net^2". And probably "^^net".
> And also the very odd case of ^^^^^^^^ which might pass if you dont check that
> there are no subsequent ^ after the first and that there are other chars
besides
> ^

Done.

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

https://codereview.appspot.com/93670046/diff/1/state/constraintsvalidation_te...
state/constraintsvalidation_test.go:43: }{{
On 2014/06/02 10:33:41, jameinel wrote:
> Is this actually changing anything, or is it just reindenting this?

Mostly reindenting, but also added a couple of network specific cases at the
end.