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

Revision history for this message
Wayne Witzel III (wwitzel3) wrote :

LGTM with a couple of comments, but not show stoppers.

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")...),
On 2014/03/20 09:42:15, rog wrote:
> 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?)

I agree, it is a shame we can't just call SetAptUpdate ... how much
extra work would it be to make the refactor?

https://codereview.appspot.com/77890045/diff/1/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/77890045/diff/1/utils/apt.go#newcode88
utils/apt.go:88: func AptGetCommand(args ...string) []string {
I have no complaint about the composition of this function, just than we
are using it to perform an apt-get install, when we have a func
AptGetInstall.

At the least I feel like any of the existing Apt functions should use
this under the covers.

This also kind of ties back to if we can refactor the ComposeUserData to
call SetAptUpdate for the cloudinit. We could then use AptGetInstall for
the package install and this function could go away entirely.

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

« Back to merge proposal