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

I've tested against ec2, will do openstack and azure later. I don't have
access to a MAAS installation, so that may take a little while.

I've not made any changes to the signal handling yet, but I will do so.
I need to think a bit more about how to handle it.

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

https://codereview.appspot.com/30190043/diff/1/cloudinit/sshinit/configure_test.go#oldcode3
cloudinit/sshinit/configure_test.go:3:
On 2013/11/22 01:54:00, wallyworld wrote:
> Is there a test that "apt-get -y upgrade" is only used when asked for?
I can't
> see it if there is. I think this is a pretty important change so
warrants an
> explicit test.

Added a test, also for apt-get update.

https://codereview.appspot.com/30190043/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/30190043/diff/1/cmd/juju/bootstrap.go#newcode126
cmd/juju/bootstrap.go:126: signal.Notify(make(chan os.Signal),
os.Interrupt)
On 2013/11/22 01:54:00, wallyworld wrote:
> Just a thought. One other option is to install a handler which prints
a message
> informing the user that their Ctrl-C cannot terminate the cmd because
cleanup is
> being done.

Yeah, that's not a bad idea. The only catch is, what do we do before
finish bootstrap is entered? That's got me thinking about your concerns
some more: we need to be able to cancel the WaitDNS/ssh phase too, I
think, and that's not currently handled.

I think what I'll have to do is move the signal handling inside
finishBootstrap. This is in the same realm as direct-os.Stdin access, so
it should probably (eventually? now?) go into some kind of OS-context
structure that gets passed to the Environment (or maybe just to
Bootstrap).

https://codereview.appspot.com/30190043/diff/1/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/30190043/diff/1/environs/cloudinit.go#newcode135
environs/cloudinit.go:135: // and setup the SSH keys. The rest we leave
to environs/manual.
On 2013/11/22 01:54:00, wallyworld wrote:
> environs/manual or cloudinit/sshinit?

cloudinit/sshinit; fixed, thanks

https://codereview.appspot.com/30190043/diff/1/environs/cloudinit_test.go
File environs/cloudinit_test.go (right):

https://codereview.appspot.com/30190043/diff/1/environs/cloudinit_test.go#newcode207
environs/cloudinit_test.go:207: // for MAAS. MAAS needs to configure and
thren bounce the
On 2013/11/22 01:54:00, wallyworld wrote:
> typo

Done.

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

https://codereview.appspot.com/30190043/diff/1/provider/common/bootstrap.go#newcode35
provider/common/bootstrap.go:35: defer func() {
On 2013/11/22 01:54:00, wallyworld wrote:
> Could we break this logic out into a separate function outside of
Bootstrap for
> clarity. The func could be

> handleBootstrapError(err error, inst instance.Instance)

Done.

https://codereview.appspot.com/30190043/diff/1/provider/common/bootstrap.go#newcode104
provider/common/bootstrap.go:104: func TestingPatchFinishBootstrap(f
func(instance.Instance, *cloudinit.MachineConfig) error) func() {
On 2013/11/22 01:54:00, wallyworld wrote:
> Does this need to be exported? It's only called from the above.

Unexported. I was going to use it in a test, but it wasn't worthwhile.

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