Please take a look.


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.
On 2013/05/21 15:37:20, rog wrote:
> 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.

Done.

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode370
state/api/apiclient.go:370: type Pinger struct {
On 2013/05/21 15:44:01, TheMue wrote:
> +1

Done.

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode375
state/api/apiclient.go:375: func (p *Pinger) Stop() error {
On 2013/05/21 15:37:20, rog wrote:
> ditto

Done.

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.
On 2013/05/21 15:37:20, rog wrote:
> // SetStatus holds the parameters for Machine.SetStatus
> // and Unit.SetStatus.
> ?

Done.

https://codereview.appspot.com/9614043/diff/1/state/api/params/params.go#newcode352
state/api/params/params.go:352: Life        string
On 2013/05/21 15:37:20, rog wrote:
> please change this to be of type Life.

Done.

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.
On 2013/05/21 15:37:20, rog wrote:
> // Machine 0 is allowed because it is an environment manager.
> ?

Done.

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.
On 2013/05/21 15:37:20, rog wrote:
> ditto?

Done.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode287
state/apiserver/api_test.go:287: }
On 2013/05/21 15:37:20, rog wrote:
> you can stop the pinger here (and you should check the error return
too)

Done.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode320
state/apiserver/api_test.go:320: setDefaultStatus(c, m)
On 2013/05/21 15:37:20, rog wrote:
> 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.

As discussed setDefaultStatus must stay (we have to move status from
pending in setupScenario, because we cannot set the status back to
Pending, which is the default), but I added reading and resetting the
original status and info.

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) {
On 2013/05/21 15:37:20, rog wrote:
> as implied above, i don't think this is necessary.

See above.

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#newcode214
state/apiserver/apiserver.go:214: func (r *srvRoot) AllWatcher(id
string) (srvClientAllWatcher, error) {
On 2013/05/21 15:44:01, TheMue wrote:
> Would like to see a doc comment here, just to be consistent.

Me too :)
Ok, I added it, even though it wasn't part of my changes.

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
On 2013/05/21 15:37:20, rog wrote:
> // authOwner returns whether the authenticated user's tag
> // matches the given entity's tag.

Done.

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
On 2013/05/21 15:37:20, rog wrote:
> // authEnvironManager returns whether the authenticated
> // user is a machine with running the ManageEnviron job.

Done.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#newcode268
state/apiserver/apiserver.go:268: ew := w.r.(*state.EntityWatcher)
On 2013/05/21 16:05:14, rog wrote:
> On 2013/05/21 15:44:01, TheMue wrote:
> > Hmm, "ew", "w", "r", not very self-documenting variable names. Using
one char
> > for the type is OK, but the other ones?

> that's perhaps a fair point. it was more obvious when "w" meant
"watcher" both
> times.

> perhaps we could rename srvResource.r to srvResource.resource.

Done.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#newcode268
state/apiserver/apiserver.go:268: ew := w.r.(*state.EntityWatcher)
On 2013/05/21 15:44:01, TheMue wrote:
> Hmm, "ew", "w", "r", not very self-documenting variable names. Using
one char
> for the type is OK, but the other ones?

I don't like it either.. Anyway I changed some names to be more
descriptive.

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