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.
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 sshinit/ configure_ test.go (right):
File cloudinit/
https:/ /codereview. appspot. com/30190043/ diff/20001/ cloudinit/ sshinit/ configure_ test.go# newcode136 sshinit/ configure_ test.go: 136: assertScriptMat ches(c, cfg,
cloudinit/
"(.|\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 sshinit/ configure_ test.go: 149: assertScriptMat ches(c, cfg,
cloudinit/
"(.|\n)*apt-get -y upgrade(.|\n)*", false)
ditto
https:/ /codereview. appspot. com/30190043/ diff/20001/ provider/ common/ bootstrap. go common/ bootstrap. go (right):
File provider/
https:/ /codereview. appspot. com/30190043/ diff/20001/ provider/ common/ bootstrap. go#newcode37 common/ bootstrap. go:37: // TODO(axw) 2013-11-22 #XXX
provider/
a bug number would be great here
https:/ /codereview. appspot. com/30190043/ diff/20001/ provider/ common/ bootstrap. go#newcode90 common/ bootstrap. go:90: fmt.Fprintln( ctx.Stderr, "Stopping
provider/
instance...")
pedant alert - I think we should use case consistently for out messages.
"Stopping...." vs "cleaning up..." etc
https:/ /codereview. appspot. com/30190043/