apiserver/uniter: SubordinateNames returns Error

apiserver/uniter: SubordinateNames returns Error

As suggested by a previous review, SubordinateNames
now have an Error field in its result.

https://code.launchpad.net/~dimitern/juju-core/096-apiserver-uniter-unit-ops4/+merge/179661

(do not edit description out of merge proposal)

[dimitern, 2013-08-12 10:25:26]

Please take a look.

[gz, 2013-08-12 10:30:04]

LGTM.

[rog, 2013-08-12 10:34:17]

LGTM modulo the below considerations.

https://codereview.appspot.com/12748043/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/12748043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:35: type StringsErrorResults struct {
I think this should probably just be StringsResults
(all the other Results types return an error
too).

If you put a `json:",omitempty"` on the Error field
in the result (and possibly on all the other Error
fields too, for consistency and to save network bandwidth
in the usual case) then you can continue to use
StringsResult for the result of the StateAddresses call.

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

https://codereview.appspot.com/12748043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:264: var unit *state.Unit
Please declare this in the closest scope
that is possible (inside the canAccess test block).

The same goes for similar code above.
The smaller the scope, the less places to
look for possible references to the variable.

https://codereview.appspot.com/12748043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:276: // TODO(dimitern): Add the other needed
API calls.
Perhaps consider adding a TODO for each call to be added.
That way it's easy for people to pick them off without referring to
a separate list.

https://codereview.appspot.com/12748043/diff/1/state/apiserver/uniter/uniter_...
File state/apiserver/uniter/uniter_test.go (right):

https://codereview.appspot.com/12748043/diff/1/state/apiserver/uniter/uniter_...
state/apiserver/uniter/uniter_test.go:449: {Error:
apiservertesting.ErrUnauthorized},
It would be nice if this could check that an error
from the unit get call was passed back, but I'm
happy if that's too hard to arrange.

[dimitern, 2013-08-12 11:04:20]

Please take a look.

https://codereview.appspot.com/12748043/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/12748043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:35: type StringsErrorResults struct {
On 2013/08/12 10:34:17, rog wrote:
> I think this should probably just be StringsResults
> (all the other Results types return an error
> too).
> 
> If you put a `json:",omitempty"` on the Error field
> in the result (and possibly on all the other Error
> fields too, for consistency and to save network bandwidth
> in the usual case) then you can continue to use
> StringsResult for the result of the StateAddresses call.

-1 on `json:",omitempty"` for this CL, otherwise I'll do as you suggest.

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

https://codereview.appspot.com/12748043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:264: var unit *state.Unit
On 2013/08/12 10:34:17, rog wrote:
> Please declare this in the closest scope
> that is possible (inside the canAccess test block).
> 
> The same goes for similar code above.
> The smaller the scope, the less places to
> look for possible references to the variable.

Done.

https://codereview.appspot.com/12748043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:276: // TODO(dimitern): Add the other needed
API calls.
On 2013/08/12 10:34:17, rog wrote:
> Perhaps consider adding a TODO for each call to be added.
> That way it's easy for people to pick them off without referring to
> a separate list.

Done.

https://codereview.appspot.com/12748043/diff/1/state/apiserver/uniter/uniter_...
File state/apiserver/uniter/uniter_test.go (right):

https://codereview.appspot.com/12748043/diff/1/state/apiserver/uniter/uniter_...
state/apiserver/uniter/uniter_test.go:449: {Error:
apiservertesting.ErrUnauthorized},
On 2013/08/12 10:34:17, rog wrote:
> It would be nice if this could check that an error
> from the unit get call was passed back, but I'm
> happy if that's too hard to arrange.

Well, we have to inject some entity with a unit tag, but which is not a
state.Unit. Or, if we did have a way to introduce state connection errors at
will, it could be used here. I don't see how I do any of these easily now,
without significant changes to state code.

[dimitern, 2013-08-12 11:07:15]

Please take a look.