apiserver: common Remover mixin

apiserver: common Remover mixin

Like the LifeGetter and PasswordChanger, this mixin is
intended for composition into worker facades like the
deployer and provisioner - handles entities with Remove
and EnsureDead methods, handling authorization correctly.

Cleaned up some of minor things inside the other mixins
as well, added more tests.

https://code.launchpad.net/~dimitern/juju-core/068-api-common-remover/+merge/174014

(do not edit description out of merge proposal)

[dimitern, 2013-07-10 18:13:48]

Please take a look.

[fwereade, 2013-07-10 18:26:55]

LGTM, just trivials.

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/life_te...
File state/apiserver/common/life_test.go (left):

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:69: }
One more test that we *don't* error with no entities please -- that way we get
to test that addition.

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/passwor...
File state/apiserver/common/password_test.go (right):

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/passwor...
state/apiserver/common/password_test.go:85: _, err :=
pc.SetPasswords(params.PasswordChanges{Changes: changes})
ditto

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

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove....
state/apiserver/common/remove.go:14: getCanRead GetAuthFunc
getCanRemove

...or think of a smart common name for all of them, even

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove....
state/apiserver/common/remove.go:39: canRead, err := r.getCanRead()
better name

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove....
state/apiserver/common/remove.go:48: if err == nil {
maybe collapse this if as well, for symetry? or unpack the one underneath?

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove_...
File state/apiserver/common/remove_test.go (right):

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove_...
state/apiserver/common/remove_test.go:46: unauth := &params.Error{
hey, this is looking pretty common, might be worth moving out.

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove_...
state/apiserver/common/remove_test.go:68: c.Assert(err, ErrorMatches, "pow")
same test as asked for in life and setpasswords please

[jameinel, 2013-07-10 18:41:44]

I think with William's suggestions, LGTM.

I have some suggestions, but naming is hard. :)

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

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove....
state/apiserver/common/remove.go:14: getCanRead GetAuthFunc
On 2013/07/10 18:26:55, fwereade wrote:
> getCanRemove
> 
> ...or think of a smart common name for all of them, even

Remove sounds more like a Write action, isn't it?

getCanModify?

or even

getAuthModify
getHaveAuthModify

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove....
state/apiserver/common/remove.go:17: type RemoverGetter interface {
Removerer right?

If you add a doc string about:

// state.State implements Remover to provide ways for us to 
// call object.Remove (for Units, etc).
// This is used to allow us to test with mocks without having to actually bring
up state

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove....
state/apiserver/common/remove.go:48: if err == nil {
On 2013/07/10 18:26:55, fwereade wrote:
> maybe collapse this if as well, for symetry? or unpack the one underneath?

A couple times I did like pulling the inner loop into a helper function, so that
you can "return err" at any time, and it will set that in the result.Errors
rather than having to play tricks with nesting if statements.

[dimitern, 2013-07-10 18:58:50]

Please take a look.

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/life_te...
File state/apiserver/common/life_test.go (left):

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:69: }
On 2013/07/10 18:26:55, fwereade wrote:
> One more test that we *don't* error with no entities please -- that way we get
> to test that addition.

Was thinking to add it, but now you mentioned it as well - will do.

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/passwor...
File state/apiserver/common/password_test.go (right):

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/passwor...
state/apiserver/common/password_test.go:85: _, err :=
pc.SetPasswords(params.PasswordChanges{Changes: changes})
On 2013/07/10 18:26:55, fwereade wrote:
> ditto

Done.

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

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove....
state/apiserver/common/remove.go:14: getCanRead GetAuthFunc
On 2013/07/10 18:41:44, jameinel wrote:
> On 2013/07/10 18:26:55, fwereade wrote:
> > getCanRemove
> > 
> > ...or think of a smart common name for all of them, even
> 
> Remove sounds more like a Write action, isn't it?
> 
> getCanModify?
> 
> or even
> 
> getAuthModify
> getHaveAuthModify

So finally canModify and getCanModify it is.

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove....
state/apiserver/common/remove.go:17: type RemoverGetter interface {
On 2013/07/10 18:41:44, jameinel wrote:
> Removerer right?
> 
> If you add a doc string about:
> 
> // state.State implements Remover to provide ways for us to 
> // call object.Remove (for Units, etc).
> // This is used to allow us to test with mocks without having to actually
bring
> up state

Renamed to Removerer - I hate these conventions :)
Added doc comment as suggested.

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove....
state/apiserver/common/remove.go:39: canRead, err := r.getCanRead()
On 2013/07/10 18:26:55, fwereade wrote:
> better name

Done.

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove....
state/apiserver/common/remove.go:48: if err == nil {
On 2013/07/10 18:41:44, jameinel wrote:
> On 2013/07/10 18:26:55, fwereade wrote:
> > maybe collapse this if as well, for symetry? or unpack the one underneath?
> 
> A couple times I did like pulling the inner loop into a helper function, so
that
> you can "return err" at any time, and it will set that in the result.Errors
> rather than having to play tricks with nesting if statements.

Good idea, done.

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove_...
File state/apiserver/common/remove_test.go (right):

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove_...
state/apiserver/common/remove_test.go:46: unauth := &params.Error{
On 2013/07/10 18:26:55, fwereade wrote:
> hey, this is looking pretty common, might be worth moving out.

Good point - I'll move it to package-level var and reuse it in other tests.

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove_...
state/apiserver/common/remove_test.go:68: c.Assert(err, ErrorMatches, "pow")
On 2013/07/10 18:26:55, fwereade wrote:
> same test as asked for in life and setpasswords please

Done.

[dimitern, 2013-07-10 19:07:23]

https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove_...
state/apiserver/common/remove_test.go:46: unauth := &params.Error{
On 2013/07/10 18:26:55, fwereade wrote:
>> hey, this is looking pretty common, might be worth moving out.

>Good point - I'll move it to package-level var and reuse it >in other tests.

Whoops I forgot about this - will change it in a follow-up, sorry.