Code review comment for lp:~axwalk/juju-core/lp1271144-maas-bridge-utils

Revision history for this message
Andrew Wilkins (axwalk) wrote :

On 2014/03/20 09:42:15, rog wrote:
> LGTM with a couple of thoughts below.

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ.go
> File provider/maas/environ.go (right):

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ.go#newcode316
> provider/maas/environ.go:316:
> utils.CommandString(utils.AptGetCommand("update")...),
> It seems a pity that we can't just call SetAptUpdate on the
> cloudinit config, but that would require a change to ComposeUserData
> (might it make sense to pass in a possibly-nil cloudinit.Config to
that
> function?)

> Aside: CommandString should also escape backquotes, although it won't
be a
> problem here

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ_test.go
> File provider/maas/environ_test.go (right):

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ_test.go#newcode208
> provider/maas/environ_test.go:208: const aptGetPrefix = "env
> DEBIAN_FRONTEND=noninteractive apt-get
--option=Dpkg::Options::=--force-confold
> --option=Dpkg::options::=--force-unsafe-io --assume-yes --quiet"
> or aptGetPrefix := utils.AptGetCommand()
> ?

Except you also have to flatten, and I'd rather avoid using the code
that's used by the thing being tested.

https://codereview.appspot.com/77890045/

« Back to merge proposal