api(server)/uniter: RelationById API call

api(server)/uniter: RelationById API call

This is needed by the uniter in a couple of places
where relations are retrieved from state by id,
rather than by key. Having this will save some
unnecessary refactoring of the uniter code to
use tags always.

https://code.launchpad.net/~dimitern/juju-core/127-api-uniter-relationbyid/+merge/184532

(do not edit description out of merge proposal)

[dimitern, 2013-09-09 10:38:59]

Please take a look.

[rog, 2013-09-09 11:47:04]

Looks good with one minor query below.

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

https://codereview.appspot.com/13238046/diff/1/state/api/uniter/uniter.go#new...
state/api/uniter/uniter.go:140: err := st.caller.Call("Uniter", "",
"RelationById", args, &results)
As discussed online, it seems wrong that this makes two API round-trips, but I'm
OK for this to be fixed in another CL.

https://codereview.appspot.com/13238046/diff/1/state/api/uniter/uniter.go#new...
state/api/uniter/uniter.go:152: // Now get the life
d
(not really a very useful comment)

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

https://codereview.appspot.com/13238046/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:672: // Because we're not doing any other
authorization
I don't understand this comment. Can this failure ever happen in practice?

[dimitern, 2013-09-09 12:17:04]

Please take a look.

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

https://codereview.appspot.com/13238046/diff/1/state/api/uniter/uniter.go#new...
state/api/uniter/uniter.go:140: err := st.caller.Call("Uniter", "",
"RelationById", args, &results)
On 2013/09/09 11:47:04, rog wrote:
> As discussed online, it seems wrong that this makes two API round-trips, but
I'm
> OK for this to be fixed in another CL.

Yeah, as discussed, will be fixed in a follow-up.

https://codereview.appspot.com/13238046/diff/1/state/api/uniter/uniter.go#new...
state/api/uniter/uniter.go:152: // Now get the life
On 2013/09/09 11:47:04, rog wrote:
> d
> (not really a very useful comment)

Removed.

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

https://codereview.appspot.com/13238046/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:672: // Because we're not doing any other
authorization
On 2013/09/09 11:47:04, rog wrote:
> I don't understand this comment. Can this failure ever happen in practice?

Yes, it can happen - take a look at relation.Endpoint.
The point of the comment is we're not using any access*() methods to make sure
we can access the relation.

[rog, 2013-09-09 12:28:05]

Thanks for the clarification. I think this could be a little clearer. Perhaps a
better comment is all that's needed though.

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

https://codereview.appspot.com/13238046/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:672: // Because we're not doing any other
authorization
On 2013/09/09 12:17:04, dimitern wrote:
> On 2013/09/09 11:47:04, rog wrote:
> > I don't understand this comment. Can this failure ever happen in practice?
> 
> Yes, it can happen - take a look at relation.Endpoint.
> The point of the comment is we're not using any access*() methods to make sure
> we can access the relation.

Ah, I see - prepareRelationResult can only fail if the given relation does not
involve the service of the currently authenticated unit. That seems a bit
non-obvious to me. Perhaps state.Relation.Endpoint should be changed so it
returns a not-found error, then we can check for that error explicitly here.

As it is, I don't think it's clear that the only way that prepareRelationResult
can fail is because the relation is with a unit from a different service, and
that's why it's appropriate to return ErrPerm.

[dimitern, 2013-09-09 12:43:18]

Please take a look.

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

https://codereview.appspot.com/13238046/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:672: // Because we're not doing any other
authorization
On 2013/09/09 12:28:05, rog wrote:
> On 2013/09/09 12:17:04, dimitern wrote:
> > On 2013/09/09 11:47:04, rog wrote:
> > > I don't understand this comment. Can this failure ever happen in practice?
> > 
> > Yes, it can happen - take a look at relation.Endpoint.
> > The point of the comment is we're not using any access*() methods to make
sure
> > we can access the relation.
> 
> Ah, I see - prepareRelationResult can only fail if the given relation does not
> involve the service of the currently authenticated unit. That seems a bit
> non-obvious to me. Perhaps state.Relation.Endpoint should be changed so it
> returns a not-found error, then we can check for that error explicitly here.
> 
> As it is, I don't think it's clear that the only way that
prepareRelationResult
> can fail is because the relation is with a unit from a different service, and
> that's why it's appropriate to return ErrPerm.

Done as discussed online - updated the comment.

[rog, 2013-09-09 12:56:11]

LGTM with the "most likely" qualifier removed, as discussed.

[dimitern, 2013-09-09 12:59:04]

Please take a look.