|
|
|
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)
Patch Set 1 #
Total comments: 1
Patch Set 2 : Always pick amd64 if it's an option #
Total messages: 5
|
natefinch
Please take a look.
|
11 years, 8 months ago (2014年04月21日 17:33:25 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
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 :)
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 ;).
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.