Code review comment for lp:~axwalk/juju-core/lp1296475-block-sigint-bootstrap

Please take a look.

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

https://codereview.appspot.com/76160046/diff/20001/cmd/juju/bootstrap.go#newcode112
cmd/juju/bootstrap.go:112: fmt.Fprintln(ctx.GetStderr(), "Interrupt
signalled: waiting for bootstrap to exit")
On 2014/03/26 02:41:47, thumper wrote:
> You could use the new awesome:
> ctx.Infof("Interrupt signalled: waiting for bootstrap to exit")

Done.

https://codereview.appspot.com/76160046/diff/20001/cmd/juju/common.go
File cmd/juju/common.go (right):

https://codereview.appspot.com/76160046/diff/20001/cmd/juju/common.go#newcode44
cmd/juju/common.go:44: fmt.Fprintf(ctx.GetStderr(), "%s failed,
destroying environment\n", action)
On 2014/03/26 02:41:47, thumper wrote:
> ctx.Infof("%s failed, destroying environment", action)

> A new line is added to the end if missing.

Done.

https://codereview.appspot.com/76160046/diff/20001/environs/bootstrap/interruptiblestorage.go
File environs/bootstrap/interruptiblestorage.go (right):

https://codereview.appspot.com/76160046/diff/20001/environs/bootstrap/interruptiblestorage.go#newcode48
environs/bootstrap/interruptiblestorage.go:48: n, err = r.Reader.Read(p)
On 2014/03/26 02:41:47, thumper wrote:
> Is it expected that this will continue until finished even if
interrupted?

> Is there a race condition here between the return 0, interruptedError
below and
> this assignment?

Yes, good call. Updated to use locals rather than named results.

https://codereview.appspot.com/76160046/diff/20001/environs/bootstrap/synctools.go
File environs/bootstrap/synctools.go (right):

https://codereview.appspot.com/76160046/diff/20001/environs/bootstrap/synctools.go#newcode35
environs/bootstrap/synctools.go:35: logger.Infof("checking that upload
is possible")
On 2014/03/26 02:41:47, thumper wrote:
> we could change this now to use the Verbosef method (if we add it to
the
> BootstrapContext interface)

I don't think this particular one should be logged to stderr, but I have
added Infof/Verbosef to BootstrapContext and changed the other ones
over.

https://codereview.appspot.com/76160046/diff/20001/environs/bootstrap/synctools.go#newcode56
environs/bootstrap/synctools.go:56: fmt.Fprintln(ctx.GetStderr(),
"cancelling tools upload")
On 2014/03/26 02:41:47, thumper wrote:
> we should put Infof and Verbosef onto the bootstrap context I think.

Done.

https://codereview.appspot.com/76160046/