Add SetServiceConstraints to API

Add SetServiceConstraints to API

This also involved moving the Constraints struct into its own package in order
to break an import cycle.

https://code.launchpad.net/~benji/juju-core/1130172/+merge/153931

(do not edit description out of merge proposal)

[benji, 2013-03-18 20:50:20]

Please take a look.

[dave_cheney.net, 2013-03-18 23:54:42]

Please make sure you sync up with fwereade, otherwise you'll have massive merge
conflicts.

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

https://codereview.appspot.com/7600044/diff/1/constraints/constraints.go#newc...
constraints/constraints.go:14: type Value struct {
I would be fine with constraints.Constraints, we have state.State,
environs.Environ, config.Config, etc.

[bac, 2013-03-19 00:20:11]

A few minor things to fix.  It looks ok to me but I'm not going to do a proper
review.

https://codereview.appspot.com/7600044/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7600044/diff/1/state/api/apiclient.go#newcode151
state/api/apiclient.go:151: // SetConstraints destroys a given service.
Comment doesn't match function.

https://codereview.appspot.com/7600044/diff/1/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7600044/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:372: // SetConstraints destroys a given service.
Wrong comment.

https://codereview.appspot.com/7600044/diff/1/state/statecmd/setcontstraints.go
File state/statecmd/setcontstraints.go (right):

https://codereview.appspot.com/7600044/diff/1/state/statecmd/setcontstraints....
state/statecmd/setcontstraints.go:1: // Code shared by the CLI and API for the
SetContstraints function.
typo: SetContstraints

https://codereview.appspot.com/7600044/diff/1/state/statecmd/setcontstraints....
state/statecmd/setcontstraints.go:2: 
Actually I see you have a typo in the name of the file too.

[dimitern, 2013-03-19 09:34:53]

LGTM, minus some trivials,.

https://codereview.appspot.com/7600044/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/7600044/diff/1/state/api/params/params.go#newc...
state/api/params/params.go:139: // SetConstraints stores parameters for making
the GetAnnotations call.
fix comment

https://codereview.appspot.com/7600044/diff/1/state/statecmd/setcontstraints.go
File state/statecmd/setcontstraints.go (right):

https://codereview.appspot.com/7600044/diff/1/state/statecmd/setcontstraints....
state/statecmd/setcontstraints.go:11: // SetContstraints removes the specified
units.
fix comment

[benji, 2013-03-19 18:55:56]

I have fixed the issues pointed out in reviews and will re-propose.

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

https://codereview.appspot.com/7600044/diff/1/constraints/constraints.go#newc...
constraints/constraints.go:14: type Value struct {
On 2013/03/18 23:54:42, dfc wrote:
> I would be fine with constraints.Constraints, we have state.State,
> environs.Environ, config.Config, etc.

constraints.Constraints sounds good to me, but this was the spelling I was given
by Roger.  I will leave this as-is in the current branch but we should feel free
to change it should a consensus arise.

https://codereview.appspot.com/7600044/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7600044/diff/1/state/api/apiclient.go#newcode151
state/api/apiclient.go:151: // SetConstraints destroys a given service.
On 2013/03/19 00:20:11, bac wrote:
> Comment doesn't match function.

Fixed.

I really want to write a godoc linter that will point these problems out before
they get to review.

https://codereview.appspot.com/7600044/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/7600044/diff/1/state/api/params/params.go#newc...
state/api/params/params.go:139: // SetConstraints stores parameters for making
the GetAnnotations call.
On 2013/03/19 09:34:54, dimitern wrote:
> fix comment

Done.

https://codereview.appspot.com/7600044/diff/1/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7600044/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:372: // SetConstraints destroys a given service.
On 2013/03/19 00:20:11, bac wrote:
> Wrong comment.

Done.

https://codereview.appspot.com/7600044/diff/1/state/statecmd/setcontstraints.go
File state/statecmd/setcontstraints.go (right):

https://codereview.appspot.com/7600044/diff/1/state/statecmd/setcontstraints....
state/statecmd/setcontstraints.go:1: // Code shared by the CLI and API for the
SetContstraints function.
On 2013/03/19 00:20:11, bac wrote:
> typo: SetContstraints

Done.

https://codereview.appspot.com/7600044/diff/1/state/statecmd/setcontstraints....
state/statecmd/setcontstraints.go:1: // Code shared by the CLI and API for the
SetContstraints function.
On 2013/03/19 00:20:11, bac wrote:
> typo: SetContstraints

Done.

https://codereview.appspot.com/7600044/diff/1/state/statecmd/setcontstraints....
state/statecmd/setcontstraints.go:2: 
On 2013/03/19 00:20:11, bac wrote:
> Actually I see you have a typo in the name of the file too.

Wow, I did this a lot.  Fixed.

https://codereview.appspot.com/7600044/diff/1/state/statecmd/setcontstraints....
state/statecmd/setcontstraints.go:11: // SetContstraints removes the specified
units.
On 2013/03/19 09:34:54, dimitern wrote:
> fix comment

Done.

[benji, 2013-03-19 18:56:19]

Please take a look.

[dimitern, 2013-03-20 08:39:49]

LGTM

https://codereview.appspot.com/7600044/diff/15001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/7600044/diff/15001/constraints/constraints.go#...
constraints/constraints.go:34: // String expresses a constraints.Value in the
language in which it was specified.
No need to comment on this - it simply returns the object as a string.

[fwereade, 2013-03-20 13:19:05]

Not clear on the plan for handling environment constraints; couple of other
comments below.

https://codereview.appspot.com/7600044/diff/15001/cmd/juju/constraints.go
File cmd/juju/constraints.go (left):

https://codereview.appspot.com/7600044/diff/15001/cmd/juju/constraints.go#old...
cmd/juju/constraints.go:107: return
conn.State.SetEnvironConstraints(c.Constraints)
We need to handle this branch as well. Is that intended for a follow-up? I would
expect that an empty service field in the params could be handled quite easily
on the API side, rather than leaving the implementation of this command
half-moved.

https://codereview.appspot.com/7600044/diff/15001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/7600044/diff/15001/constraints/constraints.go#...
constraints/constraints.go:34: // String expresses a constraints.Value in the
language in which it was specified.
On 2013/03/20 08:39:49, dimitern wrote:
> No need to comment on this - it simply returns the object as a string.

Disagree. This property of the String() method is important, and other code
relies upon it.

https://codereview.appspot.com/7600044/diff/15001/constraints/constraints.go#...
constraints/constraints.go:63: // ParseConstraints constructs a constraint.Value
from the supplied arguments,
Please just call this Parse.

https://codereview.appspot.com/7600044/diff/15001/constraints/constraints.go#...
constraints/constraints.go:138: func (c *Value) setMem(str string) error {
I would like it if you were to rename c to v throughout here, but it's up to
you.

https://codereview.appspot.com/7600044/diff/15001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7600044/diff/15001/state/api/apiclient.go#newc...
state/api/apiclient.go:151: // SetConstraints specifies the constraints for the
given service.
If we're going to have two methods, this one should probably be
SetServiceConstraints.

[benji, 2013-03-20 16:44:59]

All of the latest round of review changes have been made.  I will be submitting
the branch shortly.

https://codereview.appspot.com/7600044/diff/15001/cmd/juju/constraints.go
File cmd/juju/constraints.go (left):

https://codereview.appspot.com/7600044/diff/15001/cmd/juju/constraints.go#old...
cmd/juju/constraints.go:107: return
conn.State.SetEnvironConstraints(c.Constraints)
On 2013/03/20 13:19:05, fwereade wrote:
> We need to handle this branch as well. Is that intended for a follow-up? I
would
> expect that an empty service field in the params could be handled quite easily
> on the API side, rather than leaving the implementation of this command
> half-moved.

This branch is about the service constraints.  Environment constraints will be
handled in a future branch.

https://codereview.appspot.com/7600044/diff/15001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/7600044/diff/15001/constraints/constraints.go#...
constraints/constraints.go:34: // String expresses a constraints.Value in the
language in which it was specified.
On 2013/03/20 08:39:49, dimitern wrote:
> No need to comment on this - it simply returns the object as a string.

I have removed this pre-existing comment.

https://codereview.appspot.com/7600044/diff/15001/constraints/constraints.go#...
constraints/constraints.go:34: // String expresses a constraints.Value in the
language in which it was specified.
On 2013/03/20 13:19:05, fwereade wrote:
> On 2013/03/20 08:39:49, dimitern wrote:
> > No need to comment on this - it simply returns the object as a string.
> 
> Disagree. This property of the String() method is important, and other code
> relies upon it.

I have re-instated the comment.

https://codereview.appspot.com/7600044/diff/15001/constraints/constraints.go#...
constraints/constraints.go:63: // ParseConstraints constructs a constraint.Value
from the supplied arguments,
On 2013/03/20 13:19:05, fwereade wrote:
> Please just call this Parse.

Done.

https://codereview.appspot.com/7600044/diff/15001/constraints/constraints.go#...
constraints/constraints.go:138: func (c *Value) setMem(str string) error {
On 2013/03/20 13:19:05, fwereade wrote:
> I would like it if you were to rename c to v throughout here, but it's up to
> you.

Done.

https://codereview.appspot.com/7600044/diff/15001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7600044/diff/15001/state/api/apiclient.go#newc...
state/api/apiclient.go:151: // SetConstraints specifies the constraints for the
given service.
On 2013/03/20 13:19:05, fwereade wrote:
> If we're going to have two methods, this one should probably be
> SetServiceConstraints.

Done.

[benji, 2013-03-20 20:06:00]

*** Submitted:

Add SetServiceConstraints to API

This also involved moving the Constraints struct into its own package in order
to break an import cycle.

R=dfc, bac, dimitern, fwereade
CC=
https://codereview.appspot.com/7600044