Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2)

Issue 12522043: Implemented the ServiceUpdate API call.

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 11 months ago by frankban
Modified:
8 years, 11 months ago
Reviewers:
mp+178718, dimitern, rog
Visibility:
Public.

Description

Implemented the ServiceUpdate API call. Pre-imp discussion with William about the requirement of a ServiceUpdate call. From the GUI perspective, this API is important also because it exposes a way to change the minimum number of units for a service. Also updated the megawatcher for the service so that Service.MinUnits is included in the returned info. https://code.launchpad.net/~frankban/juju-core/minunits-api/+merge/178718 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 18

Patch Set 2 : Implemented the ServiceUpdate API call. #

Patch Set 3 : Implemented the ServiceUpdate API call. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+370 lines, -39 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M state/api/client.go View 1 1 chunk +7 lines, -0 lines 0 comments Download
M state/api/params/params.go View 2 chunks +12 lines, -0 lines 0 comments Download
M state/api/params/params_test.go View 2 chunks +2 lines, -1 line 0 comments Download
M state/apiserver/client/client.go View 1 4 chunks +93 lines, -38 lines 0 comments Download
M state/apiserver/client/client_test.go View 1 1 chunk +227 lines, -0 lines 0 comments Download
M state/apiserver/client/perm_test.go View 1 2 chunks +19 lines, -0 lines 0 comments Download
M state/megawatcher.go View 1 chunk +1 line, -0 lines 0 comments Download
M state/megawatcher_internal_test.go View 5 chunks +7 lines, -0 lines 0 comments Download

Messages

Total messages: 6
frankban
Please take a look.
8 years, 11 months ago (2013-08-06 11:13:51 UTC) #1
dimitern
LGTM modulo the below https://codereview.appspot.com/12522043/diff/1/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/12522043/diff/1/state/api/client.go#newcode114 state/api/client.go:114: // ServiceUpdate updates the service ...
8 years, 11 months ago (2013-08-07 13:58:38 UTC) #2
rog
LGTM assuming we're happy with the goal. Personally, I am not very happy with ServiceUpdate ...
8 years, 11 months ago (2013-08-07 14:07:56 UTC) #3
frankban
Please take a look. https://codereview.appspot.com/12522043/diff/1/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/12522043/diff/1/state/api/client.go#newcode114 state/api/client.go:114: // ServiceUpdate updates the service ...
8 years, 11 months ago (2013-08-08 09:37:09 UTC) #4
frankban
On 2013/08/07 14:07:56, rog wrote: > LGTM assuming we're happy with the goal. > > ...
8 years, 11 months ago (2013-08-08 09:39:12 UTC) #5
rog
8 years, 11 months ago (2013-08-08 09:42:18 UTC) #6
On 2013/08/08 09:39:12, frankban wrote:
> On 2013/08/07 14:07:56, rog wrote:
> > LGTM assuming we're happy with the goal.
> > 
> > Personally, I am not very happy with ServiceUpdate - it seems like a
solution
> in
> > search of a problem, and it means we'll have to either maintain overlapping
> > entry points in Client, or deprecate existing Client entry points, neither
of
> > which I would do given the choice.
> > 
> > If there was existing client-side code that could benefit from setting all
> these
> > attributes atomically, I might be in favour, but
> > a) I'm not aware of any such client-side code.
> > b) By creating this call, we provide the illusion that the settings are
being
> > changed atomically, but if it fails half-way through, the client will be
left
> in
> > an unknown state, something that's not true if it is making individual
calls.
> 
> As discussed, we will discuss about API implications later. In the meanwhile,
> the GUI
> can take advantage of the possibility to set minimum units for a service.
> 
> >
https://codereview.appspot.com/12522043/diff/1/state/api/client.go#newcode116
> > state/api/client.go:116: func (c *Client) ServiceUpdate(serviceName string,
> > charmUrl string,
> > I think that this has enough arguments that its argument should
> > be a struct. In fact, why don't we just use params.ServiceUpdate?
> 
> Done.

still LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b