Code review comment for lp:~sidnei/juju-core/lxc-mirror

Please take a look.

https://codereview.appspot.com/12143043/diff/15002/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/15002/container/lxc/lxc_test.go#newcode124
container/lxc/lxc_test.go:124: c.Assert(scripts[len(scripts)-4],
gc.Equals, "start jujud-machine-1-lxc-0")
On 2013/08/05 16:05:34, rog wrote:
> c.Assert(scripts[len(scripts)-4:], gc.DeepEquals, []string{
> "start juju-machine-1-lxc-0",
> "install -m 600 /dev/null '/etc/apt.conf.d/99proxy-extra'",
> etc
> })

> ?

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode32
utils/apt.go:32: var commandOutput = osCommandOutput
On 2013/08/05 16:05:34, rog wrote:

> var commandOutput = exec.Cmd.Output

> (and you can delete the osCommandOutput definition
> and probably move most of the comment onto commandOutput)

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode60
utils/apt.go:60: var (
On 2013/08/05 16:05:34, rog wrote:

> No need to declare these here.

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode70
utils/apt.go:70: if out, err = commandOutput(cmd); err != nil {
On 2013/08/05 16:05:34, rog wrote:

> out, err := commandOutput(cmd)
> if err != nil {
> return "", err
> }
> return strings.TrimSpace(string(out)), nil

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode70
utils/apt.go:70: if out, err = commandOutput(cmd); err != nil {
On 2013/08/05 16:05:34, rog wrote:

> out, err := commandOutput(cmd)
> if err != nil {
> return "", err
> }
> return strings.TrimSpace(string(out)), nil

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode71
utils/apt.go:71: return "", err
On 2013/08/05 16:05:34, rog wrote:
> If the command has failed for some reason other
> than a missing apt-config command, this error
> will be highly opaque (it will contain only
> the apt-config exit code).

> I suggest that you acquire the standard error output
> too, and parcel it up into an error if
> the command fails.

> You could use the same runCommand hook mechanism that AptGetInstall
> uses (and in fact, AptGetInstall suffers from the same issue, but
that's
> probably something to fix in another CL)

Done, fixed in this branch.

https://codereview.appspot.com/12143043/