Code review comment for lp:~dimitern/juju-core/290-lp-1240667-cloud-tools-priority

Please take a look.

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go
File cloudinit/options.go (right):

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go#newcode17
cloudinit/options.go:17: // AptPreferencesTemplate defines the format
used to create an
On 2014/02/11 17:31:54, rog wrote:
> I don't really like using printf format strings as poor-man's
> templates, particularly when the data is already in a struct,
> and we have text/template.

> I'd do something like:

> var aptPreferences = template.Must(template.New("").Parse(
> `Explanation: {{.Explanation}}
> Package: {{.Package}}
> Pin: {{.Pin}}
> Pin-Priority: {{.PinPriority}}
> `))

> Then below, you can do:

> if prefs != nil {
> var buf bytes.Buffer
> err := aptPreferencesTemplate.Execute(&buf, prefs)
> if err != nil {
> panic(err)
> }
> cfg.AddFile(prefs.Path, buf.String(), 0644)
> }

> then the template is easier to read and easier to change
> in the future.

> Also, rather than exporting the template, you could
> define a method on AptPreferences to return the
> file contents:

> e.g.

> // FileContents returns file contents suitable
> // for setting up the given apt preferences.
> func (prefs *AptPreferences) FileContents() string

Good point, done!

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go#newcode134
cloudinit/options.go:134: cfg.AddPackage(fmt.Sprintf("--target-release
'%s' '%s'", targetRelease, packageName))
On 2014/02/11 17:05:01, gz wrote:
> This is the only dodgy bit of the operation, I'm not sure if we should
really
> rely on cloud-init passing through flags on the packages key like
this, but
> given our tie to a specific older cloud-init on precise, as it happens
to work
> we can probably get away with it. Let's check with Scott.

As discussed on IRC with smoser, this is fine and I tested it to do the
right thing. It's much simpler than passing an entire certificate file
with rigid formatting through cloudinit. I added tests for all newly
generated code.

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go#newcode134
cloudinit/options.go:134: cfg.AddPackage(fmt.Sprintf("--target-release
'%s' '%s'", targetRelease, packageName))
On 2014/02/11 17:31:54, rog wrote:
> +1 to mgz's remarks.
> Also:

> cfg.AddPackage(fmt.Sprintf("--target-release %s %s",
> utils.ShQuote(targetRelease), utils.ShQuote(packageName)))

I don't want to shquote these here, because they weren't escaped before
and they get escaped at rendering time, somewhat differently in
cloudinit and sshinit, after doing some checks.

https://codereview.appspot.com/61410051/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/61410051/diff/1/environs/cloudinit/cloudinit.go#newcode213
environs/cloudinit/cloudinit.go:213: c.AddFile(noncefile,
cfg.MachineNonce, 0644)
On 2014/02/11 17:31:54, rog wrote:
> +1 to this drive-by, assuming it's tested.

Yep, it is - works fine.

https://codereview.appspot.com/61410051/