Check each URL for sign and unsigned metadata

Check each URL for sign and unsigned metadata

When looking for simplestreams metadata, the order
used to be: check all urls for signed, then check all
urls for unsigned metadata. Now the order is: check
each url in turn for signed and then unsigned metadata.

There are no tests for signed metadata as such; this is
the case before this branch. A followup branch will add some
new signed metadata tests.

https://code.launchpad.net/~wallyworld/juju-core/simplestreams-signed-metadata-chamge/+merge/187467

(do not edit description out of merge proposal)

[wallyworld, 2013-09-25 08:34:30]

Please take a look.

[jameinel, 2013-09-25 09:24:40]

I really like the changes, however it does feel like we are missing a test.
Something that shows the actual search order. You changed us from searching:
 http://a.sjson
 http://b.sjson
 http://a.json
 http://b.json

Into:
 http://a.sjson
 http://a.json
 http://b.sjson
 http://b.json

And no actual test data was changed in any way. I do think it is something that
we want to assert about how we handle the order of requests.

One thought is to put data into multiple locations, and assert that the content
we get back is from the right priority. (So if you put the data 1,2,3,4 in the
various locations, you assert you get back 1, then if 1 is removed, you get 2,
etc.)

I realize it might be hard, so I won't strictly block on it. But it does feel
like a fundamental design decision that we should have recorded in test form.

LGTM

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

https://codereview.appspot.com/13899044/diff/1/environs/simplestreams/simples...
environs/simplestreams/simplestreams.go:379: unsignedSuffix   = ".json"
Given these are const, is there a strong reason to make them private?

https://codereview.appspot.com/13899044/diff/1/environs/simplestreams/simples...
environs/simplestreams/simplestreams.go:435: logger.Debugf("skipping index
because of error getting latest metadata %q: %v", indexURL, err)
Given we checked IsNotFound I'm not sure logging the error here helps a lot.
Especially since we *are* propagating the error up a level.

[wallyworld, 2013-09-25 10:51:54]

On 2013/09/25 09:24:40, jameinel wrote:
> I really like the changes, however it does feel like we are missing a test.
> Something that shows the actual search order. You changed us from searching:
>  http://a.sjson
>  http://b.sjson
>  http://a.json
>  http://b.json
> 
> Into:
>  http://a.sjson
>  http://a.json
>  http://b.sjson
>  http://b.json
> 
> And no actual test data was changed in any way. I do think it is something
that
> we want to assert about how we handle the order of requests.
>

There never was proper test coverage for signed metadata even before this
branch, as noted in the review comment. A followup branch needs to set up a
public/private keypair and generate some signed metadata etc. It's a fairly big
job. It's on the list of things to do.

[wallyworld, 2013-09-25 10:54:16]

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

https://codereview.appspot.com/13899044/diff/1/environs/simplestreams/simples...
environs/simplestreams/simplestreams.go:379: unsignedSuffix   = ".json"
On 2013/09/25 09:24:40, jameinel wrote:
> Given these are const, is there a strong reason to make them private?

I really don't want things outside using parts of file names. The only thing
that used to use them was tests and I believe tests should more often than not
use the text rather than a const.

https://codereview.appspot.com/13899044/diff/1/environs/simplestreams/simples...
environs/simplestreams/simplestreams.go:435: logger.Debugf("skipping index
because of error getting latest metadata %q: %v", indexURL, err)
On 2013/09/25 09:24:40, jameinel wrote:
> Given we checked IsNotFound I'm not sure logging the error here helps a lot.
> Especially since we *are* propagating the error up a level.

It helped at some point I recall. Maybe not now. I'll see if it makes sense to
remove it.