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

Looks OK to me, although I'm not qualified to
review it - I have no knowledge of apt proxy
settings, and given that we can't have tests that
actually test this for real, I'd be happier if it's
reviewed by someone that does.

Some suggestions below.

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

?

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

var commandOutput = exec.Cmd.Output

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

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode60
utils/apt.go:60: var (

No need to declare these here.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode70
utils/apt.go:70: if out, err = commandOutput(cmd); err != nil {

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

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode71
utils/apt.go:71: return "", err
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)

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