Add ServiceDestroyUnits to the API

Add ServiceDestroyUnits to the API



https://code.launchpad.net/~benji/juju-core/1130151/+merge/153351

(do not edit description out of merge proposal)

[benji, 2013-03-14 12:51:30]

Please take a look.

[benji, 2013-03-14 12:54:17]

I added a comment about the motivation of one of the changes.

https://codereview.appspot.com/7554046/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7554046/diff/1/state/state.go#newcode65
state/state.go:65: )
The above is tangential to this branch.  I noticed the duplication between the
validService and validUnit regexes as a bug magnet (if they ever differ it is a
problem) so I unified them.

[fwereade, 2013-03-14 14:02:28]

LGTM modulo naming.

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

https://codereview.appspot.com/7554046/diff/1/state/api/apiclient.go#newcode138
state/api/apiclient.go:138: // ServiceDestroyUnits decreases the number of units
dedicated to a service.
Not sure about "Service" -- the units could come from any mix of units. Maybe
AddServiceUnits/DestroyServiceUnits throughout for (1) consistency and (2)
additional sanity ("unit" is short for "service unit" anyway).

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

https://codereview.appspot.com/7554046/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:404: // We can't remove the last unit so we have to
add one first.
Can't we? Where's that restriction implemented?

https://codereview.appspot.com/7554046/diff/1/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7554046/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:361: // ServiceDestroyUnits removes a given number
of units from a service.
...removes the given service units?

(similarly elsewhere)

https://codereview.appspot.com/7554046/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7554046/diff/1/state/state.go#newcode65
state/state.go:65: )
On 2013/03/14 12:54:17, benji wrote:
> The above is tangential to this branch.  I noticed the duplication between the
> validService and validUnit regexes as a bug magnet (if they ever differ it is
a
> problem) so I unified them.

Nice, thanks.

[dimitern, 2013-03-14 14:18:49]

Overall LGTM, but I have doubts about the regex change and a few other
suggestions.

https://codereview.appspot.com/7554046/diff/1/cmd/juju/destroyunit.go
File cmd/juju/destroyunit.go (right):

https://codereview.appspot.com/7554046/diff/1/cmd/juju/destroyunit.go#newcode41
cmd/juju/destroyunit.go:41: // DestroyUnitCommand connects to the environment
specified on the command line
It's still Run(), so it starts with // Run ... . Probably move the more general
comment to the struct doc comment?

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

https://codereview.appspot.com/7554046/diff/1/state/api/apiclient.go#newcode133
state/api/apiclient.go:133: return clientError(err)
I think clientError() accepts err == nil, so maybe just replace the above with
just return clientError(err) ?

https://codereview.appspot.com/7554046/diff/1/state/api/apiclient.go#newcode138
state/api/apiclient.go:138: // ServiceDestroyUnits decreases the number of units
dedicated to a service.
On 2013/03/14 14:02:28, fwereade wrote:
> Not sure about "Service" -- the units could come from any mix of units. Maybe
> AddServiceUnits/DestroyServiceUnits throughout for (1) consistency and (2)
> additional sanity ("unit" is short for "service unit" anyway).

+1

https://codereview.appspot.com/7554046/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7554046/diff/1/state/state.go#newcode63
state/state.go:63: validUnit      = regexp.MustCompile(serviceSnippet +
"(-[a-z0-9]*[a-z][a-z0-9]*)*/[0-9]+$")
I don't think this is the same:

validService(old) = "^serviceSnippet$"
validUnit(old) = ^serviceSnippet*/[0-9]+$"
validUnit(new) = "^serviceSnippet+{second part of serviceSnippet:
(-[a-z0-9]*[a-z][a-z0-9]*)*}+"/[0-9]+$"

So you're actually checking for
validUnit(new) =
"^[a-z][a-z0-9]*(-[a-z0-9]*[a-z][a-z0-9]*)*(-[a-z0-9]*[a-z][a-z0-9]*)*/[0-9]+$"

Now you can have an extra (-[a-z0-9]*[a-z][a-z0-9]*)* part, which is both
redundant (already covered in serviceSnippet) and confusing IMHO.

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

https://codereview.appspot.com/7554046/diff/1/state/statecmd/destroyunit.go#n...
state/statecmd/destroyunit.go:17: err = conn.DestroyUnits(args.UnitNames...)
just return conn.DestroyUnits(args.UnitNames...)

[fwereade, 2013-03-14 14:32:53]

https://codereview.appspot.com/7554046/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7554046/diff/1/state/state.go#newcode63
state/state.go:63: validUnit      = regexp.MustCompile(serviceSnippet +
"(-[a-z0-9]*[a-z][a-z0-9]*)*/[0-9]+$")
On 2013/03/14 14:18:49, dimitern wrote:
> I don't think this is the same:
> 
> validService(old) = "^serviceSnippet$"
> validUnit(old) = ^serviceSnippet*/[0-9]+$"
> validUnit(new) = "^serviceSnippet+{second part of serviceSnippet:
> (-[a-z0-9]*[a-z][a-z0-9]*)*}+"/[0-9]+$"
> 
> So you're actually checking for
> validUnit(new) =
>
"^[a-z][a-z0-9]*(-[a-z0-9]*[a-z][a-z0-9]*)*(-[a-z0-9]*[a-z][a-z0-9]*)*/[0-9]+$"
> 
> Now you can have an extra (-[a-z0-9]*[a-z][a-z0-9]*)* part, which is both
> redundant (already covered in serviceSnippet) and confusing IMHO.

Worse, I think it actually changes the meaning -- the duplicated part is saying
"no -{digits} suffixes allowed".

/me hangs head in shame for not spotting that -- thnks dimitern.

[rog, 2013-03-14 15:44:11]

looks good, apart from the opClientServiceDestroyUnits issue.

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

https://codereview.appspot.com/7554046/diff/1/state/api/apiclient.go#newcode133
state/api/apiclient.go:133: return clientError(err)
On 2013/03/14 14:18:49, dimitern wrote:
> I think clientError() accepts err == nil, so maybe just replace the above with
> just return clientError(err) ?

+1 (assuming that change has gone in)

https://codereview.appspot.com/7554046/diff/1/state/api/apiclient.go#newcode138
state/api/apiclient.go:138: // ServiceDestroyUnits decreases the number of units
dedicated to a service.
On 2013/03/14 14:18:49, dimitern wrote:
> On 2013/03/14 14:02:28, fwereade wrote:
> > Not sure about "Service" -- the units could come from any mix of units.
Maybe
> > AddServiceUnits/DestroyServiceUnits throughout for (1) consistency and (2)
> > additional sanity ("unit" is short for "service unit" anyway).
> 
> +1

yeah, i could go with that.

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

https://codereview.appspot.com/7554046/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:404: // We can't remove the last unit so we have to
add one first.
On 2013/03/14 14:02:28, fwereade wrote:
> Can't we? Where's that restriction implemented?

Apart from that, this is actually testing the wrong call.
ServiceAddUnits will fail in exactly the same way that ServiceDestroyUnits will,
so AFAICS ServiceDestroyUnits will never be called here.

That leaves the question of how to implement this test, because destroying a
unit is not something that can be undone.

To be honest, I'm starting to have second feelings about all these opClient*
functions. They are all just testing exactly the same code (the requireClient if
statement in srvRoot.Client) and they eat up a lot of time. I think I'd be happy
if we deleted all but one of them, and left a comment saying why one is
sufficient to test all Client entry points (and probably a comment in
srvRoot.Client too, as a warning against future structural changes there).

Thoughts?

https://codereview.appspot.com/7554046/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7554046/diff/1/state/state.go#newcode61
state/state.go:61: serviceSnippet = "^[a-z][a-z0-9]*(-[a-z0-9]*[a-z][a-z0-9]*)*"
i think i'd leave the "^" off here, which makes serviceSnippet into a
potentially useful thing if anywhere else needs to use a service name as part of
a regexp. then i'd name it "serviceNamePattern", making its value more obvious.

then both the valid* variables below would add the leading "^", so it's obvious
at a glance that both of them are anchored.

also (trivial) serviceSnippet should be a const.

[rog, 2013-03-14 16:28:24]

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

https://codereview.appspot.com/7554046/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:404: // We can't remove the last unit so we have to
add one first.
On 2013/03/14 15:44:11, rog wrote:
> On 2013/03/14 14:02:28, fwereade wrote:
> > Can't we? Where's that restriction implemented?
> 
> Apart from that, this is actually testing the wrong call.
> ServiceAddUnits will fail in exactly the same way that ServiceDestroyUnits
will,
> so AFAICS ServiceDestroyUnits will never be called here.
> 
> That leaves the question of how to implement this test, because destroying a
> unit is not something that can be undone.
> 
> To be honest, I'm starting to have second feelings about all these opClient*
> functions. They are all just testing exactly the same code (the requireClient
if
> statement in srvRoot.Client) and they eat up a lot of time. I think I'd be
happy
> if we deleted all but one of them, and left a comment saying why one is
> sufficient to test all Client entry points (and probably a comment in
> srvRoot.Client too, as a warning against future structural changes there).
> 
> Thoughts?

after discussion with William, this is not a preferred approach, so I'd suggest
using mst to add the unit and Client.DestroyUnits to destroy it. unlike in most
of these functions, you'll have something to undo when the operation *fails*
rather than succeeds.

[benji, 2013-03-15 13:39:12]

Thanks for the good review guys.  I have addressed everything and am preparing
to submit the branch.

https://codereview.appspot.com/7554046/diff/1/cmd/juju/destroyunit.go
File cmd/juju/destroyunit.go (right):

https://codereview.appspot.com/7554046/diff/1/cmd/juju/destroyunit.go#newcode41
cmd/juju/destroyunit.go:41: // DestroyUnitCommand connects to the environment
specified on the command line
On 2013/03/14 14:18:49, dimitern wrote:
> It's still Run(), so it starts with // Run ... . Probably move the more
general
> comment to the struct doc comment?

Good catch, I'm still not used to reading Go's function signatures.

This class of error would be caught by a good linter.  Is there such a thing for
Go?

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

https://codereview.appspot.com/7554046/diff/1/state/api/apiclient.go#newcode133
state/api/apiclient.go:133: return clientError(err)
On 2013/03/14 14:18:49, dimitern wrote:
> I think clientError() accepts err == nil, so maybe just replace the above with
> just return clientError(err) ?

Done.

https://codereview.appspot.com/7554046/diff/1/state/api/apiclient.go#newcode138
state/api/apiclient.go:138: // ServiceDestroyUnits decreases the number of units
dedicated to a service.
On 2013/03/14 14:02:28, fwereade wrote:
> Not sure about "Service" -- the units could come from any mix of units. Maybe
> AddServiceUnits/DestroyServiceUnits throughout for (1) consistency and (2)
> additional sanity ("unit" is short for "service unit" anyway).

Done.

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

https://codereview.appspot.com/7554046/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:404: // We can't remove the last unit so we have to
add one first.
On 2013/03/14 16:28:24, rog wrote:
> On 2013/03/14 15:44:11, rog wrote:
> > On 2013/03/14 14:02:28, fwereade wrote:
> > > Can't we? Where's that restriction implemented?
> > 
> > Apart from that, this is actually testing the wrong call.
> > ServiceAddUnits will fail in exactly the same way that ServiceDestroyUnits
> will,
> > so AFAICS ServiceDestroyUnits will never be called here.
> > 
> > That leaves the question of how to implement this test, because destroying a
> > unit is not something that can be undone.
> > 
> > To be honest, I'm starting to have second feelings about all these opClient*
> > functions. They are all just testing exactly the same code (the
requireClient
> if
> > statement in srvRoot.Client) and they eat up a lot of time. I think I'd be
> happy
> > if we deleted all but one of them, and left a comment saying why one is
> > sufficient to test all Client entry points (and probably a comment in
> > srvRoot.Client too, as a warning against future structural changes there).
> > 
> > Thoughts?
> 
> after discussion with William, this is not a preferred approach, so I'd
suggest
> using mst to add the unit and Client.DestroyUnits to destroy it. unlike in
most
> of these functions, you'll have something to undo when the operation *fails*
> rather than succeeds.

Done.

https://codereview.appspot.com/7554046/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:404: // We can't remove the last unit so we have to
add one first.
On 2013/03/14 14:02:28, fwereade wrote:
> Can't we? Where's that restriction implemented?

That was a bad assumption on my part.  Fixed.

https://codereview.appspot.com/7554046/diff/1/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7554046/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:361: // ServiceDestroyUnits removes a given number
of units from a service.
On 2013/03/14 14:02:28, fwereade wrote:
> ...removes the given service units?
> 
> (similarly elsewhere)

Done.

https://codereview.appspot.com/7554046/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7554046/diff/1/state/state.go#newcode61
state/state.go:61: serviceSnippet = "^[a-z][a-z0-9]*(-[a-z0-9]*[a-z][a-z0-9]*)*"
On 2013/03/14 15:44:11, rog wrote:
> i think i'd leave the "^" off here, which makes serviceSnippet into a
> potentially useful thing if anywhere else needs to use a service name as part
of
> a regexp. then i'd name it "serviceNamePattern", making its value more
obvious.
> 
> then both the valid* variables below would add the leading "^", so it's
obvious
> at a glance that both of them are anchored.
> 
> also (trivial) serviceSnippet should be a const.

Done.

https://codereview.appspot.com/7554046/diff/1/state/state.go#newcode63
state/state.go:63: validUnit      = regexp.MustCompile(serviceSnippet +
"(-[a-z0-9]*[a-z][a-z0-9]*)*/[0-9]+$")
On 2013/03/14 14:32:53, fwereade wrote:
> On 2013/03/14 14:18:49, dimitern wrote:
> > I don't think this is the same:
> > 
> > validService(old) = "^serviceSnippet$"
> > validUnit(old) = ^serviceSnippet*/[0-9]+$"
> > validUnit(new) = "^serviceSnippet+{second part of serviceSnippet:
> > (-[a-z0-9]*[a-z][a-z0-9]*)*}+"/[0-9]+$"
> > 
> > So you're actually checking for
> > validUnit(new) =
> >
>
"^[a-z][a-z0-9]*(-[a-z0-9]*[a-z][a-z0-9]*)*(-[a-z0-9]*[a-z][a-z0-9]*)*/[0-9]+$"
> > 
> > Now you can have an extra (-[a-z0-9]*[a-z][a-z0-9]*)* part, which is both
> > redundant (already covered in serviceSnippet) and confusing IMHO.
> 
> Worse, I think it actually changes the meaning -- the duplicated part is
saying
> "no -{digits} suffixes allowed".

I'm pretty sure this is not the case.  The duplicated bit certainly shouldn't be
there, but in my reading of the regex the meaning with the duplicated part is
identical to the meaning without.

Regardless, I have removed the duplicate part and beefed up the tests for
IsServiceName.

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

https://codereview.appspot.com/7554046/diff/1/state/statecmd/destroyunit.go#n...
state/statecmd/destroyunit.go:17: err = conn.DestroyUnits(args.UnitNames...)
On 2013/03/14 14:18:49, dimitern wrote:
> just return conn.DestroyUnits(args.UnitNames...)

Done.

https://codereview.appspot.com/7554046/diff/1/state/statecmd/destroyunit.go#n...
state/statecmd/destroyunit.go:17: err = conn.DestroyUnits(args.UnitNames...)
On 2013/03/14 14:18:49, dimitern wrote:
> just return conn.DestroyUnits(args.UnitNames...)

Done.

[dimitern, 2013-03-15 13:52:36]

Much better, thanks! Although in the future, please repropose after applying the
suggested changes, rather than just publishing comments (lbox propose will
publish any drafts automatically), because now I can still see the old code, but
I assume it's changed. LGTM still.

https://codereview.appspot.com/7554046/diff/1/cmd/juju/destroyunit.go
File cmd/juju/destroyunit.go (right):

https://codereview.appspot.com/7554046/diff/1/cmd/juju/destroyunit.go#newcode41
cmd/juju/destroyunit.go:41: // DestroyUnitCommand connects to the environment
specified on the command line
On 2013/03/15 13:39:12, benji wrote:
> On 2013/03/14 14:18:49, dimitern wrote:
> > It's still Run(), so it starts with // Run ... . Probably move the more
> general
> > comment to the struct doc comment?
> 
> Good catch, I'm still not used to reading Go's function signatures.
> 
> This class of error would be caught by a good linter.  Is there such a thing
for
> Go?

Not that I know of, probably some quick research is in order. If there is
indeed, I'd gladly use it!

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

https://codereview.appspot.com/7554046/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:404: // We can't remove the last unit so we have to
add one first.
On 2013/03/15 13:39:12, benji wrote:
> On 2013/03/14 14:02:28, fwereade wrote:
> > Can't we? Where's that restriction implemented?
> 
> That was a bad assumption on my part.  Fixed.

I can still see the comment mentioning the assumption - should it be removed
now?

https://codereview.appspot.com/7554046/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7554046/diff/1/state/state.go#newcode61
state/state.go:61: serviceSnippet = "^[a-z][a-z0-9]*(-[a-z0-9]*[a-z][a-z0-9]*)*"
I can still see the old code - but I assume it's fixed and not reproposed?

[benji, 2013-03-15 14:16:32]

Please take a look.

https://codereview.appspot.com/7554046/diff/1/cmd/juju/destroyunit.go
File cmd/juju/destroyunit.go (right):

https://codereview.appspot.com/7554046/diff/1/cmd/juju/destroyunit.go#newcode41
cmd/juju/destroyunit.go:41: // DestroyUnitCommand connects to the environment
specified on the command line
On 2013/03/15 13:52:36, dimitern wrote:
> On 2013/03/15 13:39:12, benji wrote:
> > On 2013/03/14 14:18:49, dimitern wrote:
> > > It's still Run(), so it starts with // Run ... . Probably move the more
> > general
> > > comment to the struct doc comment?
> > 
> > Good catch, I'm still not used to reading Go's function signatures.
> > 
> > This class of error would be caught by a good linter.  Is there such a thing
> for
> > Go?
> 
> Not that I know of, probably some quick research is in order. If there is
> indeed, I'd gladly use it!

My search has found only an incomplete and abandoned linter.  For this use case
(linting docstrings) we could probably put something together relatively easily.
 The juju-gui team built a small yuidoc linter for use on that project.

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

https://codereview.appspot.com/7554046/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:404: // We can't remove the last unit so we have to
add one first.
On 2013/03/15 13:52:36, dimitern wrote:
> On 2013/03/15 13:39:12, benji wrote:
> > On 2013/03/14 14:02:28, fwereade wrote:
> > > Can't we? Where's that restriction implemented?
> > 
> > That was a bad assumption on my part.  Fixed.
> 
> I can still see the comment mentioning the assumption - should it be removed
> now?

My understanding of the semantics of "LGTM" is that no further review is
required once any "trivials" are addressed to the submitter's satisfaction,
therefore I did not re-propose after fixing.  Is that not the case?

By the way, I have resubmitted now, so you should be able to see the changes.

[benji, 2013-03-15 14:16:52]

See below for a report on go linters and a question.

Thanks.

[benji, 2013-03-15 15:21:10]

*** Submitted:

Add ServiceDestroyUnits to the API

R=fwereade, dimitern, rog
CC=
https://codereview.appspot.com/7554046