Refactor deploy as proper statecmd

Refactor deploy as proper statecmd

This is the second step to implementing upgrade charm in the API (proper testing requires deploying, which wasn't available in the statecmd_test package).  Deploy is now implemented in the API in the same way as other commands (rather than directly in apiserver).  Tests were moved so that both the API command and CLI command are tested.

https://code.launchpad.net/~makyo/juju-core/deploy-refactor/+merge/164754

(do not edit description out of merge proposal)

[matthew.scott, 2013-05-20 15:19:47]

Please take a look.

[teknico, 2013-05-21 14:54:03]

LGTM

[gz, 2013-05-21 17:12:06]

LGTM, seems like a resonable moving around of code and addition of a couple of
bits. You probably still want another review from someone on juju-core.

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

https://codereview.appspot.com/9566045/diff/1/state/statecmd/deploy.go#newcode13
state/statecmd/deploy.go:13: func ServiceDeploy(st *state.State, args
params.ServiceDeploy, conn *juju.Conn, curl *charm.URL, repo charm.Repository)
error {
Could do with a doc comment for this function. Also, I find the argument list
somehwat long and opaque, having an args struct that gets partially examined
then repacked in a new one seems like a funny smell.

[jameinel, 2013-05-22 10:53:50]

The refactoring makes a lot of sense to me, and lets us re-use code.

I'd like to hear your view of why you want to use ConfigYAML rather than just
Config for the API call. And I'm concerned that we are breaking API
compatibility.

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

https://codereview.appspot.com/9566045/diff/1/state/api/params/params.go#newc...
state/api/params/params.go:37: ConfigYAML     string // Takes precedence over
config if both are present.
Since you removed "Config" it doesn't make sense to keep the comment about
taking precedence.

Also, from an API point of view, does it makes sense to have a JSON RPC call
that embeds a YAML configuration? It makes a lot more sense to me to have the
API parameters take a Config map[string]string and have the command line code
parse it as YAML, rather than having things like Juju-GUI needing to serialize
their config from JSON back into YAML to shove it into a string field.

https://codereview.appspot.com/9566045/diff/1/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (left):

https://codereview.appspot.com/9566045/diff/1/state/apiserver/apiserver.go#ol...
state/apiserver/apiserver.go:361: ConfigYAML:  args.ConfigYAML,
Also, I believe this is technically an API break, because there was a parameter
we used to accept which we no longer accept.
Do we need to worry about bumping the version of the API?
Do you know what Juju-GUI wants to pass? (Is it calling this API right now?)

[matthew.scott, 2013-05-22 15:11:08]

jameinel:

I appear to have gone in the wrong direction with regards to config, and will
re-propose with that added back in (I had been looking at
http://bazaar.launchpad.net/~juju/juju-core/trunk/view/head:/state/api/apicli...
which does not accept config, only configYAML)

[matthew.scott, 2013-05-22 17:01:53]

Replying to a comment below.  The doc comment has been added.

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

https://codereview.appspot.com/9566045/diff/1/state/statecmd/deploy.go#newcode13
state/statecmd/deploy.go:13: func ServiceDeploy(st *state.State, args
params.ServiceDeploy, conn *juju.Conn, curl *charm.URL, repo charm.Repository)
error {
On 2013/05/21 17:12:06, gz wrote:
> Could do with a doc comment for this function. Also, I find the argument list
> somehwat long and opaque, having an args struct that gets partially examined
> then repacked in a new one seems like a funny smell.

params.ServiceDeploy is introspected by the rpc package to determine the
arguments coming in over the websocket, and so I can't add conn, curl, and repo
there.  The reason for passing these as arguments is put well by rog here:
https://codereview.appspot.com/8926043/

[matthew.scott, 2013-05-22 19:36:27]

Please take a look.

[jameinel, 2013-05-23 07:41:16]

On 2013/05/22 15:11:08, matthew.scott wrote:
> jameinel:
> 
> I appear to have gone in the wrong direction with regards to config, and will
> re-propose with that added back in (I had been looking at
>
http://bazaar.launchpad.net/%7Ejuju/juju-core/trunk/view/head:/state/api/apic...
> which does not accept config, only configYAML)

So it does seem that the API only takes configYAML. That seems really silly to
me, but your design is correct for that.

[bac, 2013-05-23 14:33:10]

LGTM modulo comments about num units and some suggested new tests.

https://codereview.appspot.com/9566045/diff/10001/state/statecmd/deploy.go
File state/statecmd/deploy.go (right):

https://codereview.appspot.com/9566045/diff/10001/state/statecmd/deploy.go#ne...
state/statecmd/deploy.go:45: return errors.New("must deploy at least one unit")
Check with William on this policy especially for subordinates, as discussed on
IRC.

https://codereview.appspot.com/9566045/diff/10001/state/statecmd/deploy_test.go
File state/statecmd/deploy_test.go (right):

https://codereview.appspot.com/9566045/diff/10001/state/statecmd/deploy_test....
state/statecmd/deploy_test.go:180: 
Add TestNumUnitsZero (depending on outcome of discussion with William).

https://codereview.appspot.com/9566045/diff/10001/state/statecmd/deploy_test....
state/statecmd/deploy_test.go:191: 
Add TestServiceNameFromCharm

[matthew.scott, 2013-05-24 16:35:05]

*** Submitted:

Refactor deploy as proper statecmd

This is the second step to implementing upgrade charm in the API (proper testing
requires deploying, which wasn't available in the statecmd_test package). 
Deploy is now implemented in the API in the same way as other commands (rather
than directly in apiserver).  Tests were moved so that both the API command and
CLI command are tested.

R=teknico, gz, jameinel, bac
CC=
https://codereview.appspot.com/9566045