Code review comment for lp:~frankban/juju-core/bug-1147138-annotations-api

LGTM with the below trivial points addressed.
really good stuff, thanks!

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

https://codereview.appspot.com/7526043/diff/8001/state/api/apiclient.go#newcode146
state/api/apiclient.go:146: func (c *Client) GetAnnotations(id string)
(*Annotations, error) {
i think there's not really a point in having a new type for annotations
here, just to contain a map.
i'd just return the map directly, like the state Annotations method
does.

something like this, perhaps? (note the variable name change,
just to make it obvious what kind of id we're talking about here).

// SetAnnotations returns annotations that have been
// set on the given entity.
func (c *Client) SetAnnotations(entityId string) (map[string]string,
error)

https://codereview.appspot.com/7526043/diff/8001/state/api/apiclient.go#newcode156
state/api/apiclient.go:156: // SetAnnotation stores an annotation about
a given entity.
// SetAnnotation sets the annotation with the given key
// on the given entity to the given value.
// Currently annotations are supported on machines,
// services, units and the environment itself.
func (c *Client) SetAnnotation(entityId, key, value string) error {

?

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

https://codereview.appspot.com/7526043/diff/8001/state/api/params/params.go#newcode76
state/api/params/params.go:76: Id string
s/Id/EntityId/

https://codereview.appspot.com/7526043/diff/8001/state/api/params/params.go#newcode81
state/api/params/params.go:81: Id string
s/Id/EntityId/

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

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#newcode301
state/apiserver/api_test.go:301: return func() {}, nil
the returned function should reset the annotation to its original value
- all these op* functions should leave the state unchanged.

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#newcode641
state/apiserver/api_test.go:641: // Annotations are correctly set.
s/Annotations/Check annotations/ ?

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#newcode642
state/apiserver/api_test.go:642: entity.Refresh()
check error

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#newcode647
state/apiserver/api_test.go:647: // Annotations are correctly returned.
s/Annotations/Check annotations/ ?

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#newcode648
state/apiserver/api_test.go:648: entity.Refresh()
check error

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#newcode652
state/apiserver/api_test.go:652: err = entity.SetAnnotation(key, "")
check error

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/apiserver.go#newcode308
state/apiserver/apiserver.go:308: func (c *srvClient)
GetAnnotations(args params.GetAnnotations) (api.Annotations, error) {
if the api.Annotations type goes away (as i suggest elsewhere) then this
should return params.GetAnnotationsResults.

https://codereview.appspot.com/7526043/