state: implement MachineInfoWatcher

state: implement MachineInfoWatcher



https://code.launchpad.net/~rogpeppe/juju-core/machine-info-watcher/+merge/117023

(do not edit description out of merge proposal)

[rog, 2012-07-27 09:33:29]

Please take a look.

[niemeyer, 2012-07-27 13:07:17]

https://codereview.appspot.com/6447054/diff/2001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6447054/diff/2001/state/machine.go#newcode109
state/machine.go:109: func (m *Machine) WatchInfo() *MachineInfoWatcher {
s/WatchInfo/Watch/
s/MachineInfoWatcher/MachineWatcher/

https://codereview.appspot.com/6447054/diff/2001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6447054/diff/2001/state/watcher.go#newcode473
state/watcher.go:473: func (w *MachineInfoWatcher) Changes() <-chan *MachineInfo
{
s/*MachineInfo/*Machine/

[rog, 2012-07-27 15:53:50]

Please take a look.

[niemeyer, 2012-07-30 15:52:59]

Almost there.

https://codereview.appspot.com/6447054/diff/19/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6447054/diff/19/state/machine.go#newcode80
state/machine.go:80: if _, ok := err.(*NotFoundError); ok || (err == nil &&
instanceId == "") {
If we're going with a generic NotFoundError (which sounds good rather than tons
of per-method types), let's drop this special case and have it returning the
same error too. Will need to fix it elsewhere.

https://codereview.appspot.com/6447054/diff/19/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/6447054/diff/19/state/machine_test.go#newcode289
state/machine_test.go:289: proposedVersion version.Version
Should check a non-version related field too (instance id, for example).

https://codereview.appspot.com/6447054/diff/19/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6447054/diff/19/state/watcher.go#newcode435
state/watcher.go:435: // Currently, alterations to AgentVersion and
ProposedAgentVersion
Why is that specific to versioning? Are we not monitoring the node itself, and
thus any changes to the machine metadata? This shouldn't be special casing every
field.

[rog, 2012-07-30 16:48:49]

https://codereview.appspot.com/6447054/diff/19/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6447054/diff/19/state/watcher.go#newcode435
state/watcher.go:435: // Currently, alterations to AgentVersion and
ProposedAgentVersion
On 2012/07/30 15:52:59, niemeyer wrote:
> Why is that specific to versioning? Are we not monitoring the node itself, and
> thus any changes to the machine metadata? This shouldn't be special casing
every
> field.

There are other aspects of the machine that this does not monitor, such as the
units assigned to a machine, or the agent's liveness status. Funnily enough, I
hadn't realised that monitoring the machine's instance id would work with this
watcher...

I guess my point of view is that it's not obvious to a naive user of the API
which aspects of the Machine are "settings" (i.e. stored in the machine's
confignode) and which are not.

That said, I'll delete the specific field mentions if you like.

[rog, 2012-07-30 17:07:51]

Please take a look.

https://codereview.appspot.com/6447054/diff/19/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6447054/diff/19/state/machine.go#newcode80
state/machine.go:80: if _, ok := err.(*NotFoundError); ok || (err == nil &&
instanceId == "") {
On 2012/07/30 15:52:59, niemeyer wrote:
> If we're going with a generic NotFoundError (which sounds good rather than
tons
> of per-method types), let's drop this special case and have it returning the
> same error too. Will need to fix it elsewhere.

Done. (though there may be more types yet to find).

https://codereview.appspot.com/6447054/diff/19/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/6447054/diff/19/state/machine_test.go#newcode289
state/machine_test.go:289: proposedVersion version.Version
On 2012/07/30 15:52:59, niemeyer wrote:
> Should check a non-version related field too (instance id, for example).

Done.

[niemeyer, 2012-07-30 17:15:14]

LGTM, except for the minor typo:

https://codereview.appspot.com/6447054/diff/36/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6447054/diff/36/state/machine.go#newcode67
state/machine.go:67: // If the id is not set, or its value is "" an error of
type NotFoundError
The s/and/an/ change here is not sound.

[rog, 2012-07-30 17:31:41]

*** Submitted:

state: implement MachineInfoWatcher

R=niemeyer
CC=
https://codereview.appspot.com/6447054