Add service constraint fetching to the API

Add service constraint fetching to the API



https://code.launchpad.net/~benji/juju-core/expose-service-constraints/+merge/157115

(do not edit description out of merge proposal)

[benji, 2013-04-04 13:53:40]

Please take a look.

[dimitern, 2013-04-04 14:21:34]

LGTM, modulo a few trivials.

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

https://codereview.appspot.com/8366043/diff/1/cmd/juju/constraints.go#newcode66
cmd/juju/constraints.go:66: if err != nil {
how about just: if err == nil { cons = results.Constraints }, the if below will
take care of the other case.

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

https://codereview.appspot.com/8366043/diff/1/state/api/apiclient.go#newcode171
state/api/apiclient.go:171: // GetServiceConstraints specifies the constraints
for the given service.
s/specifies/returns/

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

https://codereview.appspot.com/8366043/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:374: // GetServiceConstraints sets the constraints
for a given service.
s/sets/returns/

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

https://codereview.appspot.com/8366043/diff/1/state/statecmd/getconstraints.g...
state/statecmd/getconstraints.go:11: // GetServiceContstraints sets the
constraints for a given service
ditto

[benji, 2013-04-04 14:32:45]

Thanks for the review.  I have made the suggested changes.

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

https://codereview.appspot.com/8366043/diff/1/cmd/juju/constraints.go#newcode66
cmd/juju/constraints.go:66: if err != nil {
On 2013/04/04 14:21:34, dimitern wrote:
> how about just: if err == nil { cons = results.Constraints }, the if below
will
> take care of the other case.

I initially tried that but since err is being (re)defined in the inner scope its
value does not survive to be evaluated outside the "else".

But, looking at the code again I see that if I change from := to = and add an
explicit declaration for "results" then the outer "err" will be used inside the
"else" and it will work as you suggest.  It is about the same amount of code so
it isn't as big a win as I would like, but it's done.

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

https://codereview.appspot.com/8366043/diff/1/state/api/apiclient.go#newcode171
state/api/apiclient.go:171: // GetServiceConstraints specifies the constraints
for the given service.
Thanks.  Fixed.

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

https://codereview.appspot.com/8366043/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:374: // GetServiceConstraints sets the constraints
for a given service.
On 2013/04/04 14:21:34, dimitern wrote:
> s/sets/returns/

Done.

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

https://codereview.appspot.com/8366043/diff/1/state/statecmd/getconstraints.g...
state/statecmd/getconstraints.go:11: // GetServiceContstraints sets the
constraints for a given service
On 2013/04/04 14:21:34, dimitern wrote:
> ditto

Done.

[benji, 2013-04-04 14:44:06]

Please take a look.

[bac, 2013-04-04 15:21:02]

The code looks good, except for a few typos, etc.

There is quite a paucity of tests in this branch.  Is your reasoning that the
call is just a wrapper around an already well-tested function?

https://codereview.appspot.com/8366043/diff/11001/cmd/juju/constraints.go
File cmd/juju/constraints.go (right):

https://codereview.appspot.com/8366043/diff/11001/cmd/juju/constraints.go#new...
cmd/juju/constraints.go:66: var results params.GetServiceConstraintsResults
Is the explicit declaration required?

https://codereview.appspot.com/8366043/diff/11001/state/statecmd/getconstrain...
File state/statecmd/getconstraints.go (right):

https://codereview.appspot.com/8366043/diff/11001/state/statecmd/getconstrain...
state/statecmd/getconstraints.go:1: // Code shared by the CLI and API for the
SetConstraints function.
GetConstraints

https://codereview.appspot.com/8366043/diff/11001/state/statecmd/getconstrain...
state/statecmd/getconstraints.go:11: // GetServiceContstraints returns the
constraints for a given service
typo: GetServiceContstraints

[benji, 2013-04-04 15:53:47]

Thanks for the review.  I have fixed all the errors you pointed out.

As discussed on IRC, the tests are minimal because 1) the new code is minimal
and 2) this level of testing has been determined acceptable by previous
branches.

https://codereview.appspot.com/8366043/diff/11001/cmd/juju/constraints.go
File cmd/juju/constraints.go (right):

https://codereview.appspot.com/8366043/diff/11001/cmd/juju/constraints.go#new...
cmd/juju/constraints.go:66: var results params.GetServiceConstraintsResults
On 2013/04/04 15:21:02, bac wrote:
> Is the explicit declaration required?

It is if we let "err" fall out and be tested below.  This is because if we use
an = on line 67 instead of := so a new "err" is not created but instead the
outer "err" is reused.  This means that "results" has to exist before the
assignment, hence the declaration.

https://codereview.appspot.com/8366043/diff/11001/state/statecmd/getconstrain...
File state/statecmd/getconstraints.go (right):

https://codereview.appspot.com/8366043/diff/11001/state/statecmd/getconstrain...
state/statecmd/getconstraints.go:1: // Code shared by the CLI and API for the
SetConstraints function.
On 2013/04/04 15:21:02, bac wrote:
> GetConstraints

Thanks; fixed.

https://codereview.appspot.com/8366043/diff/11001/state/statecmd/getconstrain...
state/statecmd/getconstraints.go:11: // GetServiceContstraints returns the
constraints for a given service
On 2013/04/04 15:21:02, bac wrote:
> typo: GetServiceContstraints

Done.

[benji, 2013-04-04 15:54:36]

Please take a look.

[bac, 2013-04-04 18:47:48]

LGTM, FWIW.

[benji, 2013-04-04 19:33:16]

*** Submitted:

Add service constraint fetching to the API

R=dimitern, bac
CC=
https://codereview.appspot.com/8366043