Code review comment for lp:~axwalk/juju-core/synchronous-bootstrap

Revision history for this message
Ian Booth (wallyworld) wrote :

Thanks for dealing with the finishBootstrap issues - I think it's much
better now that it allows the process to complete in it's own time, but
still allows a Ctrl-C. The code looks ok from a reading it perspective,
and you said you've tested it live :-) One final suggestion - after each
"tick" in the for loops while waiting for dns and ssh, should we print a
"." or something so the user knows the system is alive. Every one second
would be too often so perhaps after each 10 1 second timeouts we could
do it?

We do need to test on maas before landing.

LGTM after a MAAS test and final move of those test helpers to a testing
package.

https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configure_test.go
File cloudinit/sshinit/configure_test.go (right):

https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configure_test.go#newcode136
cloudinit/sshinit/configure_test.go:136: assertScriptMatches(c, cfg,
"(.|\n)*apt-get -y update(.|\n)*", false)
To prevent typos causing the test to pass, I'd love to see the regexp
string which is cut and pasted be put into a const or var.

https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configure_test.go#newcode149
cloudinit/sshinit/configure_test.go:149: assertScriptMatches(c, cfg,
"(.|\n)*apt-get -y upgrade(.|\n)*", false)
ditto

https://codereview.appspot.com/30190043/diff/20001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/30190043/diff/20001/provider/common/bootstrap.go#newcode37
provider/common/bootstrap.go:37: // TODO(axw) 2013-11-22 #XXX
a bug number would be great here

https://codereview.appspot.com/30190043/diff/20001/provider/common/bootstrap.go#newcode90
provider/common/bootstrap.go:90: fmt.Fprintln(ctx.Stderr, "Stopping
instance...")
pedant alert - I think we should use case consistently for out messages.
"Stopping...." vs "cleaning up..." etc

https://codereview.appspot.com/30190043/

« Back to merge proposal