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/40001/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/76910044/diff/40001/state/apiserver/client/client.go#newcode298
state/apiserver/client/client.go:298: if len(args.IncludedNetworks) == 0
&& len(args.ExcludedNetworks) == 0 {
On 2014/03/20 15:45:03, rog wrote:
> I'd remove this and make the two commands exactly the same. Then
clients can use
> this call for all Deploy purposes if they wish.

Done.

https://codereview.appspot.com/76910044/diff/40001/state/apiserver/client/client.go#newcode298
state/apiserver/client/client.go:298: if len(args.IncludedNetworks) == 0
&& len(args.ExcludedNetworks) == 0 {
On 2014/03/20 20:38:22, jameinel wrote:
> On 2014/03/20 15:45:03, rog wrote:
> > I'd remove this and make the two commands exactly the same. Then
clients can
> use
> > this call for all Deploy purposes if they wish.

> As William pointed out in IRC, I think the "ideal" would be to have
API
> versioning and then we'd just have ServiceDeploy that accepts
everything.
> However, I think lacking that I agree with Roger. I'd rather have just
1 API
> that is always called.
> (So we effectively have API versioning, just via a clumsy renaming of
the API
> each time.)

Done as rog suggested. Also, I'm not landing this until mgz's branch
lands. Then I'll integrate the two and drop the related TODOs.

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

« Back to merge proposal