Code review comment for lp://staging/~dimitern/juju-core/040-machine-api-calls-needed-by-machiner

LGTM, some rambling comments, not all of them actionable, so ignore as
appropriate.

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.
I find this name and comment slightly confusing. The call esentially
registers the machine over the api and creates a heartbeat object?

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
This is where I want a type system that lets you describe the set of
valid values in something other than a comment. :)

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
That's a lot of this line in these test helpers... at least func() {} is
short-ish...

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")
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?

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)
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...

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.
Okay, I'm still confused by this, even seeing the other side. The
registration seems a pretty important side-effect that should be
mentioned.

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

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