api/uniter: Relation and RelationById take 1 call

api/uniter: Relation and RelationById take 1 call

Instead of making 2 server API calls to retrieve
a single relation as before, now it takes just
one call. Added Life as part of the RelationResult
and changed the server-side uniter API not to report
relations lifecycle with Life().

https://code.launchpad.net/~dimitern/juju-core/128-api-uniter-refactor-relations-calls/+merge/184564

Requires: 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 12:49:30]

Please take a look.

[rog, 2013-09-09 13:00:22]

Nice change, thanks. Just a few remarks below.

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

https://codereview.appspot.com/13511044/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:154: // local endpoint for a single relation or an
error.
How about:
// RelationResult returns information about a single relation,
// or an error.

?
The information returned should be evident from the fields in the struct (which,
BTW, could probably do with some documentation themselves at some point)

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

https://codereview.appspot.com/13511044/diff/1/state/api/uniter/relation.go#n...
state/api/uniter/relation.go:55: r.life = result.Life
// Note: The life cycle information is the only
// thing that can change - id, tag and endpoint
// information are static.

?

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

https://codereview.appspot.com/13511044/diff/1/state/apiserver/uniter/uniter_...
state/apiserver/uniter/uniter_test.go:169: c.Assert(rel.Life(), gc.Equals,
state.Dying)
why not leave this in?

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

https://codereview.appspot.com/13511044/diff/1/state/apiserver/uniter/uniter_...
state/apiserver/uniter/uniter_test.go:190: {Error:
apiservertesting.ErrUnauthorized},
this change seems odd - aren't we allowed to ask for the life status of a dying
relation?

[dimitern, 2013-09-09 13:06:36]

Please take a look.

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

https://codereview.appspot.com/13511044/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:154: // local endpoint for a single relation or an
error.
On 2013/09/09 13:00:22, rog wrote:
> How about:
> // RelationResult returns information about a single relation,
> // or an error.
> 
> ?
> The information returned should be evident from the fields in the struct
(which,
> BTW, could probably do with some documentation themselves at some point)

Done.

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

https://codereview.appspot.com/13511044/diff/1/state/api/uniter/relation.go#n...
state/api/uniter/relation.go:55: r.life = result.Life
On 2013/09/09 13:00:22, rog wrote:
> // Note: The life cycle information is the only
> // thing that can change - id, tag and endpoint
> // information are static.
> 
> ?

Done.

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

https://codereview.appspot.com/13511044/diff/1/state/apiserver/uniter/uniter_...
state/apiserver/uniter/uniter_test.go:169: c.Assert(rel.Life(), gc.Equals,
state.Dying)
On 2013/09/09 13:00:22, rog wrote:
> why not leave this in?

We don't really need this anymore - it was used to ensure the prerequisites of
testing the relation's lifecycle.

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

https://codereview.appspot.com/13511044/diff/1/state/apiserver/uniter/uniter_...
state/apiserver/uniter/uniter_test.go:190: {Error:
apiservertesting.ErrUnauthorized},
On 2013/09/09 13:00:22, rog wrote:
> this change seems odd - aren't we allowed to ask for the life status of a
dying
> relation?

As the description says, we can no longer use Life() to ask for relations at
all.

[rog, 2013-09-09 13:27:56]

On 2013/09/09 13:06:36, dimitern wrote:
> Please take a look.
> 
> https://codereview.appspot.com/13511044/diff/1/state/api/params/internal.go
> File state/api/params/internal.go (right):
> 
>
https://codereview.appspot.com/13511044/diff/1/state/api/params/internal.go#n...
> state/api/params/internal.go:154: // local endpoint for a single relation or
an
> error.
> On 2013/09/09 13:00:22, rog wrote:
> > How about:
> > // RelationResult returns information about a single relation,
> > // or an error.
> > 
> > ?
> > The information returned should be evident from the fields in the struct
> (which,
> > BTW, could probably do with some documentation themselves at some point)
> 
> Done.
> 
> https://codereview.appspot.com/13511044/diff/1/state/api/uniter/relation.go
> File state/api/uniter/relation.go (right):
> 
>
https://codereview.appspot.com/13511044/diff/1/state/api/uniter/relation.go#n...
> state/api/uniter/relation.go:55: r.life = result.Life
> On 2013/09/09 13:00:22, rog wrote:
> > // Note: The life cycle information is the only
> > // thing that can change - id, tag and endpoint
> > // information are static.
> > 
> > ?
> 
> Done.
> 
>
https://codereview.appspot.com/13511044/diff/1/state/apiserver/uniter/uniter_...
> File state/apiserver/uniter/uniter_test.go (left):
> 
>
https://codereview.appspot.com/13511044/diff/1/state/apiserver/uniter/uniter_...
> state/apiserver/uniter/uniter_test.go:169: c.Assert(rel.Life(), gc.Equals,
> state.Dying)
> On 2013/09/09 13:00:22, rog wrote:
> > why not leave this in?
> 
> We don't really need this anymore - it was used to ensure the prerequisites of
> testing the relation's lifecycle.
> 
>
https://codereview.appspot.com/13511044/diff/1/state/apiserver/uniter/uniter_...
> File state/apiserver/uniter/uniter_test.go (right):
> 
>
https://codereview.appspot.com/13511044/diff/1/state/apiserver/uniter/uniter_...
> state/apiserver/uniter/uniter_test.go:190: {Error:
> apiservertesting.ErrUnauthorized},
> On 2013/09/09 13:00:22, rog wrote:
> > this change seems odd - aren't we allowed to ask for the life status of a
> dying
> > relation?
> 
> As the description says, we can no longer use Life() to ask for relations at
> all.

LGTM, thanks!