Thanks gz, here are some answers to the best of my understanding.


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

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode335
state/api/apiclient.go:335: // returns the started pinger.
On 2013/05/21 16:19:41, gz wrote:
> I find this name and comment slightly confusing. The call esentially
registers
> the machine over the api and creates a heartbeat object?

No, there's no registering involved (the machine is already in state and
we just access it through the API). The heartbeat part is essentially
right - or in state parlance a  presence pinger.

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

https://codereview.appspot.com/9614043/diff/1/state/api/params/params.go#newcode40
state/api/params/params.go:40: type Life string
On 2013/05/21 16:19:41, gz wrote:
> This is where I want a type system that lets you describe the set of
valid
> values in something other than a comment. :)

We already have constants for all these values in state/life.go, but
when marshalled through JSON using we rather use strings, rather than
having to define those constants again in JS client-side.

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

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode317
state/apiserver/api_test.go:317: return func() {}, err
On 2013/05/21 16:19:41, gz wrote:
> That's a lot of this line in these test helpers... at least func() {}
is
> short-ish...

I agree it could be improved in general with some helpers.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode1026
state/apiserver/api_test.go:1026: c.Assert(string(life), Equals,
"alive")
On 2013/05/21 16:19:41, gz wrote:
> Wouldn't asserting (life, Equals, params.Life("alive")) make more
sense? Or does
> that mess with test readability more than it helps with type
verification?

It probably due to my profound laziness when typing :) But I agree the
second form is probably better. If you insist I'll change it, otherwise
let's live with it.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode1053
state/apiserver/api_test.go:1053: c.Assert(life, Equals, state.Alive)
On 2013/05/21 16:19:41, gz wrote:
> Hm, and this one asserts against a variable, not a string... which is
because
> it's a different type, an int enum representing the same values...

Yeah, see the comment above about JSON and marshalling. Also it's a bit
a matter of preference.

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

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#newcode528
state/apiserver/apiserver.go:528: // SetAgentAlive signals that the
agent for machine m is alive.
On 2013/05/21 16:19:41, gz wrote:
> Okay, I'm still confused by this, even seeing the other side. The
registration
> seems a pretty important side-effect that should be mentioned.

The registration here is only relevant to the internals of the API
server - it does not affect how it's used externally. The registration
is necessary to keep track of all per-client connection resources, and
has nothing to do with the machine itself.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#newcode570
state/apiserver/apiserver.go:570: if !m.root.authOwner(m.m) &&
!m.root.authEnvironManager() {
On 2013/05/21 15:44:01, TheMue wrote:
> Elegant.

Cheers :)

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#newcode737
state/apiserver/apiserver.go:737: rs.rs = make(map[string]*srvResource)
On 2013/05/21 16:19:41, gz wrote:
> Assuming the compiler/tests will catch any of the mechanical rename
issues,and
> that somebody understands the variables.

I changed them a bit - see the next patch set, should be better now.

https://codereview.appspot.com/9614043/