On 2013/12/02 14:54:09, 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.

I've added the "Launching instance" message. Leaving the status bit for
now; that can easily be added in afterwards, and is somewhat separate
from this CL.

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

Yes, I intend to kill it soon.

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

I don't know, that wasn't part of my plan. Sounds easy enough.

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

debug-log still uses SSH; it only uses the API for getting the address.
IIANM, TheMue is looking at changing this.
ssh, scp, debug-* all use SSH, as do environs/manual,
environs/sshstorage, and cloudinit/sshinit. I've changed them all to use
utils/ssh.

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

The SSH-bits could easily be pretty easily swapped to use os/exec
Command, so it's all on localhost. The challenging bit will be how to
package configuration in a nice way.

> Oh! And, can we run in-environment storage everywhere soon? This
doesn't handle
> the bootstrap-state case ofc, but it would be great to keep charms and
tools
> inside the environment.

I can still see one major problem, which is that you couldn't then
sync-tools before bootstrapping. Other than that, I think it should be
doable.


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/