Fetch apt proxy config via host apt-config

Fetch apt proxy config via host apt-config

Since we're passing custom userdata to the ubuntu-cloud lxc template, it ends
up not setting the apt_mirror to the MIRROR setting. Read apt proxy
configuration from the container's host instead via the apt-config command and
inject that into the container via apt_proxy cloud init setting for the http
proxy plus a runcmd that writes /etc/apt/apt.conf.d/99proxy-extra with the
same config as the host for the remaing settings not supported explicitly by
cloud-init at the moment.

https://code.launchpad.net/~sidnei/juju-core/lxc-mirror/+merge/177737

(do not edit description out of merge proposal)

[sidnei.da.silva, 2013-08-05 09:22:32]

Please take a look.

[sidnei.da.silva, 2013-08-05 09:26:33]

Please take a look.

[dimitern, 2013-08-05 10:46:08]

LGTM, although I think I reviewed part of that as a previous CL (AddFile,
AddScripts, etc.) did that one get merged here?

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

https://codereview.appspot.com/12143043/diff/7001/container/lxc/lxc_test.go#n...
container/lxc/lxc_test.go:46: []byte(aptConfig), 0755)

Please put 0755 on a separate line, like the other args.

[sidnei.da.silva, 2013-08-05 11:05:46]

Please take a look.

[sidnei.da.silva, 2013-08-05 11:48:58]

Please take a look.

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

https://codereview.appspot.com/12143043/diff/7001/container/lxc/lxc_test.go#n...
container/lxc/lxc_test.go:46: []byte(aptConfig), 0755)
On 2013/08/05 10:46:08, dimitern wrote:
> 
> Please put 0755 on a separate line, like the other args.

Done.

[sidnei.da.silva, 2013-08-05 11:55:41]

Please take a look.

[rog, 2013-08-05 16:05:34]

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#...
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)

[sidnei.da.silva, 2013-08-05 17:19:06]

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#...
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.

[rog, 2013-08-05 17:35:38]

LGTM with a couple of trivials.
Thanks a lot.

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

https://codereview.appspot.com/12143043/diff/22001/container/lxc/lxc_test.go#...
container/lxc/lxc_test.go:128: "ifconfig"})

We'd usually put the }) on a new line here, so each string gets its own line.

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

https://codereview.appspot.com/12143043/diff/22001/utils/apt.go#newcode61
utils/apt.go:61: "Acquire::ftp::Proxy"}

newline before } please

[sidnei.da.silva, 2013-08-05 17:47:05]

Please take a look.

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

https://codereview.appspot.com/12143043/diff/22001/container/lxc/lxc_test.go#...
container/lxc/lxc_test.go:128: "ifconfig"})
On 2013/08/05 17:35:39, rog wrote:
> 
> We'd usually put the }) on a new line here, so each string gets its own line.

Done.

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

https://codereview.appspot.com/12143043/diff/22001/utils/apt.go#newcode61
utils/apt.go:61: "Acquire::ftp::Proxy"}
On 2013/08/05 17:35:39, rog wrote:
> 
> newline before } please

Done.