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

Revision history for this message
William Reade (fwereade) wrote :

This is *great*. Thanks very much. It would be really nice to have an
initial line saying "Launching instance", followed by the actual
instance id that got started -- and maybe to finish by running juju
status -- but those are just details.

So LGTM modulo trivials below, assuming jam is happy; and a few followup
questions:

1) Can we drop the secrets dance soon? (I guess we still need it for the
first connect to 1.16 via state :/... but maybe we don't need it for the
API)

2) How close are we to writing the state server address directly into
the .jenv, so we get fast API connections from the word go? I'd be
really keen to see that followup soon :).

3) Which SSH commands do we still use? I think it's just debug-log that
goes over the API now, so ssh, scp, and debug-hooks are all still SSHy
AIUI -- can we consolidate those bits now? Any I didn't think of?

4) How hard will it be to extract the manual-provisioning code such that
it's possible to run a "juju register" command on an instance that you
want to provision and already have direct access to? I think we touched
on this a while back, but I'm not sure we came to any conclusion.

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

https://codereview.appspot.com/30190043/diff/60001/cloudinit/sshinit/configure_test.go#newcode36
cloudinit/sshinit/configure_test.go:36:
environs.RegisterProvider("sshinit", &testProvider{})
Can we stick an explicit "test" into the name somewhere please?

https://codereview.appspot.com/30190043/diff/60001/environs/bootstrap/state_test.go
File environs/bootstrap/state_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/environs/bootstrap/state_test.go#newcode59
environs/bootstrap/state_test.go:59: c.Assert(err, gc.IsNil) // doesn't
exist, juju don't carea
s/carea/care/

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

https://codereview.appspot.com/30190043/diff/60001/provider/common/bootstrap.go#newcode40
provider/common/bootstrap.go:40: // Std{in,out,err}, and interrupt
signal handling.
I'd like this to be an imminent followup, please :).

https://codereview.appspot.com/30190043/diff/60001/provider/common/bootstrap.go#newcode61
provider/common/bootstrap.go:61: inst, hw, err = env.StartInstance(cons,
selectedTools, machineConfig)
Wouldn't this still work with :=, even given inst? Maybe I'm still
sluggish after lunch...

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

https://codereview.appspot.com/30190043/diff/60001/provider/common/testing/bootstrap.go#newcode17
provider/common/testing/bootstrap.go:17: f :=
func(*common.BootstrapContext, instance.Instance,
*cloudinit.MachineConfig) error {
Let's log something in here so that there's some way of diagnosing what
happened if Disable gets called in a surprising situation.

https://codereview.appspot.com/30190043/diff/60001/provider/maas/maas_test.go
File provider/maas/maas_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/provider/maas/maas_test.go#newcode14
provider/maas/maas_test.go:14: commontesting
"launchpad.net/juju-core/provider/common/testing"
Oh, a thought. Might DisableFinishBootstrap be happier in
environs/testing?

https://codereview.appspot.com/30190043/diff/60001/utils/ssh/ssh.go
File utils/ssh/ssh.go (left):

https://codereview.appspot.com/30190043/diff/60001/utils/ssh/ssh.go#oldcode15
utils/ssh/ssh.go:15: // Move this to a common package for use in
cmd/juju, and others.
Can the SSHy bits in cmd/juju use this now?

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

« Back to merge proposal