Code review comment for lp://staging/~aramh/juju-core/121-state-machiner-watchers-machine-principals7

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

That's great. LGTM assuming the following:

https://codereview.appspot.com/6814108/diff/8001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/6814108/diff/8001/state/machine_test.go#newcode325
state/machine_test.go:325: "check initial empty event",
Thanks for the summaries.

https://codereview.appspot.com/6814108/diff/8001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6814108/diff/8001/state/unit.go#newcode417
state/unit.go:417: return &notAssignedError{fmt.Sprintf(format+" is not
assigned to a machine", args...)}
Pre-req has a missing test? It should have a test actually expecting
the NotAssignedError suggested there.

https://codereview.appspot.com/6814108/diff/8001/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6814108/diff/8001/worker/machiner/machiner.go#newcode82
worker/machiner/machiner.go:82: assigned = machineId == m.machine.Id()
New logic was added here verifying that it is indeed assigned to the
machine we think it is. This is awesome, but deserves proper testing.

https://codereview.appspot.com/6814108/

« Back to merge proposal