apiserver: Uniter API unit ops (cont.)

apiserver: Uniter API unit ops (cont.)

Implemeted a few additional uniter API unit methods:
* Destroy
* GetPrincipal
* SubordinateNames
* ClearResolved

https://code.launchpad.net/~dimitern/juju-core/095-api-uniter-unit-ops3/+merge/179448

Requires: https://code.launchpad.net/~dimitern/juju-core/094-apiserver-uniter-unit-ops2/+merge/179421

(do not edit description out of merge proposal)

[dimitern, 2013-08-09 13:50:55]

Please take a look.

[dimitern, 2013-08-09 13:52:30]

Please take a look.

[fwereade, 2013-08-09 14:40:36]

not a review, just a couple of comments

https://codereview.appspot.com/12698043/diff/7001/state/apiserver/uniter/unit...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12698043/diff/7001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:173: func (u *UniterAPI) SetResolved(args
params.SetEntitiesResolved) (params.ErrorResults, error) {
This isn't used by uniter, is it?

https://codereview.appspot.com/12698043/diff/7001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:214: unit, err = u.getUnit(entity.Tag)
Yeah, this should handle NotFounds and turn them into ErrPerms, shouldn't it? I
suspect there are quite a lot like this...

https://codereview.appspot.com/12698043/diff/7001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:225: // given unit, if possible.
*surely* we don't need an API call to strip /<number> off the unit name, do we?

[dimitern, 2013-08-09 14:49:46]

Please take a look.

https://codereview.appspot.com/12698043/diff/7001/state/apiserver/uniter/unit...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12698043/diff/7001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:173: func (u *UniterAPI) SetResolved(args
params.SetEntitiesResolved) (params.ErrorResults, error) {
On 2013/08/09 14:40:36, fwereade wrote:
> This isn't used by uniter, is it?

Hmm.. you're right actually - it's used only in unite tests. Will remove it.
Thanks!

https://codereview.appspot.com/12698043/diff/7001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:214: unit, err = u.getUnit(entity.Tag)
On 2013/08/09 14:40:36, fwereade wrote:
> Yeah, this should handle NotFounds and turn them into ErrPerms, shouldn't it?
I
> suspect there are quite a lot like this...

It does, check the tests with unit-foo-42.

https://codereview.appspot.com/12698043/diff/7001/state/apiserver/uniter/unit...
state/apiserver/uniter/uniter.go:225: // given unit, if possible.
On 2013/08/09 14:40:36, fwereade wrote:
> *surely* we don't need an API call to strip /<number> off the unit name, do
we?

Don't we? OK then, will remove it and keep it client-side only, if you think so.

[dimitern, 2013-08-09 14:54:19]

Please take a look.

[dimitern, 2013-08-09 15:44:58]

Please take a look.

[natefinch, 2013-08-09 16:31:32]

LGTM. If it were me, I'd make the table driven tests have the inputs and results
together, so it's easier to see how each input maps to a result... however, I
know you're just following the pattern the rest of the tests take.

[jameinel, 2013-08-10 15:16:33]

On 2013/08/09 16:31:32, nate.finch wrote:
> LGTM. If it were me, I'd make the table driven tests have the inputs and
results
> together, so it's easier to see how each input maps to a result... however, I
> know you're just following the pattern the rest of the tests take.

I don't think these are "table driven tests" as much as a multi parameter
request that has multi-parameter results.

It would be reasonable to have:

args := [...]
results := [...]

AssertResult(Request(params), DeepEquals, Results)

if you wanted.

But the reason they are split is because we send and receive a list of things,
not because we are iterating over them one by one.

[gz, 2013-08-12 09:01:54]

LGTM if the calls get the permissions right, see comments.

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:177: canModify, err := u.getCanRead()
The variable name is canModify, but the function being called is getCanRead?

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:232: canModify, err := u.getCanRead()
Again, modify or read?

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter_test.go (right):

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter_test.go:444: c.Assert(err, gc.IsNil)
As the second chain of actions+asserts, feels like a helper addService function
is warrented.

[rog, 2013-08-12 09:12:52]

LGTM module the below suggestions
and +1 to other people's comments.

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:181: var unit *state.Unit

I'd declare this inside the canModify test, to
make its intended scope more clear.

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:207: var unit *state.Unit
ditto

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:236: var unit *state.Unit
ditto

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:265: if err == nil {

Why are we ignoring the non-nil error here?

Endless repetition of this same code template
does not make me happy.

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter_test.go (right):

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter_test.go:444: c.Assert(err, gc.IsNil)
On 2013/08/12 09:01:55, gz wrote:
> As the second chain of actions+asserts, feels like a helper addService
function
> is warrented.

+1
even if it's just a closure declared inside the test itself
and called twice with different arguments.

[dimitern, 2013-08-12 09:25:50]

Please take a look.

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:177: canModify, err := u.getCanRead()
On 2013/08/12 09:01:55, gz wrote:
> The variable name is canModify, but the function being called is getCanRead?

I'll rename it to canAccess and getCanAccess, as discussed online.

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:181: var unit *state.Unit
On 2013/08/12 09:12:52, rog wrote:
> 
> I'd declare this inside the canModify test, to
> make its intended scope more clear.

Done.

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:207: var unit *state.Unit
On 2013/08/12 09:12:52, rog wrote:
> ditto

Done.

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:232: canModify, err := u.getCanRead()
On 2013/08/12 09:01:55, gz wrote:
> Again, modify or read?

See above.

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:236: var unit *state.Unit
On 2013/08/12 09:12:52, rog wrote:
> ditto

Done.

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:265: if err == nil {
On 2013/08/12 09:12:52, rog wrote:
> 
> Why are we ignoring the non-nil error here?
> 
> Endless repetition of this same code template
> does not make me happy.

Because there's no error to report here - we just return an empty string.

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter_test.go (right):

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter_test.go:444: c.Assert(err, gc.IsNil)
On 2013/08/12 09:01:55, gz wrote:
> As the second chain of actions+asserts, feels like a helper addService
function
> is warrented.

Done.

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter_test.go:444: c.Assert(err, gc.IsNil)
On 2013/08/12 09:12:52, rog wrote:
> On 2013/08/12 09:01:55, gz wrote:
> > As the second chain of actions+asserts, feels like a helper addService
> function
> > is warrented.
> 
> +1
> even if it's just a closure declared inside the test itself
> and called twice with different arguments.

Done.

[rog, 2013-08-12 09:45:00]

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:265: if err == nil {
On 2013/08/12 09:25:50, dimitern wrote:
> On 2013/08/12 09:12:52, rog wrote:
> > 
> > Why are we ignoring the non-nil error here?
> > 
> > Endless repetition of this same code template
> > does not make me happy.
> 
> Because there's no error to report here - we just return an empty string.

What if there's a network error trying to fetch the
unit from the database? I don't think we want to
return as if the unit has no subordinates in that case.

[jameinel, 2013-08-12 13:41:13]

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/12698043/diff/17001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:265: if err == nil {
On 2013/08/12 09:45:01, rog wrote:
> On 2013/08/12 09:25:50, dimitern wrote:
> > On 2013/08/12 09:12:52, rog wrote:
> > > 
> > > Why are we ignoring the non-nil error here?
> > > 
> > > Endless repetition of this same code template
> > > does not make me happy.
> > 
> > Because there's no error to report here - we just return an empty string.
> 
> What if there's a network error trying to fetch the
> unit from the database? I don't think we want to
> return as if the unit has no subordinates in that case.

Aren't we missing the line:
results.Results[i].Error = common.ServerError(err)

At least for the other API calls we support individual error statements for each
entity we are asking about. (see line 190 above)