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

LGTM with the error code fix and some other minor suggestions.

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#newcode352
state/api/apiclient.go:352: // a HasAssignedUnitsError.
i think you need a new error code for this.
you'll need to add a new code (say CodeHasAssignedUnits) to the error
codes in api/error.go, and add some code to translate from
*state.HasAssignUnitsError to CodeHasAssignedUnits in
apiserver.serverError.

plus an associated test and changing the comment above appropriately.

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode370
state/api/apiclient.go:370: type Pinger struct {
doc comment please.

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode375
state/api/apiclient.go:375: func (p *Pinger) Stop() error {
ditto

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#newcode32
state/api/params/params.go:32: // SetStatus holds the parameters for
making the SetStatus call for a machine or unit.
// SetStatus holds the parameters for Machine.SetStatus
// and Unit.SetStatus.
?

https://codereview.appspot.com/9614043/diff/1/state/api/params/params.go#newcode352
state/api/params/params.go:352: Life string
please change this to be of type Life.

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#newcode77
state/apiserver/api_test.go:77: // 1, because it's managing the
environment.
// Machine 0 is allowed because it is an environment manager.
?

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode90
state/apiserver/api_test.go:90: // Machine 0 needs to set the status of
a machine when provisioning.
ditto?

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode287
state/apiserver/api_test.go:287: }
you can stop the pinger here (and you should check the error return too)

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode320
state/apiserver/api_test.go:320: setDefaultStatus(c, m)
rather than setting a default status here, i think you should find out
the previous status and set it back.
you should also check the error when doing so.

that way you won't need to call setDefaultStatus in setupScenario, which
is then free to add other machines with different statuses if necessary.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode692
state/apiserver/api_test.go:692: func setDefaultStatus(c *C, entity
setStatuser) {
as implied above, i don't think this is necessary.

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#newcode246
state/apiserver/apiserver.go:246: // authOwner returns true only if the
authenticated user's tag
// authOwner returns whether the authenticated user's tag
// matches the given entity's tag.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#newcode253
state/apiserver/apiserver.go:253: // authEnvironManager returns true
only if the authenticated user is
// authEnvironManager returns whether the authenticated
// user is a machine with running the ManageEnviron job.

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