environs/simplestreams: fix GetMetadata

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)

[axw, 2013-10-02 03:12:13]

Please take a look.

[thumper, 2013-10-02 04:02:00]

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simples...
File environs/simplestreams/simplestreams_test.go (right):

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simples...
environs/simplestreams/simplestreams_test.go:300: Series: []string{"precise"},
// never match
What is the never match? precise or arm?  Also, perhaps better to give a region
that doesn't exist?  Someone may add arm at some stage.  Would that work?

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simples...
environs/simplestreams/simplestreams_test.go:326: messages = append(messages,
messages...)
I agree with our IRC chat, ideally we would have a simple mock datasource.  What
about...

type countingSource struct {
  simplestreams.DataSource
  count int
}

func (s *countingSource) URL(path string) (string, error) {
  s.count++
  return s.DataSource.URL(path)
}

Then have sources defined by:

  first :=
&countingSource{DataSource:simplestreams.NewURLDataSource("test:/daily",
simplestreams.VerifySSLHostnames)}
  second :=
&countingSource{DataSource:simplestreams.NewURLDataSource("test:/daily",
simplestreams.VerifySSLHostnames)}
  sources := []simplestream.DataSource{first, second}

  // do stuff

  c.Check(first.count, gc.Equals, 1)
  c.Check(second.count, gc.Equals, 1)

[axw, 2013-10-02 04:48:50]

Please take a look.

[axw1, 2013-10-02 05:07:52]

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simples...
File environs/simplestreams/simplestreams_test.go (right):

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simples...
environs/simplestreams/simplestreams_test.go:300: Series: []string{"precise"},
// never match
On 2013/10/02 04:02:00, thumper wrote:
> What is the never match? precise or arm?  Also, perhaps better to give a
region
> that doesn't exist?  Someone may add arm at some stage.  Would that work?

precise, which isn't in the daily stream I added to testing. I can't use a
non-existent region, as that causes GetMetadata to error, which we don't want.

The series are validated, but architectures aren't. I'll put in an invalid arch.

https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simples...
environs/simplestreams/simplestreams_test.go:326: messages = append(messages,
messages...)
On 2013/10/02 04:02:00, thumper wrote:
> I agree with our IRC chat, ideally we would have a simple mock datasource. 
What
> about...
> 
> type countingSource struct {
>   simplestreams.DataSource
>   count int
> }
> 
> func (s *countingSource) URL(path string) (string, error) {
>   s.count++
>   return s.DataSource.URL(path)
> }
> 
> Then have sources defined by:
> 
>   first :=
> &countingSource{DataSource:simplestreams.NewURLDataSource("test:/daily",
> simplestreams.VerifySSLHostnames)}
>   second :=
> &countingSource{DataSource:simplestreams.NewURLDataSource("test:/daily",
> simplestreams.VerifySSLHostnames)}
>   sources := []simplestream.DataSource{first, second}
> 
>   // do stuff
> 
>   c.Check(first.count, gc.Equals, 1)
>   c.Check(second.count, gc.Equals, 1)
>  

Done, tho I'm reusing a single countingSource.

[axw1, 2013-10-02 07:21:38]

On 2013/10/02 05:07:52, axw1 wrote:
>
https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simples...
> File environs/simplestreams/simplestreams_test.go (right):
> 
>
https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simples...
> environs/simplestreams/simplestreams_test.go:300: Series: []string{"precise"},
> // never match
> On 2013/10/02 04:02:00, thumper wrote:
> > What is the never match? precise or arm?  Also, perhaps better to give a
> region
> > that doesn't exist?  Someone may add arm at some stage.  Would that work?
> 
> precise, which isn't in the daily stream I added to testing. I can't use a
> non-existent region, as that causes GetMetadata to error, which we don't want.
> 
> The series are validated, but architectures aren't. I'll put in an invalid
arch.
> 
>
https://codereview.appspot.com/14218044/diff/1/environs/simplestreams/simples...
> environs/simplestreams/simplestreams_test.go:326: messages = append(messages,
> messages...)
> On 2013/10/02 04:02:00, thumper wrote:
> > I agree with our IRC chat, ideally we would have a simple mock datasource. 
> What
> > about...
> > 
> > type countingSource struct {
> >   simplestreams.DataSource
> >   count int
> > }
> > 
> > func (s *countingSource) URL(path string) (string, error) {
> >   s.count++
> >   return s.DataSource.URL(path)
> > }
> > 
> > Then have sources defined by:
> > 
> >   first :=
> > &countingSource{DataSource:simplestreams.NewURLDataSource("test:/daily",
> > simplestreams.VerifySSLHostnames)}
> >   second :=
> > &countingSource{DataSource:simplestreams.NewURLDataSource("test:/daily",
> > simplestreams.VerifySSLHostnames)}
> >   sources := []simplestream.DataSource{first, second}
> > 
> >   // do stuff
> > 
> >   c.Check(first.count, gc.Equals, 1)
> >   c.Check(second.count, gc.Equals, 1)
> >  
> 
> Done, tho I'm reusing a single countingSource.

So, after discussing with jam, GetMetadata *might* be how it is on purpose. You
might want to populate metadata into your private cloud, and then expect only to
be able to create images that match your private metadata.

I'll leave this for now, until it's decided how it should be working.

[thumper, 2013-10-02 21:23:31]

> So, after discussing with jam, GetMetadata *might* be how it is on purpose.
You
> might want to populate metadata into your private cloud, and then expect only
to
> be able to create images that match your private metadata.
> 
> I'll leave this for now, until it's decided how it should be working.

I think that it should be a configuration option to specify "don't fall-back to
other sources".

[thumper, 2013-10-02 21:24:54]

LGTM

https://codereview.appspot.com/14218044/diff/6001/environs/simplestreams/simp...
File environs/simplestreams/simplestreams_test.go (right):

https://codereview.appspot.com/14218044/diff/6001/environs/simplestreams/simp...
environs/simplestreams/simplestreams_test.go:327: c.Assert(source.count,
gc.Equals, 2*len(sources))
Much nicer.

[fwereade, 2013-10-04 10:26:41]

Fine with the idea, I think the implementation's in the wrong place.

https://codereview.appspot.com/14218044/diff/6001/environs/simplestreams/simp...
File environs/simplestreams/simplestreams.go (right):

https://codereview.appspot.com/14218044/diff/6001/environs/simplestreams/simp...
environs/simplestreams/simplestreams.go:406: if err == nil && len(items) != 0 {
I'm not sure this is the right place for it -- it's a hack-on-a-hack, instead of
removing the original hack in getmaybeSignedMetadata that replaces errors with
nil.

[axw, 2013-10-10 07:05:40]

Please take a look.

https://codereview.appspot.com/14218044/diff/6001/environs/simplestreams/simp...
File environs/simplestreams/simplestreams.go (right):

https://codereview.appspot.com/14218044/diff/6001/environs/simplestreams/simp...
environs/simplestreams/simplestreams.go:406: if err == nil && len(items) != 0 {
On 2013/10/04 10:26:42, fwereade wrote:
> I'm not sure this is the right place for it -- it's a hack-on-a-hack, instead
of
> removing the original hack in getmaybeSignedMetadata that replaces errors with
> nil.

I've moved it, but I think it's much the same either way. What we have is an
internal error that is used to communicate that there are no matching products,
for one of several reasons. It can be quashed in either GetMetadata or
getMaybeSignedMetadata - doesn't really matter which, I think.

[axw, 2013-11-05 04:40:29]

Please take a look.