Add ServiceDeploy to the API.

Add ServiceDeploy to the API.



https://code.launchpad.net/~bac/juju-core/api-deploy/+merge/152503

(do not edit description out of merge proposal)

[bac, 2013-03-08 20:21:25]

Please take a look.

[dimitern, 2013-03-08 20:48:46]

Looks good, except for the following suggestions.

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode195
juju/conn.go:195: panic("Charm is nil")
should this be a panic rather than just return an error?

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode199
juju/conn.go:199: return &state.Service{}, err
just return nil, err

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode203
juju/conn.go:203: ss_args := params.ServiceSetYAML{
ssArgs - no underscores please

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode207
juju/conn.go:207: err = ServiceSetYAML(conn.State, ss_args)
if err := ServicSetYAML(...); err != nil {
  return nil, err
}

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode212
juju/conn.go:212: ss_args := params.ServiceSet{
ssArgs

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode216
juju/conn.go:216: err = ServiceSet(conn.State, ss_args)
like above

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode226
juju/conn.go:226: return svc, err
i think you should return nil, err here and drop the service before

https://codereview.appspot.com/7635044/diff/2001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/7635044/diff/2001/juju/conn_test.go#newcode553
juju/conn_test.go:553: // interface.
delete this line

This is actually wrong (and I mislead you probably) - it means initialize the
Suite with the passed suite pointer.
Compile-time check for "does it implement the interface" is
var _ InterfaceType = (*implementationType)(nil)

https://codereview.appspot.com/7635044/diff/2001/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/7635044/diff/2001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:46: err = service.Refresh()
Please, add before the line with the first Refresh():

// TODO: This is due to LP bug #1152717 - remove when fixed.

https://codereview.appspot.com/7635044/diff/2001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7635044/diff/2001/state/apiserver/apiserver.go...
state/apiserver/apiserver.go:340: deploy_args := juju.DeployServiceParams{
deployArgs - no underscores please

Also, you can probably drop the keys and use just:
juju.DeployServiceParams{charm, serviceName, args.NumUnits, args.Config,
args.ConfigYAML}

[dave_cheney.net, 2013-03-08 20:56:48]

A few more comments, mostly style related.

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

https://codereview.appspot.com/7635044/diff/2001/cmd/juju/deploy.go#newcode114
cmd/juju/deploy.go:114: }
defer f.Close() is more traditional

https://codereview.appspot.com/7635044/diff/2001/cmd/juju/deploy.go#newcode116
cmd/juju/deploy.go:116: f.Close()
than this

https://codereview.appspot.com/7635044/diff/2001/cmd/juju/set.go
File cmd/juju/set.go (right):

https://codereview.appspot.com/7635044/diff/2001/cmd/juju/set.go#newcode71
cmd/juju/set.go:71: err = juju.ServiceSet(conn.State, params.ServiceSet{
if len(contents) == 0 {
   return juju.ServiceSet(conn.State, ...
}
return juju.ServiceSetYAML( ...

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode188
juju/conn.go:188: // ConfigYAML akes precedence over Config if both are
provided.
akes ?

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode196
juju/conn.go:196: }
AddService will already panic if the charm is nil, please drop this check. I
will propose a change to add a sanity check inside state.AddService.

[bac, 2013-03-09 14:39:03]

Thanks for the reviews.  All changes done.

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

https://codereview.appspot.com/7635044/diff/2001/cmd/juju/deploy.go#newcode114
cmd/juju/deploy.go:114: }
I'm now using ioutil.ReadFile which avoids the problem.

https://codereview.appspot.com/7635044/diff/2001/cmd/juju/set.go
File cmd/juju/set.go (right):

https://codereview.appspot.com/7635044/diff/2001/cmd/juju/set.go#newcode71
cmd/juju/set.go:71: err = juju.ServiceSet(conn.State, params.ServiceSet{
On 2013/03/08 20:56:49, dfc wrote:
> if len(contents) == 0 {
>    return juju.ServiceSet(conn.State, ...
> }
> return juju.ServiceSetYAML( ... 

Done.

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode188
juju/conn.go:188: // ConfigYAML akes precedence over Config if both are
provided.
On 2013/03/08 20:56:49, dfc wrote:
> akes ?

Done.

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode195
juju/conn.go:195: panic("Charm is nil")
On 2013/03/08 20:48:46, dimitern wrote:
> should this be a panic rather than just return an error?

Done.

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode196
juju/conn.go:196: }
On 2013/03/08 20:56:49, dfc wrote:
> AddService will already panic if the charm is nil, please drop this check. I
> will propose a change to add a sanity check inside state.AddService.

Done.

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode199
juju/conn.go:199: return &state.Service{}, err
On 2013/03/08 20:48:46, dimitern wrote:
> just return nil, err

Done.

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode207
juju/conn.go:207: err = ServiceSetYAML(conn.State, ss_args)
I'll make the change but this is one of the most unreadable language constructs
in Go.  I don't think we should be forced to use every Go-ism just because it is
new and shiny.  Some, like this one, are turds.

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode212
juju/conn.go:212: ss_args := params.ServiceSet{
On 2013/03/08 20:48:46, dimitern wrote:
> ssArgs

Done.

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode216
juju/conn.go:216: err = ServiceSet(conn.State, ss_args)
On 2013/03/08 20:48:46, dimitern wrote:
> like above

Done.

https://codereview.appspot.com/7635044/diff/2001/juju/conn.go#newcode226
juju/conn.go:226: return svc, err
On 2013/03/08 20:48:46, dimitern wrote:
> i think you should return nil, err here and drop the service before

Done.

https://codereview.appspot.com/7635044/diff/2001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/7635044/diff/2001/juju/conn_test.go#newcode553
juju/conn_test.go:553: // interface.
On 2013/03/08 20:48:46, dimitern wrote:
> delete this line
> 
> This is actually wrong (and I mislead you probably) - it means initialize the
> Suite with the passed suite pointer.
> Compile-time check for "does it implement the interface" is
> var _ InterfaceType = (*implementationType)(nil)

Done.

https://codereview.appspot.com/7635044/diff/2001/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/7635044/diff/2001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:46: err = service.Refresh()
Thanks for your help finding this bug and the work-around.

https://codereview.appspot.com/7635044/diff/2001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7635044/diff/2001/state/apiserver/apiserver.go...
state/apiserver/apiserver.go:340: deploy_args := juju.DeployServiceParams{
Sorry, too many programming environments to keep straight.

[bac, 2013-03-09 14:54:26]

*** Submitted:

Add ServiceDeploy to the API.

R=dimitern, dfc
CC=
https://codereview.appspot.com/7635044