Code review comment for lp:~axwalk/juju-core/lp1279710-cloudinitoutput-on-error

On 2014/03/17 03:21:31, axw wrote:
> Please take a look.

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

https://codereview.appspot.com/75990043/diff/1/environs/cloudinit/cloudinit.go#newcode409
> environs/cloudinit/cloudinit.go:409: func DumpLogOnError(mcfg
*MachineConfig)
> string {
> On 2014/03/14 12:11:39, rog wrote:
> > This isn't really that deeply connected to MachineConfig,
> > or environs, and it's perhaps reasonable for the callers to know
about
> > the cloudinit file, so perhaps this might be better in
> > utils, say, as:
> >
> > // DumpFileOnErrorScript returns a script that
> > // prints the contents of the given file to
> > // standard error if any commands following it exit with an error.
> > // The returned script is newline terminated.
> > func DumpFileOnErrorScript(filename string) string
> >
> > then the callers can use
utils.DumpFileOnErrorScript(mcfg.CloudInitOutputLog)

> Done, though I'm starting a new package: utils/shell. We can move
ShQuote and
> other shell related things in there over time.

https://codereview.appspot.com/75990043/diff/1/environs/cloudinit/cloudinit.go#newcode418
> environs/cloudinit/cloudinit.go:418: trap "dump_cloudinit_log" EXIT
> On 2014/03/14 12:11:39, rog wrote:
> > i didn't know that EXIT works rather than 0, but as both dash and
bash seem to
> > support it, i guess it's ok.
> >
> > The quotes are unnecessary here BTW.

> Thanks, removed

https://codereview.appspot.com/75990043/diff/1/environs/manual/provisioner.go
> File environs/manual/provisioner.go (right):

https://codereview.appspot.com/75990043/diff/1/environs/manual/provisioner.go#newcode319
> environs/manual/provisioner.go:319: script = fmt.Sprintf("rm -f %s\n",
> utils.ShQuote(mcfg.CloudInitOutputLog)) + script
> On 2014/03/14 12:11:39, rog wrote:
> > rather than doing thing backwards, i'd be tempted to build the
script up in
> > order:
> >
> > var buf bytes.Buffer
> > // Always remove the cloud-init-output.log file first, if it exists.
> > fmt.Fprintf(&buf, "rm -f %s\n", ...)
> > buf.WriteString(cloudinit.DumpLogOnError(mcfg))
> > configScript, err := sshinit.ConfigureScript(cloudcfg)
> > ...
> > buf.WriteString(configScript)
> > return buf.String(), nil

> Done.

LGTM

https://codereview.appspot.com/75990043/