Annotations API functionality

Annotations API functionality

Annotations are now exposed through the API via 'GetAnnotations' and 
'SetAnnotation'.  These allow a user to set an annotation on an Entity (service,
'unit', 'machine', with environment coming in a future branch), and retrieve
all annotations on an entity.  Tests were added around the added functionality.
Note that now the service is an Entity, as suggested by Roger.

https://code.launchpad.net/~frankban/juju-core/bug-1147138-annotations-api/+merge/152037

(do not edit description out of merge proposal)

[frankban, 2013-03-06 19:12:36]

Please take a look.

[frankban, 2013-03-06 19:43:39]

Please take a look.

[dimitern, 2013-03-07 14:28:00]

Overall LGTM, just a couple of comments.

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

https://codereview.appspot.com/7526043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:127: args := params.GetAnnotations{Id: id}
can you skip the keys here? If it's only one {id} will work.

https://codereview.appspot.com/7526043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:138: args := params.SetAnnotation{Id: id, Key: key,
Value: value}
similarly - see above

[frankban, 2013-03-07 15:32:20]

Please take a look.

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

https://codereview.appspot.com/7526043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:127: args := params.GetAnnotations{Id: id}
On 2013/03/07 14:28:01, dimitern wrote:
> can you skip the keys here? If it's only one {id} will work.

Done.

https://codereview.appspot.com/7526043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:138: args := params.SetAnnotation{Id: id, Key: key,
Value: value}
On 2013/03/07 14:28:01, dimitern wrote:
> similarly - see above

Done.

[rog, 2013-03-08 20:52:37]

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#newco...
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#newco...
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#n...
state/api/params/params.go:76: Id string
s/Id/EntityId/

https://codereview.appspot.com/7526043/diff/8001/state/api/params/params.go#n...
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#...
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#...
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#...
state/apiserver/api_test.go:642: entity.Refresh()
check error

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#...
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#...
state/apiserver/api_test.go:648: entity.Refresh()
check error

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#...
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...
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.

[frankban, 2013-03-08 21:37:35]

*** Submitted:

Annotations API functionality

Annotations are now exposed through the API via 'GetAnnotations' and 
'SetAnnotation'.  These allow a user to set an annotation on an Entity (service,
'unit', 'machine', with environment coming in a future branch), and retrieve
all annotations on an entity.  Tests were added around the added functionality.
Note that now the service is an Entity, as suggested by Roger.

R=dimitern, rog
CC=
https://codereview.appspot.com/7526043

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#newco...
state/api/apiclient.go:146: func (c *Client) GetAnnotations(id string)
(*Annotations, error) {
On 2013/03/08 20:52:37, rog wrote:
> 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)

Done.

https://codereview.appspot.com/7526043/diff/8001/state/api/apiclient.go#newco...
state/api/apiclient.go:156: // SetAnnotation stores an annotation about a given
entity.
On 2013/03/08 20:52:37, rog wrote:
> // 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 {
> 
> ?

Done.

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#n...
state/api/params/params.go:76: Id string
On 2013/03/08 20:52:37, rog wrote:
> s/Id/EntityId/

Done.

https://codereview.appspot.com/7526043/diff/8001/state/api/params/params.go#n...
state/api/params/params.go:81: Id    string
On 2013/03/08 20:52:37, rog wrote:
> s/Id/EntityId/

Done.

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#...
state/apiserver/api_test.go:301: return func() {}, nil
On 2013/03/08 20:52:37, rog wrote:
> the returned function should reset the annotation to its original value - all
> these op* functions should leave the state unchanged.

Done.

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:641: // Annotations are correctly set.
On 2013/03/08 20:52:37, rog wrote:
> s/Annotations/Check annotations/ ?

Done.

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:642: entity.Refresh()
On 2013/03/08 20:52:37, rog wrote:
> check error

Done.

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:647: // Annotations are correctly returned.
On 2013/03/08 20:52:37, rog wrote:
> s/Annotations/Check annotations/ ?

Done.

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:648: entity.Refresh()
On 2013/03/08 20:52:37, rog wrote:
> check error

Done.

https://codereview.appspot.com/7526043/diff/8001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:652: err = entity.SetAnnotation(key, "")
On 2013/03/08 20:52:37, rog wrote:
> check error

Done.

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...
state/apiserver/apiserver.go:308: func (c *srvClient) GetAnnotations(args
params.GetAnnotations) (api.Annotations, error) {
On 2013/03/08 20:52:37, rog wrote:
> if the api.Annotations type goes away (as i suggest elsewhere) then this
should
> return params.GetAnnotationsResults.

Done.

[frankban, 2013-03-08 22:05:13]

Hi Dimiter and Roger,
thanks for the reviews.