Implement service/unit/machine annotations

Implement service/unit/machine annotations

Annotations were added to services, units, and machines in MongoDB.  
Annotations are stored as key/value pairs and accessed through either 
Annotations or, for a single annotation, Annotation.  
Non-existent annotations are returned as the zero-value for strings (""), 
and annotations can be removed one at a time.  
Annotations will be used by the GUI (for storing x/y coordinates for services),
Landscape (for reboots/security upgrades), and by providers.  
This is a precursor branch for adding annotations to the environment, 
and exposing annotation methods via the API.

https://code.launchpad.net/~frankban/juju-core/annotations/+merge/151792

(do not edit description out of merge proposal)

[frankban, 2013-03-05 15:41:59]

Please take a look.

[dimitern, 2013-03-05 16:10:26]

I think this needs a bit more polishing. Possibly reduce code duplication in
service/machine/unit. Tests feel a bit weird and I think they can be improved.

https://codereview.appspot.com/7483043/diff/1/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7483043/diff/1/state/annotator.go#newcode18
state/annotator.go:18: func (a annotator) SetAnnotation(key, value string) error
{
I think you should change this to take *annotator, since you're changing the
map.

https://codereview.appspot.com/7483043/diff/1/state/annotator.go#newcode44
state/annotator.go:44: func (a annotator) RemoveAnnotation(key string) error {
ditto

https://codereview.appspot.com/7483043/diff/1/state/annotator_test.go
File state/annotator_test.go (right):

https://codereview.appspot.com/7483043/diff/1/state/annotator_test.go#newcode7
state/annotator_test.go:7: type annotator interface {
Why do you need the interface? Can't you use just the annotator struct?

https://codereview.appspot.com/7483043/diff/1/state/annotator_test.go#newcode15
state/annotator_test.go:15: func testSetAnnotation(c *C, entity annotator) {
ISTM you don't need all these separate cases if you follow the usual pattern for
table-based tests (see below). See how the other tests are written.

https://codereview.appspot.com/7483043/diff/1/state/annotator_test.go#newcode69
state/annotator_test.go:69: tests := []struct {
move this outside as var tests ...
Also, you can have fields like:

input map[string]string // what key(s) to set or remove (nil value)
err string // what error to expect (regexp or empty)

Then, you can set/remove keys according to what's in input, check for error (if
expected).

https://codereview.appspot.com/7483043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/7483043/diff/1/state/machine.go#newcode63
state/machine.go:63: ann := annotator{st: st, coll: st.machines.Name, id:
doc.Id}
See the other comment about *a, and changing that you'll need to change this to
ann := &annotator{...}

https://codereview.appspot.com/7483043/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7483043/diff/1/state/service.go#newcode37
state/service.go:37: ann := annotator{st: st, coll: st.services.Name, id:
doc.Name}
see the other comment about &annotator{...}

[frankban, 2013-03-05 19:06:58]

Please take a look.

https://codereview.appspot.com/7483043/diff/1/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7483043/diff/1/state/annotator.go#newcode18
state/annotator.go:18: func (a annotator) SetAnnotation(key, value string) error
{
On 2013/03/05 16:10:26, dimitern wrote:
> I think you should change this to take *annotator, since you're changing the
> map.

Done.

https://codereview.appspot.com/7483043/diff/1/state/annotator.go#newcode44
state/annotator.go:44: func (a annotator) RemoveAnnotation(key string) error {
On 2013/03/05 16:10:26, dimitern wrote:
> ditto

Method discarded as suggested by Roger.

https://codereview.appspot.com/7483043/diff/1/state/annotator_test.go
File state/annotator_test.go (right):

https://codereview.appspot.com/7483043/diff/1/state/annotator_test.go#newcode7
state/annotator_test.go:7: type annotator interface {
On 2013/03/05 16:10:26, dimitern wrote:
> Why do you need the interface? Can't you use just the annotator struct?

annotator is not exported. Moreover, we also need something implementing
Annotations() and Refresh().

https://codereview.appspot.com/7483043/diff/1/state/annotator_test.go#newcode15
state/annotator_test.go:15: func testSetAnnotation(c *C, entity annotator) {
On 2013/03/05 16:10:26, dimitern wrote:
> ISTM you don't need all these separate cases if you follow the usual pattern
for
> table-based tests (see below). See how the other tests are written.

Done.

https://codereview.appspot.com/7483043/diff/1/state/annotator_test.go#newcode69
state/annotator_test.go:69: tests := []struct {
On 2013/03/05 16:10:26, dimitern wrote:
> move this outside as var tests ...
> Also, you can have fields like:
> 
> input map[string]string // what key(s) to set or remove (nil value)
> err string // what error to expect (regexp or empty)
> 
> Then, you can set/remove keys according to what's in input, check for error
(if
> expected).

Done.

https://codereview.appspot.com/7483043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/7483043/diff/1/state/machine.go#newcode63
state/machine.go:63: ann := annotator{st: st, coll: st.machines.Name, id:
doc.Id}
On 2013/03/05 16:10:26, dimitern wrote:
> See the other comment about *a, and changing that you'll need to change this
to
> ann := &annotator{...}

On Roger's suggestion, we maintain the embedded value in the annotator, so it
does not need to be dereferenced.

[dimitern, 2013-03-05 19:09:55]

Much nicer, thank you! LGTM

[rog, 2013-03-05 19:11:13]

LGTM (two tiny trivials below that might be worth changing - we keep variables
local where reasonable)

https://codereview.appspot.com/7483043/diff/1/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7483043/diff/1/state/annotator.go#newcode44
state/annotator.go:44: func (a annotator) RemoveAnnotation(key string) error {
as discussed earlier, i think this should be merged into SetAnnotation
(triggered by setting an empty string)

https://codereview.appspot.com/7483043/diff/1/state/annotator_test.go
File state/annotator_test.go (right):

https://codereview.appspot.com/7483043/diff/1/state/annotator_test.go#newcode7
state/annotator_test.go:7: type annotator interface {
On 2013/03/05 16:10:26, dimitern wrote:
> Why do you need the interface? Can't you use just the annotator struct?

as discussed in person, it lets us test all the different types of annotatable
objects with the same test code.

https://codereview.appspot.com/7483043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/7483043/diff/1/state/machine.go#newcode63
state/machine.go:63: ann := annotator{st: st, coll: st.machines.Name, id:
doc.Id}
On 2013/03/05 16:10:26, dimitern wrote:
> See the other comment about *a, and changing that you'll need to change this
to
> ann := &annotator{...}

even if the method is a pointer, it's fine to embed it by value (like we do with
sync.Mutex for example)

https://codereview.appspot.com/7483043/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7483043/diff/1/state/service.go#newcode37
state/service.go:37: ann := annotator{st: st, coll: st.services.Name, id:
doc.Name}
On 2013/03/05 16:10:26, dimitern wrote:
> see the other comment about &annotator{...}

see other reply :-)

https://codereview.appspot.com/7483043/diff/6001/state/annotator_test.go
File state/annotator_test.go (right):

https://codereview.appspot.com/7483043/diff/6001/state/annotator_test.go#newc...
state/annotator_test.go:66: err = entity.SetAnnotation(key, value)
:=

https://codereview.appspot.com/7483043/diff/6001/state/annotator_test.go#newc...
state/annotator_test.go:70: err = entity.SetAnnotation(key, value)
:=

[frankban, 2013-03-05 19:33:36]

Hi Dimiter and Roger,
thanks for the reviews!

https://codereview.appspot.com/7483043/diff/1/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7483043/diff/1/state/annotator.go#newcode44
state/annotator.go:44: func (a annotator) RemoveAnnotation(key string) error {
On 2013/03/05 19:11:14, rog wrote:
> as discussed earlier, i think this should be merged into SetAnnotation
> (triggered by setting an empty string)

Done.

https://codereview.appspot.com/7483043/diff/6001/state/annotator_test.go
File state/annotator_test.go (right):

https://codereview.appspot.com/7483043/diff/6001/state/annotator_test.go#newc...
state/annotator_test.go:66: err = entity.SetAnnotation(key, value)
On 2013/03/05 19:11:14, rog wrote:
> :=

Done.

https://codereview.appspot.com/7483043/diff/6001/state/annotator_test.go#newc...
state/annotator_test.go:70: err = entity.SetAnnotation(key, value)
On 2013/03/05 19:11:14, rog wrote:
> :=

Done.

[frankban, 2013-03-05 19:39:32]

*** Submitted:

Implement service/unit/machine annotations

Annotations were added to services, units, and machines in MongoDB.  
Annotations are stored as key/value pairs and accessed through either 
Annotations or, for a single annotation, Annotation.  
Non-existent annotations are returned as the zero-value for strings (""), 
and annotations can be removed one at a time.  
Annotations will be used by the GUI (for storing x/y coordinates for services),
Landscape (for reboots/security upgrades), and by providers.  
This is a precursor branch for adding annotations to the environment, 
and exposing annotation methods via the API.

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