Code review comment for lp://staging/~wallyworld/juju-core/simplestreams-multiple-indices

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/ec2.go#newcode361
environs/ec2/ec2.go:361: // Add the simplestreams base URL off the
public bucket.
On 2013/05/16 08:59:11, fwereade wrote:
> Why not the private bucket?

The simplestreams stuff is all premised on being able to access an index
file relative to a base URL and then navigate to a product file relative
to the same base path. Given that the required authenticated access via
a provider storage interface doesn't really lend itself to the current
URL based approach, I'd like to leave it for now and revisit later if it
comes up as an issue.

Update: we discussed this on a hangout and the current approach is
acceptable.

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/local_test.go
File environs/ec2/local_test.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/local_test.go#newcode389
environs/ec2/local_test.go:389: // The public bucket URL ends with
"/public-tools".
On 2013/05/16 08:59:11, fwereade wrote:
> Sorry, I'm not clear why this is. Is it because of that swapping out
of tools
> for public-tools? If so, I'm pretty sure that's not correct -- tools
> (public-tools) is for *tools*.

> And... shouldn't that be a signed URL anyway?

The public-tools is an artifact of other unrelated work to prevent tests
accidentally overwriting a real public bucket. There's a bug to clean up
how our tests work.

The use of the public bucket for metadata is actually removed in a
downstream branch since it's not needed for EC2 because the metadata is
all available via simplestreams.

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simplestreams.go
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simplestreams.go#newcode212
environs/imagemetadata/simplestreams.go:212: if _, ok :=
err.(*environs.NotFoundError); ok {
On 2013/05/16 06:47:13, jameinel wrote:
> generally I really prefer the:

> environs.IsNotFound(err)

> method. This still works, but I'm wondering if you prefer it?

Sadly only goose has nice error helpers like that

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_test.go
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_test.go#newcode423
environs/openstack/local_test.go:423: c.Check(strings.HasSuffix(urls[1],
"/imagemetadata"), Equals, true)
On 2013/05/16 08:59:11, fwereade wrote:
> Both unsigned..?

Yes. But in a downstream branch, the use of the public bucket URL for
EC2 is removed.

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/provider.go#newcode626
environs/openstack/provider.go:626: publicBucketURL, err :=
e.publicStorageUnlocked.URL("")
On 2013/05/16 08:59:11, fwereade wrote:
> Again, why not private?

The URL based access used by simplestreams doesn't really lend itself to
the client based access to the private bucket.

https://codereview.appspot.com/9128047/