Always pick amd64 if it's an option

Always pick amd64 if it's an option



https://code.launchpad.net/~natefinch/juju-core/045-amd64plz/+merge/216612

(do not edit description out of merge proposal)

[natefinch, 2014-04-21 17:33:25]

Please take a look.

[jameinel, 2014-04-21 18:29:28]

If you have a strong feeling about AMD64 then we can stick with it, my personal
preference is for version.Current.Arch.

Beyond that, with a test LGTM

https://codereview.appspot.com/89900043/diff/1/environs/instances/image.go
File environs/instances/image.go (right):

https://codereview.appspot.com/89900043/diff/1/environs/instances/image.go#ne...
environs/instances/image.go:73: if spec.Image.Arch == arch.AMD64 {
I think we really want to prefer version.Current.Arch because we still expose
"--upload-tools".

And, of course, we need a test so we don't break this again accidentally in the
future.

As a side thing, if you can put a name to them, little bits of logic like this
often are nicer (IMO) in a helper function, because then the logic is
"preferLocalArch" without the clutter of how you actually do that in the code.

But that is a style thing that I don't think we're consistent on, so certainly
not something you would have to change. (Though it would be really easy to unit
test :)

[fwereade, 2014-04-23 07:48:41]

On 2014/04/21 18:29:28, jameinel wrote:
> And, of course, we need a test so we don't break this again accidentally in
the
> future.
> 
> As a side thing, if you can put a name to them, little bits of logic like this
> often are nicer (IMO) in a helper function, because then the logic is
> "preferLocalArch" without the clutter of how you actually do that in the code.
> 
> But that is a style thing that I don't think we're consistent on, so certainly
> not something you would have to change. (Though it would be really easy to
unit
> test :)

Helper functions all the way, please. I've heard all the justifications about
how it's different in go; I've struggled through writing, and reading, the
resulting code; and I've seen the rampaging herds of bugs destroy everything we
hold dear. Code really *is* written for humans to read, and only incidentally
for machines to execute; and, more importantly, it needs to be written for
humans to *modify*, and that demands that we be proactive about extracting and
naming bits of functionality (and about using them where they fit; and renaming
them as they evolve).

FWIW, I'm sure I remember writing a byArch image- (or maybe tools-?) sorter at
one point, but it seems to have disappeared. Maybe for good reasons, maybe not,
but I recall my intent being precisely to factor out arch preferences into a
single place that could be moved around and plugged in where it was necessary.

As in all things, season this advice with a liberal heaping of common sense and
practical consideration ;).

[natefinch, 2014-04-23 10:35:53]

On Wed, Apr 23, 2014 at 3:48 AM, <fwereade@gmail.com> wrote:

> On 2014/04/21 18:29:28, jameinel wrote:
>
>> And, of course, we need a test so we don't break this again
>>
> accidentally in the
>
>> future.
>>
>
>  As a side thing, if you can put a name to them, little bits of logic
>>
> like this
>
>> often are nicer (IMO) in a helper function, because then the logic is
>> "preferLocalArch" without the clutter of how you actually do that in
>>
> the code.
>
>  But that is a style thing that I don't think we're consistent on, so
>>
> certainly
>
>> not something you would have to change. (Though it would be really
>>
> easy to unit
>
>> test :)
>>
>
> Helper functions all the way, please. I've heard all the justifications
> about how it's different in go; I've struggled through writing, and
> reading, the resulting code; and I've seen the rampaging herds of bugs
> destroy everything we hold dear. Code really *is* written for humans to
> read, and only incidentally for machines to execute; and, more
> importantly, it needs to be written for humans to *modify*, and that
> demands that we be proactive about extracting and naming bits of
> functionality (and about using them where they fit; and renaming them as
> they evolve).
>

If anyone was trying to tell you Go is different as a way to avoid writing
small helper functions, they were selling you a load of shit.  Go is
subject to the tenets of good software engineering just like any other
language, and "small functions are good" is one of those tenets.  I
definitely should have put this logic in a little function, and I'm sure
when I went to unit test it, I would have done that (one of the benefits of
TDD is that you just do it the right way the first time).

While there are some differences with Go from other languages, this is not
one of them.

FWIW, I'm sure I remember writing a byArch image- (or maybe tools-?)
> sorter at one point, but it seems to have disappeared. Maybe for good
> reasons, maybe not, but I recall my intent being precisely to factor out
> arch preferences into a single place that could be moved around and
> plugged in where it was necessary.
>
> As in all things, season this advice with a liberal heaping of common
> sense and practical consideration ;).
>

I had thought about sorting the arches into some kind of preferential
order, but had NFC what that order should be.  About the only thing I was
pretty sure of was that AMD64 > i386.

[natefinch, 2014-04-23 20:39:54]

Please take a look.