Refactor simplestreams to use a DataSource

Refactor simplestreams to use a DataSource

Simplestreams metadata lookup currently assumes a http 
transport is all that is required, so that all it needs
is a URL from which to fetch the data. However, this is
no longer valid if we want to load simplestreams data
from the environment's private bucket. So this branch
introduces a new simplestreams.DataSource interface
and implementations which provide for the http and
private bucket use cases.

https://code.launchpad.net/~wallyworld/juju-core/simplestreams-control-bucket/+merge/184502

(do not edit description out of merge proposal)

[wallyworld, 2013-09-09 03:57:46]

Please take a look.

[wallyworld, 2013-09-09 04:21:13]

Please take a look.

[fwereade, 2013-09-11 12:27:55]

Thank you, this is fantastic. I don't like the explicit http naming for
non-http-handling things, and I'm suspicious of the string in the Fetch results,
but if you fix the one and clarify the other then LGTM. I'll try to come by
tonight if you want to chat about it.

https://codereview.appspot.com/13619043/diff/4001/cmd/plugins/juju-metadata/t...
File cmd/plugins/juju-metadata/toolsmetadata_test.go (right):

https://codereview.appspot.com/13619043/diff/4001/cmd/plugins/juju-metadata/t...
cmd/plugins/juju-metadata/toolsmetadata_test.go:57: source :=
simplestreams.NewHttpDataSource("file://" + metadataDir + "/tools")
"Http" and "file://" don't mix. Is it maybe a NewURLDataSource? or even just
`NewDataSource(url string) (DataSource, error)`, leaving open the option to
switch implementations based on the url?

https://codereview.appspot.com/13619043/diff/4001/environs/simplestreams/data...
File environs/simplestreams/datasource.go (right):

https://codereview.appspot.com/13619043/diff/4001/environs/simplestreams/data...
environs/simplestreams/datasource.go:18: Fetch(path string) (io.ReadCloser,
string, error)
the returned string is apparently a url, or sometimes a path. Please document
the expectations -- I suppose it's a best guess at useful flavor for the error
messages?

https://codereview.appspot.com/13619043/diff/4001/environs/simplestreams/data...
environs/simplestreams/datasource.go:48: resp, err := httpClient.Get(dataURL)
Yeah, I'm a bit uncomfortable with the "http" naming, when the fundamental
connection with "http" is that we're registering extra handlers for the global
client in order to handle things that *aren't* http. I'd prefer to call it
something more neutral, and clearly document the possibly-surprising
interaction.

https://codereview.appspot.com/13619043/diff/4001/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/13619043/diff/4001/environs/storage.go#newcode32
environs/storage.go:32: // A httpDataSource retrieves data from an
environs.StorageReader.
bad name

https://codereview.appspot.com/13619043/diff/4001/environs/storage.go#newcode37
environs/storage.go:37: // NewHttpDataSource returns a new http datasource
reading from the specified storage.
bad name

https://codereview.appspot.com/13619043/diff/4001/environs/tools/simplestream...
File environs/tools/simplestreams.go (right):

https://codereview.appspot.com/13619043/diff/4001/environs/tools/simplestream...
environs/tools/simplestreams.go:91: if !strings.HasPrefix(url,
"http://juju.canonical.com/tools") {
Hmm, shouldn't this be https?

https://codereview.appspot.com/13619043/diff/4001/environs/tools/urls_test.go
File environs/tools/urls_test.go (right):

https://codereview.appspot.com/13619043/diff/4001/environs/tools/urls_test.go...
environs/tools/urls_test.go:57: "config-tools-url/", "dummy-tools-url/",
"http://juju.canonical.com/tools/"})
again, surely https?

[wallyworld, 2013-09-12 00:17:04]

Thanks for reviewing. I've fixed the issues

https://codereview.appspot.com/13619043/diff/4001/cmd/plugins/juju-metadata/t...
File cmd/plugins/juju-metadata/toolsmetadata_test.go (right):

https://codereview.appspot.com/13619043/diff/4001/cmd/plugins/juju-metadata/t...
cmd/plugins/juju-metadata/toolsmetadata_test.go:57: source :=
simplestreams.NewHttpDataSource("file://" + metadataDir + "/tools")
On 2013/09/11 12:27:55, fwereade wrote:
> "Http" and "file://" don't mix. Is it maybe a NewURLDataSource? or even just
> `NewDataSource(url string) (DataSource, error)`, leaving open the option to
> switch implementations based on the url?

I called it HttpDataSource because it uses Go's http client to get the data. But
you raise a good point. I'll rename to NewURLDataSource

https://codereview.appspot.com/13619043/diff/4001/environs/simplestreams/data...
File environs/simplestreams/datasource.go (right):

https://codereview.appspot.com/13619043/diff/4001/environs/simplestreams/data...
environs/simplestreams/datasource.go:18: Fetch(path string) (io.ReadCloser,
string, error)
On 2013/09/11 12:27:55, fwereade wrote:
> the returned string is apparently a url, or sometimes a path. Please document
> the expectations -- I suppose it's a best guess at useful flavor for the error
> messages?

The returned result is always a URL. For environs storage instances, it is
obtained by calling storage.URL(). I've added some extra documentation.

https://codereview.appspot.com/13619043/diff/4001/environs/simplestreams/data...
environs/simplestreams/datasource.go:48: resp, err := httpClient.Get(dataURL)
On 2013/09/11 12:27:55, fwereade wrote:
> Yeah, I'm a bit uncomfortable with the "http" naming, when the fundamental
> connection with "http" is that we're registering extra handlers for the global
> client in order to handle things that *aren't* http. I'd prefer to call it
> something more neutral, and clearly document the possibly-surprising
> interaction.

Well, Go's net/http/Client instance is the thing doing the Getting, hence the
variable name. I'll rename the variable.

https://codereview.appspot.com/13619043/diff/4001/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/13619043/diff/4001/environs/storage.go#newcode32
environs/storage.go:32: // A httpDataSource retrieves data from an
environs.StorageReader.
On 2013/09/11 12:27:55, fwereade wrote:
> bad name

Ah ffs. Cut'n'paste fail

https://codereview.appspot.com/13619043/diff/4001/environs/storage.go#newcode37
environs/storage.go:37: // NewHttpDataSource returns a new http datasource
reading from the specified storage.
On 2013/09/11 12:27:55, fwereade wrote:
> bad name

ditto

https://codereview.appspot.com/13619043/diff/4001/environs/tools/simplestream...
File environs/tools/simplestreams.go (right):

https://codereview.appspot.com/13619043/diff/4001/environs/tools/simplestream...
environs/tools/simplestreams.go:91: if !strings.HasPrefix(url,
"http://juju.canonical.com/tools") {
On 2013/09/11 12:27:55, fwereade wrote:
> Hmm, shouldn't this be https?

Done.

https://codereview.appspot.com/13619043/diff/4001/environs/tools/urls_test.go
File environs/tools/urls_test.go (right):

https://codereview.appspot.com/13619043/diff/4001/environs/tools/urls_test.go...
environs/tools/urls_test.go:57: "config-tools-url/", "dummy-tools-url/",
"http://juju.canonical.com/tools/"})
On 2013/09/11 12:27:55, fwereade wrote:
> again, surely https?

Done.