state/apiserver: Relation ops for Uniter API

state/apiserver: Relation ops for Uniter API

This adds the Relation() method on the server-side
uniter API facade, which returns the id, key and 
all endpoints of a relation, identified by its tag.
Life() also handles relation tags now.
One misc call was added - EnvironUUID().

Needed to add state.Relation.Endpoints() call
to return all endpoints of a relation, and since
they don't change after the relation is created,
we can cache them at the API client-side, hence
the Relation() method that returns all endpoints.

Once we have change the relation tags format
from using the numeric IDs to (properly escaped
and validated) relation keys, there'll be a
follow-up to fix the methods above to use the
new format.

https://code.launchpad.net/~dimitern/juju-core/105-apiserver-uniter-relation-ops/+merge/180321

(do not edit description out of merge proposal)

[dimitern, 2013-08-15 11:43:46]

Please take a look.

[rog, 2013-08-15 12:28:58]

LGTM modulo some thoughts below.

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

https://codereview.appspot.com/12990043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:88: Id        string
Shouldn't this be an int?

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

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:60: getCanAccessUnitServiceOrRelation := func()
(common.AuthFunc, error) {
One thought: rather than defining one function for each combination, you
could define one function for each of {unit, service, relation} and combine
them with a higher order function.

For example:

// either returns an AuthFunc generator that returns
// an AuthFunc that accepts any tag authorized by
// either of its arguments.
func either(a, b func() (common.AuthFunc, error)) func() (common.AuthFunc,
error) {
    return func() (common.AuthFunc, error) {
        f1, err := a()
        if err != nil {
             return nil, err
        }
        f2, err := b()
        if err != nil {
            return nil, err
        }
        return func(tag string) bool {
            return f1(tag) || f2(tag)
        }
    }
}
(that function could potentially be defined in common)

Then you can define:

  getCanAccessService = func() (common.AuthFunc, error) {
       return authorizer.AuthOwner, nil
  }
  getCanAccessUnit = func() (common.AuthFunc, error) {
      unit, ok := authorizer.GetAuthEntity().(*state.Unit)
      etc...
  }

as before, but we can combine them arbitrarily, for example:

    either(canAccessUnit, either(canAccessService, canAccessRelation))

It's more flexible (and no need to write more code when we want
more combinations), but YMMV.

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:573: func stateEndpointToParams(ep
*state.Endpoint) *params.Endpoint {
Why not return the endpoint by value since we're dereferencing it
immediately anyway?

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:574: if ep == nil {
This can't happen.

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:579: Relation: charm.Relation{
Relation: ep.Relation,

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:597: rel, err := u.getOneRelation(entity.Tag)
We're using "result.Results[i]" so many times in this
body, it's probably worth doing:

result := &results.Results[i]

(assuming that you rename "result" to "results" at the top of the function)

Then you can do:
result.Id = ...
result.Key = 

etc

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:614: func (u *UniterAPI) EnvironUUID(args
params.Entities) (params.StringResults, error) {
I'd be tempted to just lose the arguments entirely and
just return the UUID of the current environment.

I can't forsee a time when we're using a uniter API that
represents more than one environment, and the unit
agent will always want the UUID of the current environment
regardless.

Perhaps name the call "CurrentEnvironUUID" just to guard
against the possibility of multiple environments in the future.

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

https://codereview.appspot.com/12990043/diff/1/state/relation.go#newcode244
state/relation.go:244: return r.doc.Endpoints
This is potentially dubious because it allows the caller
to mutate the endpoints in the doc.

I suggest documenting that fact that that's not allow,
or returning a copy of the slice.

It should probably have a test too, even if it is
utterly trivial.

[fwereade, 2013-08-15 15:57:29]

I'm pretty much 100% in agreement with rog, with a few embellishments.

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

https://codereview.appspot.com/12990043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:88: Id        string
On 2013/08/15 12:28:58, rog wrote:
> Shouldn't this be an int?

+1

https://codereview.appspot.com/12990043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:91: }
Would it be sane to have a RelationInfo with Id, Key, Endpoints?

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

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:614: func (u *UniterAPI) EnvironUUID(args
params.Entities) (params.StringResults, error) {
On 2013/08/15 12:28:58, rog wrote:
> I'd be tempted to just lose the arguments entirely and
> just return the UUID of the current environment.
> 
> I can't forsee a time when we're using a uniter API that
> represents more than one environment, and the unit
> agent will always want the UUID of the current environment
> regardless.
> 
> Perhaps name the call "CurrentEnvironUUID" just to guard
> against the possibility of multiple environments in the future.
> 

+1 again

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

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter_...
state/apiserver/uniter/uniter_test.go:788: {Tag: env.Tag()},
Yeah, +10 to CurrentEnvironUUID. We should not be depending on environ tags at
all if we can help it, they're definitely crack (they're derived from environ
names, which aren't globally unique; the only option is to always treat them as
aliases).

(Where we're stuck with them, we just have to deal with it, for now; it should
be fixed but that's a derail at this stage.)

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

https://codereview.appspot.com/12990043/diff/1/state/relation.go#newcode244
state/relation.go:244: return r.doc.Endpoints
On 2013/08/15 12:28:58, rog wrote:
> This is potentially dubious because it allows the caller
> to mutate the endpoints in the doc.
> 
> I suggest documenting that fact that that's not allow,
> or returning a copy of the slice.
> 
> It should probably have a test too, even if it is
> utterly trivial.

+1 to all this, but please consider the possibility of the uniter API always
returning a relation with a single endpoint -- which is definitely the only
relevant one -- being the one attached to the service the connected unit is a
member of. I think it'd simplify some things quite a lot. Would allow us to drop
this entirely, and return just Id/Key/LocalEndpoint in RelationResult (or Info,
if you decide that's sane).

[dimitern, 2013-08-15 16:56:56]

Please take a look.

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

https://codereview.appspot.com/12990043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:88: Id        string
On 2013/08/15 15:57:29, fwereade wrote:
> On 2013/08/15 12:28:58, rog wrote:
> > Shouldn't this be an int?
> 
> +1

Done.

https://codereview.appspot.com/12990043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:91: }
On 2013/08/15 15:57:29, fwereade wrote:
> Would it be sane to have a RelationInfo with Id, Key, Endpoints?

I don't quite get what you're asking?

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

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:60: getCanAccessUnitServiceOrRelation := func()
(common.AuthFunc, error) {
On 2013/08/15 12:28:58, rog wrote:
> One thought: rather than defining one function for each combination, you
> could define one function for each of {unit, service, relation} and combine
> them with a higher order function.
> 
> For example:
> 
> // either returns an AuthFunc generator that returns
> // an AuthFunc that accepts any tag authorized by
> // either of its arguments.
> func either(a, b func() (common.AuthFunc, error)) func() (common.AuthFunc,
> error) {
>     return func() (common.AuthFunc, error) {
>         f1, err := a()
>         if err != nil {
>              return nil, err
>         }
>         f2, err := b()
>         if err != nil {
>             return nil, err
>         }
>         return func(tag string) bool {
>             return f1(tag) || f2(tag)
>         }
>     }
> }
> (that function could potentially be defined in common)
> 
> Then you can define:
> 
>   getCanAccessService = func() (common.AuthFunc, error) {
>        return authorizer.AuthOwner, nil
>   }
>   getCanAccessUnit = func() (common.AuthFunc, error) {
>       unit, ok := authorizer.GetAuthEntity().(*state.Unit)
>       etc...
>   }
> 
> as before, but we can combine them arbitrarily, for example:
> 
>     either(canAccessUnit, either(canAccessService, canAccessRelation))
> 
> It's more flexible (and no need to write more code when we want
> more combinations), but YMMV.

Done.

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:573: func stateEndpointToParams(ep
*state.Endpoint) *params.Endpoint {
On 2013/08/15 12:28:58, rog wrote:
> Why not return the endpoint by value since we're dereferencing it
> immediately anyway?

Done.

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:574: if ep == nil {
On 2013/08/15 12:28:58, rog wrote:
> This can't happen.

Done.

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:579: Relation: charm.Relation{
On 2013/08/15 12:28:58, rog wrote:
> Relation: ep.Relation,

Done.

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:597: rel, err := u.getOneRelation(entity.Tag)
On 2013/08/15 12:28:58, rog wrote:
> We're using "result.Results[i]" so many times in this
> body, it's probably worth doing:
> 
> result := &results.Results[i]
> 
> (assuming that you rename "result" to "results" at the top of the function)
> 
> Then you can do:
> result.Id = ...
> result.Key = 
> 
> etc

Done.

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:614: func (u *UniterAPI) EnvironUUID(args
params.Entities) (params.StringResults, error) {
On 2013/08/15 15:57:29, fwereade wrote:
> On 2013/08/15 12:28:58, rog wrote:
> > I'd be tempted to just lose the arguments entirely and
> > just return the UUID of the current environment.
> > 
> > I can't forsee a time when we're using a uniter API that
> > represents more than one environment, and the unit
> > agent will always want the UUID of the current environment
> > regardless.
> > 
> > Perhaps name the call "CurrentEnvironUUID" just to guard
> > against the possibility of multiple environments in the future.
> > 
> 
> +1 again

Done.

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

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter_...
state/apiserver/uniter/uniter_test.go:788: {Tag: env.Tag()},
On 2013/08/15 15:57:29, fwereade wrote:
> Yeah, +10 to CurrentEnvironUUID. We should not be depending on environ tags at
> all if we can help it, they're definitely crack (they're derived from environ
> names, which aren't globally unique; the only option is to always treat them
as
> aliases).
> 
> (Where we're stuck with them, we just have to deal with it, for now; it should
> be fixed but that's a derail at this stage.)

Done.

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

https://codereview.appspot.com/12990043/diff/1/state/relation.go#newcode244
state/relation.go:244: return r.doc.Endpoints
On 2013/08/15 12:28:58, rog wrote:
> This is potentially dubious because it allows the caller
> to mutate the endpoints in the doc.
> 
> I suggest documenting that fact that that's not allow,
> or returning a copy of the slice.
> 
> It should probably have a test too, even if it is
> utterly trivial.

Ah, good point. Will return a copy of the slice and add a test.

https://codereview.appspot.com/12990043/diff/1/state/relation.go#newcode244
state/relation.go:244: return r.doc.Endpoints
On 2013/08/15 15:57:29, fwereade wrote:
> On 2013/08/15 12:28:58, rog wrote:
> > This is potentially dubious because it allows the caller
> > to mutate the endpoints in the doc.
> > 
> > I suggest documenting that fact that that's not allow,
> > or returning a copy of the slice.
> > 
> > It should probably have a test too, even if it is
> > utterly trivial.
> 
> +1 to all this, but please consider the possibility of the uniter API always
> returning a relation with a single endpoint -- which is definitely the only
> relevant one -- being the one attached to the service the connected unit is a
> member of. I think it'd simplify some things quite a lot. Would allow us to
drop
> this entirely, and return just Id/Key/LocalEndpoint in RelationResult (or
Info,
> if you decide that's sane).

I don't quite get that. Please, expand?

[rog, 2013-08-15 17:19:09]

A few more possible simplifications below.

LGTM with those done.

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

https://codereview.appspot.com/12990043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:60: getCanAccessUnitServiceOrRelation := func()
(common.AuthFunc, error) {
On 2013/08/15 16:56:56, dimitern wrote:
> On 2013/08/15 12:28:58, rog wrote:
> > One thought: rather than defining one function for each combination, you
> > could define one function for each of {unit, service, relation} and combine
> > them with a higher order function.
> > 
> > For example:
> > 
> > // either returns an AuthFunc generator that returns
> > // an AuthFunc that accepts any tag authorized by
> > // either of its arguments.
> > func either(a, b func() (common.AuthFunc, error)) func() (common.AuthFunc,
> > error) {
> >     return func() (common.AuthFunc, error) {
> >         f1, err := a()
> >         if err != nil {
> >              return nil, err
> >         }
> >         f2, err := b()
> >         if err != nil {
> >             return nil, err
> >         }
> >         return func(tag string) bool {
> >             return f1(tag) || f2(tag)
> >         }
> >     }
> > }
> > (that function could potentially be defined in common)
> > 
> > Then you can define:
> > 
> >   getCanAccessService = func() (common.AuthFunc, error) {
> >        return authorizer.AuthOwner, nil
> >   }
> >   getCanAccessUnit = func() (common.AuthFunc, error) {
> >       unit, ok := authorizer.GetAuthEntity().(*state.Unit)
> >       etc...
> >   }
> > 
> > as before, but we can combine them arbitrarily, for example:
> > 
> >     either(canAccessUnit, either(canAccessService, canAccessRelation))
> > 
> > It's more flexible (and no need to write more code when we want
> > more combinations), but YMMV.
> 
> Done.

Erm, not really done.

I'd thought we could lose the getCanAccessUnitOrService and
getCanAccessUnitServiceOrRelation functions in favour of a more general
combinator function that makes it trivial to produce new combinations without
writing more specific functions. Perhaps you're not keen on functional-style
programming though - I can go with that if that's your objection.

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

https://codereview.appspot.com/12990043/diff/10001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:601: r.Endpoints[j] =
stateEndpointToParams(relEp)
Since stateEndpointToParams is now so simple, you could just inline it.

https://codereview.appspot.com/12990043/diff/10001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:610: func (u *UniterAPI) CurrentEnvironUUID()
(params.StringResult, error) {
I think this can just be:

func (u *UniterAPI) CurrentEnvironUUID() (params.StringResult, error) {
    uuid, err := u.st.Environment()
    return params.StringResult{
       Result: uuid,
    }, err
}

We don't want to turn a network error into ErrPerm;
we can just use the normal error return because this
isn't a bulk call, and the rpc package will automatically
call ServerError on errors returned like this.

https://codereview.appspot.com/12990043/diff/10001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/12990043/diff/10001/state/relation.go#newcode244
state/relation.go:244: var eps []Endpoint
return append([]EndPoint{}, r.doc.Endpoints...)

[dimitern, 2013-08-15 18:06:37]

Please take a look.

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

https://codereview.appspot.com/12990043/diff/10001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:601: r.Endpoints[j] =
stateEndpointToParams(relEp)
On 2013/08/15 17:19:09, rog wrote:
> Since stateEndpointToParams is now so simple, you could just inline it.

Done.

https://codereview.appspot.com/12990043/diff/10001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:610: func (u *UniterAPI) CurrentEnvironUUID()
(params.StringResult, error) {
On 2013/08/15 17:19:09, rog wrote:
> I think this can just be:
> 
> func (u *UniterAPI) CurrentEnvironUUID() (params.StringResult, error) {
>     uuid, err := u.st.Environment()
>     return params.StringResult{
>        Result: uuid,
>     }, err
> }
> 
> We don't want to turn a network error into ErrPerm;
> we can just use the normal error return because this
> isn't a bulk call, and the rpc package will automatically
> call ServerError on errors returned like this.

That's not exactly right: I don't get UUID from u.st.Environment(), I get a
state.Environment object, on which I call UUID(). So, the network error is no
longer hidden, but I did it slightly differently.

https://codereview.appspot.com/12990043/diff/10001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/12990043/diff/10001/state/relation.go#newcode244
state/relation.go:244: var eps []Endpoint
On 2013/08/15 17:19:09, rog wrote:
> return append([]EndPoint{}, r.doc.Endpoints...)

I removed this altogether, following William's suggestion to return only current
unit's relation endpoint.

[rog, 2013-08-15 19:00:37]

LGTM with some considerations below, thanks!

https://codereview.appspot.com/12990043/diff/20001/state/apiserver/common/int...
File state/apiserver/common/interfaces.go (right):

https://codereview.appspot.com/12990043/diff/20001/state/apiserver/common/int...
state/apiserver/common/interfaces.go:50: func Either(a, b func() (AuthFunc,
error)) func() (AuthFunc, error) {
AuthEither, since it's in common?
Also, it should have a test as it's exported.
(fairly trivial to test exhaustively though)

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

https://codereview.appspot.com/12990043/diff/20001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:58: getCanAccessUnitOrService := common.Either(
Thanks! I hope you agree that reads quite nicely.

However I wonder... do you think these justify their place as UniterAPI fields
now that they're so easy to calculate in place?

I'd be tempted to use common.Either in place in the methods themselves - it
expresses exactly what the name of the function is expressing
without the need for extra fields or functions.

canAccess, err := common.EitherAuth(u.accessService, u.accessRelation)()
if err != nil {
    return err
}

To me that reads slightly more directly (you don't have to
look at the function and make sure that it's doing what its
name says) - you're using primitives to calculate the
access criteria for that particular API call in the place
where it's appropriate.

YMMV of course.

https://codereview.appspot.com/12990043/diff/20001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:622: result.Error = common.ServerError(err)
I don't see the point of returning this in result.Error - we already have an
error return (it doesn't matter that for non-bulk calls the error result.Error
field will be ignored).

We might as well use the rpc package as it was designed in the few places that
it's appropriate to do so.

[dimitern, 2013-08-16 07:59:42]

Please take a look.

https://codereview.appspot.com/12990043/diff/20001/state/apiserver/common/int...
File state/apiserver/common/interfaces.go (right):

https://codereview.appspot.com/12990043/diff/20001/state/apiserver/common/int...
state/apiserver/common/interfaces.go:50: func Either(a, b func() (AuthFunc,
error)) func() (AuthFunc, error) {
On 2013/08/15 19:00:37, rog wrote:
> AuthEither, since it's in common?
> Also, it should have a test as it's exported.
> (fairly trivial to test exhaustively though)

Done.

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

https://codereview.appspot.com/12990043/diff/20001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:58: getCanAccessUnitOrService := common.Either(
On 2013/08/15 19:00:37, rog wrote:
> Thanks! I hope you agree that reads quite nicely.
> 
> However I wonder... do you think these justify their place as UniterAPI fields
> now that they're so easy to calculate in place?
> 
> I'd be tempted to use common.Either in place in the methods themselves - it
> expresses exactly what the name of the function is expressing
> without the need for extra fields or functions.
> 
> canAccess, err := common.EitherAuth(u.accessService, u.accessRelation)()
> if err != nil {
>     return err
> }
> 
> To me that reads slightly more directly (you don't have to
> look at the function and make sure that it's doing what its
> name says) - you're using primitives to calculate the
> access criteria for that particular API call in the place
> where it's appropriate.
> 
> YMMV of course.

Done.

https://codereview.appspot.com/12990043/diff/20001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:622: result.Error = common.ServerError(err)
On 2013/08/15 19:00:37, rog wrote:
> I don't see the point of returning this in result.Error - we already have an
> error return (it doesn't matter that for non-bulk calls the error result.Error
> field will be ignored).
> 
> We might as well use the rpc package as it was designed in the few places that
> it's appropriate to do so.

Done.

[rog, 2013-08-16 08:13:30]

LGTM with one trivial below.

Thanks very much!

https://codereview.appspot.com/12990043/diff/26001/state/apiserver/common/com...
File state/apiserver/common/common_test.go (right):

https://codereview.appspot.com/12990043/diff/26001/state/apiserver/common/com...
state/apiserver/common/common_test.go:23: var (
func errorAuth() (common.AuthFunc, error)

etc

no particular need for them to be vars.

[rog, 2013-08-16 08:23:22]

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

https://codereview.appspot.com/12990043/diff/26001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:579: params.Endpoint{
You don't need to specify the type name here - it's implied by the slice
initialiser.

[dimitern, 2013-08-16 08:51:33]

Please take a look.

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

https://codereview.appspot.com/12990043/diff/26001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:579: params.Endpoint{
On 2013/08/16 08:23:23, rog wrote:
> You don't need to specify the type name here - it's implied by the slice
> initialiser.

As discussed, this is changed now to return a single (local) endpoint, rather
than a slice.

[fwereade, 2013-08-16 09:38:07]

LGTM, just a couple of things to ponder. Nothing critical for this CL, I expect
this stuff to evolve a little as we switch relation tags.

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

https://codereview.appspot.com/12990043/diff/37001/state/api/params/internal....
state/api/params/internal.go:85: type Relation struct {
This name seems slightly odd; can we maybe think of something better?

Or, *possibly*, would it make sense to always infer the appropriate unit from
the connected entity? It's probably a bad idea in some way, because I feel a bit
weird about it, but my brain keeps bringing it up.

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/common/com...
File state/apiserver/common/common_test.go (right):

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/common/com...
state/apiserver/common/common_test.go:46: {
}{{
    ...
}, {
    ...
}}

saves both horizontal and vertical space and IMO doesn't lose readability...

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/common/int...
File state/apiserver/common/interfaces.go (right):

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/common/int...
state/apiserver/common/interfaces.go:50: func AuthEither(a, b func() (AuthFunc,
error)) func() (AuthFunc, error) {
I think `func() (AuthFunc, error)` has a name already -- possibly a GetAuthFunc?

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/common/pas...
File state/apiserver/common/password_test.go (left):

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/common/pas...
state/apiserver/common/password_test.go:23: }
gaah, this stuff is crazy -- were we running all these tests twice? I'm
pondering mandating a standard "package_test.go" file that just holds these
bits...

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

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:604: // EnvironUUID returns the UUID for the
current juju environment.
CurrentEnvironUUID

[dimitern, 2013-08-16 10:32:14]

Thanks for the review! Here are my answers - some of the suggestions will be
implemented in the follow-up branch.

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

https://codereview.appspot.com/12990043/diff/37001/state/api/params/internal....
state/api/params/internal.go:85: type Relation struct {
On 2013/08/16 09:38:07, fwereade wrote:
> This name seems slightly odd; can we maybe think of something better?
> 
> Or, *possibly*, would it make sense to always infer the appropriate unit from
> the connected entity? It's probably a bad idea in some way, because I feel a
bit
> weird about it, but my brain keeps bringing it up.

I'd advise against this, because it's a precedent. Why should we do this here
and not in every other call? We always have a connected entity, so if we decide
we're going to assume we're only returning results for the authenticated entity
(unit, machine, etc.) then the whole idea of bulk operations is moot: we'll
always return results only for one entity, and most calls won't even need an
argument.

Otherwise, I'm open to suggestions for a better name.

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/common/com...
File state/apiserver/common/common_test.go (right):

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/common/com...
state/apiserver/common/common_test.go:46: {
On 2013/08/16 09:38:07, fwereade wrote:
> }{{
>     ...
> }, {
>     ...
> }}
> 
> saves both horizontal and vertical space and IMO doesn't lose readability...

Done in the follow-up.

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/common/int...
File state/apiserver/common/interfaces.go (right):

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/common/int...
state/apiserver/common/interfaces.go:50: func AuthEither(a, b func() (AuthFunc,
error)) func() (AuthFunc, error) {
On 2013/08/16 09:38:07, fwereade wrote:
> I think `func() (AuthFunc, error)` has a name already -- possibly a
GetAuthFunc?

Done in the follow-up branch.

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/common/pas...
File state/apiserver/common/password_test.go (left):

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/common/pas...
state/apiserver/common/password_test.go:23: }
On 2013/08/16 09:38:07, fwereade wrote:
> gaah, this stuff is crazy -- were we running all these tests twice? I'm
> pondering mandating a standard "package_test.go" file that just holds these
> bits...

Standardizing this across the code is a big +1.
And no, we weren't running the tests twice - this was just here, and now I moved
it to common_test.go.

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

https://codereview.appspot.com/12990043/diff/37001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:604: // EnvironUUID returns the UUID for the
current juju environment.
On 2013/08/16 09:38:07, fwereade wrote:
> CurrentEnvironUUID

Done in the follow-up branch.