Code review comment for lp:~axwalk/juju-core/lp1233924-simplestreams-getmetadata-fallback

Reviewers: mp+188758_code.launchpad.net,

Message:
Please take a look.

Description:
environs/simplestreams: fix GetMetadata

GetMetadata was dropping out if a datasource
returned no matching product IDs, without an
error. I've changed it to check that a non-
empty list of products is returned before
bailing out.

Fixes #1233924

https://code.launchpad.net/~axwalk/juju-core/lp1233924-simplestreams-getmetadata-fallback/+merge/188758

(do not edit description out of merge proposal)

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

Affected files (+71, -2 lines):
   A [revision details]
   M environs/simplestreams/simplestreams.go
   M environs/simplestreams/simplestreams_test.go
   M environs/simplestreams/testing/testing.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-20131001213504-4ynrkuya2i22q87q
+New revision: <email address hidden>

Index: environs/simplestreams/simplestreams.go
=== modified file 'environs/simplestreams/simplestreams.go'
--- environs/simplestreams/simplestreams.go 2013-09-27 07:05:45 +0000
+++ environs/simplestreams/simplestreams.go 2013-10-02 03:04:13 +0000
@@ -17,6 +17,7 @@
   "io/ioutil"
   "net/http"
   "os"
+ "path"
   "reflect"
   "sort"
   "strings"
@@ -25,7 +26,6 @@
   "launchpad.net/loggo"

   "launchpad.net/juju-core/errors"
- "path"
  )

  var logger = loggo.GetLogger("juju.environs.simplestreams")
@@ -403,7 +403,7 @@
    if err != nil && len(items) == 0 && !onlySigned {
     items, err = getMaybeSignedMetadata(source, baseIndexPath, cons, false,
params)
    }
- if err == nil {
+ if err == nil && len(items) != 0 {
     break
    }
   }

Index: environs/simplestreams/simplestreams_test.go
=== modified file 'environs/simplestreams/simplestreams_test.go'
--- environs/simplestreams/simplestreams_test.go 2013-09-26 04:11:52 +0000
+++ environs/simplestreams/simplestreams_test.go 2013-10-02 03:04:13 +0000
@@ -9,9 +9,11 @@
   "testing"

   gc "launchpad.net/gocheck"
+ "launchpad.net/loggo"

   "launchpad.net/juju-core/environs/simplestreams"
   sstesting "launchpad.net/juju-core/environs/simplestreams/testing"
+ jc "launchpad.net/juju-core/testing/checkers"
  )

  func Test(t *testing.T) {
@@ -284,6 +286,47 @@
   c.Check(dotOFormats, gc.DeepEquals,
simplestreams.IndexMetadataSlice{array[0], array[2]})
  }

+func (s *simplestreamsSuite) TestGetMetadataNoMatching(c *gc.C) {
+ sources := []simplestreams.DataSource{
+ simplestreams.NewURLDataSource("test:/daily",
simplestreams.VerifySSLHostnames),
+ simplestreams.NewURLDataSource("test:/daily",
simplestreams.VerifySSLHostnames),
+ }
+ params := simplestreams.ValueParams{DataType: "image-ids"}
+ constraint := sstesting.NewTestConstraint(simplestreams.LookupParams{
+ CloudSpec: simplestreams.CloudSpec{
+ Region: "us-east-1",
+ Endpoint: "https://ec2.us-east-1.amazonaws.com",
+ },
+ Series: []string{"precise"}, // never match
+ Arches: []string{"arm"},
+ })
+
+ tw := &loggo.TestWriter{}
+ c.Assert(loggo.RegisterWriter("filter-tester", tw, loggo.DEBUG), gc.IsNil)
+ defer loggo.RemoveWriter("filter-tester")
+
+ items, err := simplestreams.GetMetadata(
+ sources,
+ simplestreams.DefaultIndexPath,
+ constraint,
+ false,
+ params,
+ )
+ c.Assert(err, gc.IsNil)
+ c.Assert(items, gc.HasLen, 0)
+ messages := []jc.SimpleMessage{
+ {loggo.DEBUG, "cannot find URL.*sjson"},
+ {loggo.DEBUG, "cannot load index.*sjson"},
+ {loggo.DEBUG, "read metadata index at.*index\\.json"},
+ {loggo.DEBUG, "index file has no data for product name"},
+ }
+ // We should get duplicates of the messages, as the first
+ // data source returns nothing, causing GetMetadata to
+ // attempt the next one.
+ messages = append(messages, messages...)
+ c.Check(tw.Log, jc.LogMatches, messages)
+}
+
  func (s *simplestreamsSuite) TestMetadataCatalog(c *gc.C) {
   metadata := s.AssertGetMetadata(c)
   c.Check(len(metadata.Products), gc.Equals, 2)

Index: environs/simplestreams/testing/testing.go
=== modified file 'environs/simplestreams/testing/testing.go'
--- environs/simplestreams/testing/testing.go 2013-09-26 04:11:52 +0000
+++ environs/simplestreams/testing/testing.go 2013-10-02 03:04:13 +0000
@@ -17,6 +17,30 @@
  )

  var imageData = map[string]string{
+ "/daily/streams/v1/index.json": `
+ {
+ "index": {
+ "com.ubuntu.cloud:released:raring": {
+ "updated": "Wed, 01 May 2013 13:31:26 +0000",
+ "clouds": [
+ {
+ "region": "us-east-1",
+ "endpoint": "https://ec2.us-east-1.amazonaws.com"
+ }
+ ],
+ "cloudname": "aws",
+ "datatype": "image-ids",
+ "format": "products:1.0",
+ "products": [
+ "com.ubuntu.cloud:server:13.04:amd64"
+ ],
+ "path": "streams/v1/raring_metadata.json"
+ }
+ },
+ "updated": "Wed, 01 May 2013 13:31:26 +0000",
+ "format": "index:1.0"
+ }
+ `,
   "/streams/v1/index.json": `
    {
     "index": {