Code review comment for lp:~axwalk/juju-core/agent-watch-apihostports

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM with a few queries below, thanks!

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/machine_test.go#newcode851
cmd/jujud/machine_test.go:851: s.State.StartSync()
shouldn't this be BackingState? (i think the watcher is actually in the
API, right?)

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/machine_test.go#newcode852
cmd/jujud/machine_test.go:852: timeout :=
time.After(coretesting.LongWait)
perhaps this might be better phrased as an attempt loop?

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/unit_test.go
File cmd/jujud/unit_test.go (right):

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/unit_test.go#newcode270
cmd/jujud/unit_test.go:270: s.State.StartSync()
shouldn't this be BackingState?

https://codereview.appspot.com/83500043/

« Back to merge proposal