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

Revision history for this message
Tim Penhey (thumper) wrote :

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")
You could use the new awesome:
   ctx.Infof("Interrupt signalled: waiting for bootstrap to exit")

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)
ctx.Infof("%s failed, destroying environment", action)

A new line is added to the end if missing.

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

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")
we could change this now to use the Verbosef method (if we add it to the
BootstrapContext interface)

https://codereview.appspot.com/76160046/diff/20001/environs/bootstrap/synctools.go#newcode56
environs/bootstrap/synctools.go:56: fmt.Fprintln(ctx.GetStderr(),
"cancelling tools upload")
we should put Infof and Verbosef onto the bootstrap context I think.

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

« Back to merge proposal