state/apiserver: Split Machiner into subpackage

state/apiserver: Split Machiner into subpackage

Minor refactoring to put server-side Machiner
facade into its own subpackage as suggested
previously.
As a side-effect errors and interfaces used
had to be moved into another "common" subpackage
to resolve circular imports.

https://code.launchpad.net/~dimitern/juju-core/055-api-machiner-subpackages/+merge/167486

Requires: https://code.launchpad.net/~dimitern/juju-core/054-apiserver-split-suites/+merge/167284

(do not edit description out of merge proposal)

[dimitern, 2013-06-05 09:15:02]

Please take a look.

[fwereade, 2013-06-05 09:34:33]

LGTM, but I'm not sure why we're referencing the packages as "apimachiner" and
"apicommon" when "machiner" and "common" seem perfectly adequate. I'm certainly
not going to block anything on those grounds, though, fixing up the package
structure is a massive win regardless.

[dimitern, 2013-06-05 10:19:57]

On 2013/06/05 09:34:33, fwereade wrote:
> LGTM, but I'm not sure why we're referencing the packages as "apimachiner" and
> "apicommon" when "machiner" and "common" seem perfectly adequate. I'm
certainly
> not going to block anything on those grounds, though, fixing up the package
> structure is a massive win regardless.

Wasn't sure at first, but good point; now I'm changing the names and removing
aliasing.

[dimitern, 2013-06-05 10:24:18]

Please take a look.

[dimitern, 2013-06-05 10:47:24]

Please take a look.

[dimitern, 2013-06-05 10:53:04]

Please take a look.

[jameinel, 2013-06-05 11:26:28]

I like that things are getting split out.

I'm a little curious what *clients* will need to use. If common is just behind
the scenes then it seems fine for the server code.

However, for stuff like GUI will they just import 'common'? If so, it seems like
it would be useful to have 'apicommon' as the name of the package.

[dimitern, 2013-06-05 11:29:25]

On 2013/06/05 11:26:28, jameinel wrote:
> I like that things are getting split out.
> 
> I'm a little curious what *clients* will need to use. If common is just behind
> the scenes then it seems fine for the server code.
> 
> However, for stuff like GUI will they just import 'common'? If so, it seems
like
> it would be useful to have 'apicommon' as the name of the package.

No, this is just for agents. There is a api/common package in the follow up, but
I don't see a problem with that. It's just for resolving circular imports
really.

[rog, 2013-06-05 16:30:42]

given that we've chosen this direction (which, to reiterate, i disagree with),
this is getting there.
this does not lgtm until the below issues are fixed however, and i think we need
new permissions checks for the new machiner entry points.

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/errors.go
File state/apiserver/common/errors.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/errors...
state/apiserver/common/errors.go:61: func ServerErrorToParams(err error)
*params.Error {
i'm not sure that this is necessary.
couldn't we change ServerError so that
it always returns a params.Error ?

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/interf...
File state/apiserver/common/interfaces.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/interf...
state/apiserver/common/interfaces.go:6: // Tagger interface defines a single Tag
method.
this doesn't say anything about what the Tag method means, when it might be
appropriate to implement or when it might be used.

// Tagger is implemented by any entity with a Tag
// method, which should return the tag of the entity
// (for instance a machine might return the tag "machine-1")

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/interf...
state/apiserver/common/interfaces.go:11: // Authorizer interface defines
per-method authorization calls.
ditto here.

what is this type for? how might it be used? what
do its methods do?

https://codereview.appspot.com/10044043/diff/13/state/apiserver/machiner/mach...
File state/apiserver/machiner/machiner.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/machiner/mach...
state/apiserver/machiner/machiner.go:12: // Machiner represents the Machiner API
facade used by the machiner worker.
I'm not sure that the word "facade" adds much here.

// Machiner implements the API used by the machiner worker.

?

https://codereview.appspot.com/10044043/diff/13/state/apiserver/machiner/mach...
File state/apiserver/machiner/machiner_test.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/machiner/mach...
state/apiserver/machiner/machiner_test.go:58: // Create a machiner facades for
machine 1.
s/facades/API/ ?

https://codereview.appspot.com/10044043/diff/13/state/apiserver/machiner/mach...
state/apiserver/machiner/machiner_test.go:86: result, err :=
s.machiner.SetStatus(args)
i'm not convinced about these tests. the original api server tests tested the
full path from API client to server implementation, and these are no longer
doing that.

can't we use the same pattern here (and, incidentally, avoid duplicating most of
the test logic in both client and server packages)?

https://codereview.appspot.com/10044043/diff/13/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/root.go#newco...
state/apiserver/root.go:39: r.machiner = machiner.New(r.srv.state, r)
i wonder if it might not make more sense to create the Machiner
on demand, letting the machiner package itself decide whether
the given client should be allowed access to it or not.

i.e. make machiner.New return (*Machiner, error)

https://codereview.appspot.com/10044043/diff/13/state/apiserver/root.go#newco...
state/apiserver/root.go:92: if err := r.requireAgent(); err != nil {
this doesn't seem right. shouldn't we check that the agent is a machine at this
point?

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

Please take a look.

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/errors.go
File state/apiserver/common/errors.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/errors...
state/apiserver/common/errors.go:61: func ServerErrorToParams(err error)
*params.Error {
On 2013/06/05 16:30:43, rog wrote:
> i'm not sure that this is necessary.
> couldn't we change ServerError so that
> it always returns a params.Error ?

There are several places where we don't need params.Error in the code, that's
why I decided to have a separate helper.

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/interf...
File state/apiserver/common/interfaces.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/interf...
state/apiserver/common/interfaces.go:6: // Tagger interface defines a single Tag
method.
On 2013/06/05 16:30:43, rog wrote:
> this doesn't say anything about what the Tag method means, when it might be
> appropriate to implement or when it might be used.
> 
> // Tagger is implemented by any entity with a Tag
> // method, which should return the tag of the entity
> // (for instance a machine might return the tag "machine-1")

Done.

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/interf...
state/apiserver/common/interfaces.go:11: // Authorizer interface defines
per-method authorization calls.
On 2013/06/05 16:30:43, rog wrote:
> ditto here.
> 
> what is this type for? how might it be used? what
> do its methods do?

Do you have a suggestion for a better doc comment?

https://codereview.appspot.com/10044043/diff/13/state/apiserver/machiner/mach...
File state/apiserver/machiner/machiner.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/machiner/mach...
state/apiserver/machiner/machiner.go:12: // Machiner represents the Machiner API
facade used by the machiner worker.
On 2013/06/05 16:30:43, rog wrote:
> I'm not sure that the word "facade" adds much here.
> 
> // Machiner implements the API used by the machiner worker.
> 
> ?

Done.

https://codereview.appspot.com/10044043/diff/13/state/apiserver/machiner/mach...
File state/apiserver/machiner/machiner_test.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/machiner/mach...
state/apiserver/machiner/machiner_test.go:58: // Create a machiner facades for
machine 1.
On 2013/06/05 16:30:43, rog wrote:
> s/facades/API/ ?

Done.

https://codereview.appspot.com/10044043/diff/13/state/apiserver/machiner/mach...
state/apiserver/machiner/machiner_test.go:86: result, err :=
s.machiner.SetStatus(args)
On 2013/06/05 16:30:43, rog wrote:
> i'm not convinced about these tests. the original api server tests tested the
> full path from API client to server implementation, and these are no longer
> doing that.
> 
> can't we use the same pattern here (and, incidentally, avoid duplicating most
of
> the test logic in both client and server packages)?

The idea of these tests are to test the machiner in isolation. There are
client-side tests in state/api/machiner testing end-to-end.
I don't think we're duplicating code, because these tests are about handling
bulk operations parameters consistently and permissions checks. The client ones
are integration tests about the client interface.

https://codereview.appspot.com/10044043/diff/13/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/root.go#newco...
state/apiserver/root.go:39: r.machiner = machiner.New(r.srv.state, r)
On 2013/06/05 16:30:43, rog wrote:
> i wonder if it might not make more sense to create the Machiner
> on demand, letting the machiner package itself decide whether
> the given client should be allowed access to it or not.
> 
> i.e. make machiner.New return (*Machiner, error)

How can we decide when an error should be returned?

https://codereview.appspot.com/10044043/diff/13/state/apiserver/root.go#newco...
state/apiserver/root.go:92: if err := r.requireAgent(); err != nil {
On 2013/06/05 16:30:43, rog wrote:
> this doesn't seem right. shouldn't we check that the agent is a machine at
this
> point?

AIUI we need a machine tag to check that, hence the auth checks are in the
methods themselves.

[rog, 2013-06-06 11:32:08]

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/errors.go
File state/apiserver/common/errors.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/errors...
state/apiserver/common/errors.go:61: func ServerErrorToParams(err error)
*params.Error {
On 2013/06/06 09:36:01, dimitern wrote:
> On 2013/06/05 16:30:43, rog wrote:
> > i'm not sure that this is necessary.
> > couldn't we change ServerError so that
> > it always returns a params.Error ?
> 
> There are several places where we don't need params.Error in the code, that's
> why I decided to have a separate helper.

if we change ServerError to always return *params.Error,
then this function could just be:

if err == nil {
   return nil
}
return err.(*params.Error)

or we could just get rid of ServerErrorToParams entirely and put that
logic in the code that's dealing with the errors.
AFAICS that would result in hardly any more code
(an additional "continue" and type conversion).

https://codereview.appspot.com/10044043/diff/13/state/apiserver/machiner/mach...
File state/apiserver/machiner/machiner_test.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/machiner/mach...
state/apiserver/machiner/machiner_test.go:86: result, err :=
s.machiner.SetStatus(args)
On 2013/06/06 09:36:01, dimitern wrote:
> On 2013/06/05 16:30:43, rog wrote:
> > i'm not convinced about these tests. the original api server tests tested
the
> > full path from API client to server implementation, and these are no longer
> > doing that.
> > 
> > can't we use the same pattern here (and, incidentally, avoid duplicating
most
> of
> > the test logic in both client and server packages)?
> 
> The idea of these tests are to test the machiner in isolation. There are
> client-side tests in state/api/machiner testing end-to-end.
> I don't think we're duplicating code, because these tests are about handling
> bulk operations parameters consistently and permissions checks. The client
ones
> are integration tests about the client interface.

I'm not sure what we gain particularly from testing the machiner in isolation
that we don't get from testing it end to end in the context that it will
actually be used.

https://codereview.appspot.com/10044043/diff/13/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/root.go#newco...
state/apiserver/root.go:39: r.machiner = machiner.New(r.srv.state, r)
On 2013/06/06 09:36:01, dimitern wrote:
> On 2013/06/05 16:30:43, rog wrote:
> > i wonder if it might not make more sense to create the Machiner
> > on demand, letting the machiner package itself decide whether
> > the given client should be allowed access to it or not.
> > 
> > i.e. make machiner.New return (*Machiner, error)
> 
> How can we decide when an error should be returned?

machiner.New can make the same authorization decisions as srvRoot.Machiner, no?
and return an error if it wants to decline the authorization.

https://codereview.appspot.com/10044043/diff/13/state/apiserver/root.go#newco...
state/apiserver/root.go:92: if err := r.requireAgent(); err != nil {
On 2013/06/06 09:36:01, dimitern wrote:
> On 2013/06/05 16:30:43, rog wrote:
> > this doesn't seem right. shouldn't we check that the agent is a machine at
> this
> > point?
> 
> AIUI we need a machine tag to check that, hence the auth checks are in the
> methods themselves.

this code allows a uniter to get a Machiner object. is that really right?

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

https://codereview.appspot.com/10044043/diff/17001/state/apiserver/common/int...
state/apiserver/common/interfaces.go:13: // Authorizer interface defines
per-method authorization calls.
// Authorizer represents a value that can be asked for
// authorization information on its associated
// authenticated entity.
// It is implemented by an API server to allow
// an API implementation to ask questions about
// the client that is currently connected.

https://codereview.appspot.com/10044043/diff/17001/state/apiserver/common/int...
state/apiserver/common/interfaces.go:15: AuthOwner(entity Tagger) bool
// AuthOwner returns whether the authenticated
// entity is the same as the given entity.

https://codereview.appspot.com/10044043/diff/17001/state/apiserver/common/int...
state/apiserver/common/interfaces.go:16: AuthEnvironManager() bool
// AuthEnvironManager returns whether the authenticated
// entity is a machine running the environment manager
// job.

[dimitern, 2013-06-06 16:17:36]

Please take a look.

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/errors.go
File state/apiserver/common/errors.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/common/errors...
state/apiserver/common/errors.go:61: func ServerErrorToParams(err error)
*params.Error {
On 2013/06/06 11:32:08, rog wrote:
> On 2013/06/06 09:36:01, dimitern wrote:
> > On 2013/06/05 16:30:43, rog wrote:
> > > i'm not sure that this is necessary.
> > > couldn't we change ServerError so that
> > > it always returns a params.Error ?
> > 
> > There are several places where we don't need params.Error in the code,
that's
> > why I decided to have a separate helper.
> 
> if we change ServerError to always return *params.Error,
> then this function could just be:
> 
> if err == nil {
>    return nil
> }
> return err.(*params.Error)
> 
> or we could just get rid of ServerErrorToParams entirely and put that
> logic in the code that's dealing with the errors.
> AFAICS that would result in hardly any more code
> (an additional "continue" and type conversion).

Unified ServerError and ServerErrorToParams as discussed and agreed online.

https://codereview.appspot.com/10044043/diff/13/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/10044043/diff/13/state/apiserver/root.go#newco...
state/apiserver/root.go:39: r.machiner = machiner.New(r.srv.state, r)
On 2013/06/06 11:32:08, rog wrote:
> On 2013/06/06 09:36:01, dimitern wrote:
> > On 2013/06/05 16:30:43, rog wrote:
> > > i wonder if it might not make more sense to create the Machiner
> > > on demand, letting the machiner package itself decide whether
> > > the given client should be allowed access to it or not.
> > > 
> > > i.e. make machiner.New return (*Machiner, error)
> > 
> > How can we decide when an error should be returned?
> 
> machiner.New can make the same authorization decisions as srvRoot.Machiner,
no?
> and return an error if it wants to decline the authorization.

Done.

https://codereview.appspot.com/10044043/diff/13/state/apiserver/root.go#newco...
state/apiserver/root.go:92: if err := r.requireAgent(); err != nil {
On 2013/06/06 11:32:08, rog wrote:
> On 2013/06/06 09:36:01, dimitern wrote:
> > On 2013/06/05 16:30:43, rog wrote:
> > > this doesn't seem right. shouldn't we check that the agent is a machine at
> > this
> > > point?
> > 
> > AIUI we need a machine tag to check that, hence the auth checks are in the
> > methods themselves.
> 
> this code allows a uniter to get a Machiner object. is that really right?

Changed as suggested.

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

https://codereview.appspot.com/10044043/diff/17001/state/apiserver/common/int...
state/apiserver/common/interfaces.go:13: // Authorizer interface defines
per-method authorization calls.
On 2013/06/06 11:32:08, rog wrote:
> // Authorizer represents a value that can be asked for
> // authorization information on its associated
> // authenticated entity.
> // It is implemented by an API server to allow
> // an API implementation to ask questions about
> // the client that is currently connected.

Done.

https://codereview.appspot.com/10044043/diff/17001/state/apiserver/common/int...
state/apiserver/common/interfaces.go:15: AuthOwner(entity Tagger) bool
On 2013/06/06 11:32:08, rog wrote:
> // AuthOwner returns whether the authenticated
> // entity is the same as the given entity.

Done.

https://codereview.appspot.com/10044043/diff/17001/state/apiserver/common/int...
state/apiserver/common/interfaces.go:16: AuthEnvironManager() bool
On 2013/06/06 11:32:08, rog wrote:
> // AuthEnvironManager returns whether the authenticated
> // entity is a machine running the environment manager
> // job.

Done.

[rog, 2013-06-06 16:33:31]

LGTM with the below issues fixed.

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/apiserver....
state/apiserver/apiserver.go:102: return *err
s/*err/err/

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/common/err...
File state/apiserver/common/errors.go (right):

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/common/err...
state/apiserver/common/errors.go:37: func ServerError(err error) *params.Error {
// ServerError returns an error suitable
// for returning to an API client, with
// an error code suitable for various kinds
// of errors generated in packages outside
// the API.

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

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/common/int...
state/apiserver/common/interfaces.go:20: RequireMachiner() error
to fit in with the other methods, i'd be tempted to do this instead:

// AuthMachiner returns whether the authenticated entity
// is a machine agent.
AuthMachiner() bool

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/errors_tes...
File state/apiserver/errors_test.go (right):

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/errors_tes...
state/apiserver/errors_test.go:75: c.Assert(err1, NotNil)
hmm, we should probably have a nil error test too. sorry i didn't notice/do that
before!

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/errors_tes...
state/apiserver/errors_test.go:80: c.Assert(err1.Error(), Equals, t.err.Error())
we asserted that before.

i actually think that we can lose the if statement and do:
c.Assert(err1.Message, Equals, t.err.Error())
c.Assert(err1.Code, Equals, t.code)

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/root.go#ne...
state/apiserver/root.go:61: // us making the check in every single request
method.
if there's a place for this comment, it's on the Machiner method (or perhaps on
the root object).

i think this can just be:

// AuthMachiner returns whether the current client
// is a machine agent.
func (r *srvRoot) AuthMachiner() bool {
}

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/root.go#ne...
state/apiserver/root.go:86: 
d

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/root.go#ne...
state/apiserver/root.go:110: if id != "" {
this check should come before the New call.

then you can just return machiner.New(r.srv.state, r),
saving a few lines.

[fwereade, 2013-06-06 16:52:15]

just one note to fix please

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/root.go#ne...
state/apiserver/root.go:61: // us making the check in every single request
method.
On 2013/06/06 16:33:31, rog wrote:
> if there's a place for this comment, it's on the Machiner method (or perhaps
on
> the root object).
> 
> i think this can just be:
> 
> // AuthMachiner returns whether the current client
> // is a machine agent.
> func (r *srvRoot) AuthMachiner() bool {
> }

AuthMachineAgent, please. Agents are not their tasks, and vice versa.

[dimitern, 2013-06-06 17:10:52]

*** Submitted:

state/apiserver: Split Machiner into subpackage

Minor refactoring to put server-side Machiner
facade into its own subpackage as suggested
previously.
As a side-effect errors and interfaces used
had to be moved into another "common" subpackage
to resolve circular imports.

R=fwereade, jameinel, rog
CC=
https://codereview.appspot.com/10044043

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/apiserver....
state/apiserver/apiserver.go:102: return *err
On 2013/06/06 16:33:31, rog wrote:
> s/*err/err/

Done.

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/common/err...
File state/apiserver/common/errors.go (right):

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/common/err...
state/apiserver/common/errors.go:37: func ServerError(err error) *params.Error {
On 2013/06/06 16:33:31, rog wrote:
> // ServerError returns an error suitable
> // for returning to an API client, with
> // an error code suitable for various kinds
> // of errors generated in packages outside
> // the API.

Done.

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

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/common/int...
state/apiserver/common/interfaces.go:20: RequireMachiner() error
On 2013/06/06 16:33:31, rog wrote:
> to fit in with the other methods, i'd be tempted to do this instead:
> 
> // AuthMachiner returns whether the authenticated entity
> // is a machine agent.
> AuthMachiner() bool

Done, but changed it to AuthMachineAgent as fwereade correctly suggested.

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/errors_tes...
File state/apiserver/errors_test.go (right):

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/errors_tes...
state/apiserver/errors_test.go:75: c.Assert(err1, NotNil)
On 2013/06/06 16:33:31, rog wrote:
> hmm, we should probably have a nil error test too. sorry i didn't notice/do
that
> before!

Done.

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/errors_tes...
state/apiserver/errors_test.go:80: c.Assert(err1.Error(), Equals, t.err.Error())
On 2013/06/06 16:33:31, rog wrote:
> we asserted that before.
> 
> i actually think that we can lose the if statement and do:
> c.Assert(err1.Message, Equals, t.err.Error())
> c.Assert(err1.Code, Equals, t.code)

Done.

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/root.go#ne...
state/apiserver/root.go:61: // us making the check in every single request
method.
On 2013/06/06 16:33:31, rog wrote:
> if there's a place for this comment, it's on the Machiner method (or perhaps
on
> the root object).
> 
> i think this can just be:
> 
> // AuthMachiner returns whether the current client
> // is a machine agent.
> func (r *srvRoot) AuthMachiner() bool {
> }

Done.

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/root.go#ne...
state/apiserver/root.go:61: // us making the check in every single request
method.
On 2013/06/06 16:52:16, fwereade wrote:
> On 2013/06/06 16:33:31, rog wrote:
> > if there's a place for this comment, it's on the Machiner method (or perhaps
> on
> > the root object).
> > 
> > i think this can just be:
> > 
> > // AuthMachiner returns whether the current client
> > // is a machine agent.
> > func (r *srvRoot) AuthMachiner() bool {
> > }
> 
> AuthMachineAgent, please. Agents are not their tasks, and vice versa.

Done.

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/root.go#ne...
state/apiserver/root.go:86: 
On 2013/06/06 16:33:31, rog wrote:
> d

Done.

https://codereview.appspot.com/10044043/diff/30001/state/apiserver/root.go#ne...
state/apiserver/root.go:110: if id != "" {
On 2013/06/06 16:33:31, rog wrote:
> this check should come before the New call.
> 
> then you can just return machiner.New(r.srv.state, r),
> saving a few lines.

Done.