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

On 2013/11/22 06:37:49, 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?

I've added the dots. I think 1 second is fine; the DNS wait is quick
anyway. The difference in rate should also give an indication of the
relative speed of the two phases.

> 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.

Thanks, I will do that. I might just run this by thumper (and fwereade?)
as well, as I know he was interested in it to begin with.

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/