Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(252)

Issue 77890045: Fix maas bridge-utils installation

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years ago by axw
Modified:
10 years ago
Reviewers:
wwitzel3, mp+211886, rog
Visibility:
Public.

Description

Fix maas bridge-utils installation Need to do an apt-get update first. I've updated the code to use the appropriate options to disable all interactivity. Also, changed the code to the ifdown/ifup correctly. Live tested with "juju deploy ubuntu --to lxc:0" Fixes lp:1271144 Fixes lp:1248283 https://code.launchpad.net/~axwalk/juju-core/lp1271144-maas-bridge-utils/+merge/211886 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+67 lines, -12 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M provider/maas/environ.go View 3 chunks +21 lines, -10 lines 2 comments Download
M provider/maas/environ_test.go View 1 chunk +15 lines, -0 lines 1 comment Download
M provider/maas/export_test.go View 1 chunk +3 lines, -2 lines 0 comments Download
M utils/apt.go View 1 chunk +10 lines, -0 lines 1 comment Download
M utils/apt_test.go View 1 chunk +16 lines, -0 lines 0 comments Download

Messages

Total messages: 4
axw
Please take a look.
10 years ago (2014-03-20 09:16:00 UTC) #1
rog
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 ...
10 years ago (2014-03-20 09:42:15 UTC) #2
axw
On 2014/03/20 09:42:15, rog wrote: > LGTM with a couple of thoughts below. > > ...
10 years ago (2014-03-20 09:48:45 UTC) #3
wwitzel3
10 years ago (2014-03-20 10:02:28 UTC) #4
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#newco...
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b