Code review comment for lp:~dimitern/juju-core/350-api-service-deploy-with-networks

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Please take a look.

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

https://codereview.appspot.com/76910044/diff/20001/state/api/params/params.go#newcode155
state/api/params/params.go:155: type ServiceDeployWithNetworks struct {
On 2014/03/20 14:06:23, rog wrote:
> I suggest that instead of making a new set of parameters, we could
just add the
> networks parameters to ServiceDeploy, and make
ServiceDeployWithNetworks an
> alias of Deploy.

> We can document the new fields as recognised only in 1.18 (1.19?
1.20?) API
> servers.

> This means that clients that care that the networks fields are
recognised can
> use ServiceDeployWithNetworks, but existing clients can continue to
use Deploy,
> and eventually we should be able to deprecate
ServiceDeployWithNetworks.

> This adds minimal clutter to the code, and provides the same
functionality
> AFAICS.

Done.

https://codereview.appspot.com/76910044/diff/20001/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/76910044/diff/20001/state/apiserver/client/client.go#newcode243
state/apiserver/client/client.go:243: return c.serviceDeploy(&args, nil)
On 2014/03/20 13:42:50, fwereade wrote:
> might be nicer to copy args into a ServiceDeployWithNetworks...

Done as rog suggested - ServiceDeployWithNetworks as an alias for
ServiceDeploy.

https://codereview.appspot.com/76910044/diff/20001/state/apiserver/client/client.go#newcode256
state/apiserver/client/client.go:256: func (c *Client)
serviceDeploy(args0 *params.ServiceDeploy, args1
*params.ServiceDeployWithNetworks) error {
On 2014/03/20 13:42:50, fwereade wrote:
> ...and just handle one arg type here?

Done differently.

https://codereview.appspot.com/76910044/

« Back to merge proposal