Expose some more data from replica sets

Expose some more data from replica sets

Exposing the full Replica Set Config object (rather than just the members).  Also exposing the full replica set status object.  Added some tests to make sure status was working (some of the tests were commented out and there was no actual test for status).  Finally, added the Tags value on replica set member info.

https://code.launchpad.net/~natefinch/juju-core/029-mongo-tag/+merge/200926

(do not edit description out of merge proposal)

[natefinch, 2014-01-08 22:14:38]

Please take a look.

[rog, 2014-01-09 13:23:05]

LGTM modulo the below, thanks! The suggested new MemberStatus.Id field will need
a test too I guess.

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset.go
File replicaset/replicaset.go (right):

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset.go#newco...
replicaset/replicaset.go:24: cfg := Config{Name: name, Version: 1, Members:
[]Member{Member{Id: 1, Address: address}}}
This is getting quite a mouthful.
Could we split it out across lines please?

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset.go#newco...
replicaset/replicaset.go:246: Members []MemberStatus `bson:"members"`
In my own code, I've been calling this "Statuses", if you feel like following
that convention.

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset.go#newco...
replicaset/replicaset.go:252: // Address holds address of the member that the
status is describing.
Could we add the _id field in here too please?

	// Id holds the replica set id of the member that the status is describing.
	Id int `bson:"_id"`

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go
File replicaset/replicaset_test.go (right):

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go#...
replicaset/replicaset_test.go:133: expectedStatus[0].Uptime =
status.Members[0].Uptime
Perhaps we should at least check that these fields are at least >0 ?

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go#...
replicaset/replicaset_test.go:153: tags := map[string]string{"key1": "val1"}
It might be better to give a different tag to each member just so we're sure
that there's no mixing up anywhere.

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go#...
replicaset/replicaset_test.go:262: expected := Status{
&Status?

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go#...
replicaset/replicaset_test.go:265: MemberStatus{
You can lose the MemberStatus identifier from each element here (and a level of
indentation if you put the first two braces on the same line)

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go#...
replicaset/replicaset_test.go:289: deadline := time.Now().Add(time.Second * 60)
Why not use utils.Attempt here?

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go#...
replicaset/replicaset_test.go:322: c.Check(*res, gc.DeepEquals, expected)
if we initialise expected as a pointer, this can be:

   c.Check(res, gc.DeepEquals, expected)

which doesn't start me looking for pointer-vs-value mismatches around the place
:-)

[natefinch, 2014-01-09 18:02:21]

Made a bunch of changes, thanks for reviewing.

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset.go
File replicaset/replicaset.go (right):

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset.go#newco...
replicaset/replicaset.go:24: cfg := Config{Name: name, Version: 1, Members:
[]Member{Member{Id: 1, Address: address}}}
On 2014/01/09 13:23:05, rog wrote:
> This is getting quite a mouthful.
> Could we split it out across lines please?

Done.

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset.go#newco...
replicaset/replicaset.go:246: Members []MemberStatus `bson:"members"`
On 2014/01/09 13:23:05, rog wrote:
> In my own code, I've been calling this "Statuses", if you feel like following
> that convention.

I'm actually not a fan of calling it statuses... because right now it's
replicaset.Status.Members, which makes sense...  It's the status of the set...
Members could be MemberStatuses... but that seems overly verbose.

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset.go#newco...
replicaset/replicaset.go:252: // Address holds address of the member that the
status is describing.
On 2014/01/09 13:23:05, rog wrote:
> Could we add the _id field in here too please?
> 
> 	// Id holds the replica set id of the member that the status is describing.
> 	Id int `bson:"_id"`

Yep.... weird that I don't see it in the docs (which is why it wasn't just there
in the first place)

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go
File replicaset/replicaset_test.go (right):

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go#...
replicaset/replicaset_test.go:133: expectedStatus[0].Uptime =
status.Members[0].Uptime
On 2014/01/09 13:23:05, rog wrote:
> Perhaps we should at least check that these fields are at least >0 ?

I do that in the TestCurrentStatus test below... really I don't even need the
current status check here with the full test below, so I'll remove it.

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go#...
replicaset/replicaset_test.go:153: tags := map[string]string{"key1": "val1"}
On 2014/01/09 13:23:05, rog wrote:
> It might be better to give a different tag to each member just so we're sure
> that there's no mixing up anywhere.

Oh yeah, thanks for catching that.  I meant to change this to give them unique
strings but forgot to go back and do it.

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go#...
replicaset/replicaset_test.go:262: expected := Status{
On 2014/01/09 13:23:05, rog wrote:
> &Status?

Yep.

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go#...
replicaset/replicaset_test.go:265: MemberStatus{
On 2014/01/09 13:23:05, rog wrote:
> You can lose the MemberStatus identifier from each element here (and a level
of
> indentation if you put the first two braces on the same line)

Ahh yeah... I always forget what parts require the type and what parts don't, so
I end up overcompensating.

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go#...
replicaset/replicaset_test.go:289: deadline := time.Now().Add(time.Second * 60)
On 2014/01/09 13:23:05, rog wrote:
> Why not use utils.Attempt here?

Didn't realize it existed. :)  I'll use it.

https://codereview.appspot.com/49290043/diff/1/replicaset/replicaset_test.go#...
replicaset/replicaset_test.go:322: c.Check(*res, gc.DeepEquals, expected)
On 2014/01/09 13:23:05, rog wrote:
> if we initialise expected as a pointer, this can be:
> 
>    c.Check(res, gc.DeepEquals, expected)
> 
> which doesn't start me looking for pointer-vs-value mismatches around the
place
> :-)

Absolutely.