Code review comment for lp:~wallyworld/juju-core/amd64plz

Reviewers: mp+216977_code.launchpad.net,

Message:
Please take a look.

Description:
Always pick amd64 if it's an option

This branch takes lp:~natefinch/juju-core/045-amd64plz
and fixes some tests.
See https://codereview.appspot.com/89900043/

https://code.launchpad.net/~wallyworld/juju-core/amd64plz/+merge/216977

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/90720043/

Affected files (+67, -2 lines):
   A [revision details]
   M environs/instances/image.go
   M environs/instances/image_test.go
   M provider/ec2/export_test.go
   M provider/ec2/image_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140424033650-9am3xzxfcelms2x8
+New revision: <email address hidden>

Index: environs/instances/image.go
=== modified file 'environs/instances/image.go'
--- environs/instances/image.go 2014-04-17 05:44:57 +0000
+++ environs/instances/image.go 2014-04-24 04:56:36 +0000
@@ -8,6 +8,7 @@

   "launchpad.net/juju-core/constraints"
   "launchpad.net/juju-core/environs/imagemetadata"
+ "launchpad.net/juju-core/juju/arch"
  )

  // InstanceConstraint constrains the possible instances that may be
@@ -78,14 +79,19 @@
    return nil, fmt.Errorf("no instance types found matching
constraint: %s", ic)
   }

+ specs := []*InstanceSpec{}
   for _, itype := range matchingTypes {
    for _, image := range possibleImages {
     if image.match(itype) {
- return &InstanceSpec{itype, image}, nil
+ specs = append(specs, &InstanceSpec{itype, image})
     }
    }
   }

+ if spec := preferredSpec(specs); spec != nil {
+ return spec, nil
+ }
+
   names := make([]string, len(matchingTypes))
   for i, itype := range matchingTypes {
    names[i] = itype.Name
@@ -93,6 +99,23 @@
   return nil, fmt.Errorf("no %q images in %s matching instance types %v",
ic.Series, ic.Region, names)
  }

+// preferredSpec will if possible return a spec with arch matching that
+// of the host machine.
+func preferredSpec(specs []*InstanceSpec) *InstanceSpec {
+ if len(specs) > 1 {
+ hostArch := arch.HostArch()
+ for _, spec := range specs {
+ if spec.Image.Arch == hostArch {
+ return spec
+ }
+ }
+ }
+ if len(specs) > 0 {
+ return specs[0]
+ }
+ return nil
+}
+
  // Image holds the attributes that vary amongst relevant images for
  // a given series in a given region.
  type Image struct {

Index: environs/instances/image_test.go
=== modified file 'environs/instances/image_test.go'
--- environs/instances/image_test.go 2014-04-18 13:58:13 +0000
+++ environs/instances/image_test.go 2014-04-23 21:04:24 +0000
@@ -11,6 +11,7 @@
   "launchpad.net/juju-core/constraints"
   "launchpad.net/juju-core/environs/imagemetadata"
   "launchpad.net/juju-core/environs/simplestreams"
+ "launchpad.net/juju-core/juju/arch"
   "launchpad.net/juju-core/testing/testbase"
   "launchpad.net/juju-core/utils"
  )
@@ -306,6 +307,44 @@
   }
  }

+func (s *imageSuite) TestPreferredSpec(c *gc.C) {
+ type prefTest struct {
+ desc string
+ specs []*InstanceSpec
+ expected *InstanceSpec
+ }
+
+ s.PatchValue(&arch.HostArch, func() string { return arch.ARM64 })
+
+ amd64 := &InstanceSpec{Image: Image{Arch: arch.AMD64}}
+ i386 := &InstanceSpec{Image: Image{Arch: arch.I386}}
+ arm64 := &InstanceSpec{Image: Image{Arch: arch.ARM64}}
+
+ prefTests := []prefTest{
+ {
+ "choose hostarch (arm64) over other arches",
+ []*InstanceSpec{i386, arm64, amd64},
+ arm64,
+ },
+ {
+ "choose first image if no arm64",
+ []*InstanceSpec{i386, amd64},
+ i386,
+ },
+ {
+ "choose only image only one there",
+ []*InstanceSpec{amd64},
+ amd64,
+ },
+ }
+
+ for n, test := range prefTests {
+ c.Logf("PreferredSpec test %d: %s", n, test.desc)
+ actual := preferredSpec(test.specs)
+ c.Assert(actual, gc.Equals, test.expected)
+ }
+}
+
  var imageMatchtests = []struct {
   image Image
   itype InstanceType

Index: provider/ec2/export_test.go
=== modified file 'provider/ec2/export_test.go'
--- provider/ec2/export_test.go 2014-04-02 11:35:49 +0000
+++ provider/ec2/export_test.go 2014-04-24 04:56:36 +0000
@@ -152,6 +152,7 @@
     "com.ubuntu.cloud:server:12.04:i386",
     "com.ubuntu.cloud:server:12.04:amd64",
     "com.ubuntu.cloud:server:12.10:amd64",
+ "com.ubuntu.cloud:server:12.10:i386",
     "com.ubuntu.cloud:server:13.04:i386"
       ],
       "path": "streams/v1/com.ubuntu.cloud:released:aws.js"

Index: provider/ec2/image_test.go
=== modified file 'provider/ec2/image_test.go'
--- provider/ec2/image_test.go 2014-04-17 05:44:57 +0000
+++ provider/ec2/image_test.go 2014-04-24 04:56:36 +0000
@@ -64,7 +64,7 @@
    image: "ami-00000033",
   }, {
    series: "quantal",
- arches: both,
+ arches: []string{"i386"},
    itype: "m1.small",
    image: "ami-01000034",
   }, {