ec2: moved the http storage reader and tests

ec2: moved the http storage reader and tests

Moved the HTTP based StorageReader and the test server
for it to environs/ec2 and adopted those changes at
the synctools.

https://code.launchpad.net/~themue/juju-core/028-ec2-http-storage-reader/+merge/170439

(do not edit description out of merge proposal)

[mue, 2013-06-19 20:01:42]

Please take a look.

[thumper, 2013-06-20 02:51:58]

On 2013/06/19 20:01:42, mue wrote:
> Please take a look.

LGTM

[dave_cheney.net, 2013-06-20 03:11:42]

LGTM. That looks like where it is supposed to go.

[jameinel, 2013-06-20 06:04:48]

LGTM I like the location for the storage code and tests.

I'm a little concerned about putting a test-only fixture into the core code
(HttpTestStorage).

Also, the convention I've seen elsewhere is to always CAPS or lowercase
abbreviations. Like HTTP and URL. Which would make it HTTPTestStorage. (It seems
to match go convention here:
  http://golang.org/pkg/net/http/#ParseHTTPVersion
)

https://codereview.appspot.com/10296046/diff/1/cmd/juju/synctools.go
File cmd/juju/synctools.go (left):

https://codereview.appspot.com/10296046/diff/1/cmd/juju/synctools.go#oldcode75
cmd/juju/synctools.go:75: }
This is just removed because it is no longer used, right?

https://codereview.appspot.com/10296046/diff/1/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/10296046/diff/1/cmd/juju/synctools.go#newcode109
cmd/juju/synctools.go:109: sourceStorage :=
ec2.NewHttpStorageReader(defaultToolsLocation)
Long term, we probably want a command line flag for this. Even better if we
could also use local disk (so people can copy them manually, and then upload
them without having Internet access). Which is one of the big MaaS/private cloud
use cases.

But this should certainly land as is for now.

https://codereview.appspot.com/10296046/diff/1/environs/ec2/storage.go
File environs/ec2/storage.go (right):

https://codereview.appspot.com/10296046/diff/1/environs/ec2/storage.go#newcod...
environs/ec2/storage.go:274: }
Should this be in the core code rather than only in a test module?

[mue, 2013-06-20 06:43:40]

Please take a look.

[jameinel, 2013-06-20 06:56:41]

LGTM

[mue, 2013-06-20 13:33:18]

Please take a look.

[mue, 2013-06-20 13:43:35]

Please take a look.

[fwereade, 2013-06-21 11:40:49]

LGTM, thanks

[rog, 2013-06-21 12:17:09]

LGTM with some minor suggestions below.

I also think it's a good place for the storage code.

https://codereview.appspot.com/10296046/diff/14001/environs/ec2/storage.go
File environs/ec2/storage.go (right):

https://codereview.appspot.com/10296046/diff/14001/environs/ec2/storage.go#ne...
environs/ec2/storage.go:187: type HTTPStorageReader struct {
any particular reason to export this type?

https://codereview.appspot.com/10296046/diff/14001/environs/ec2/storage.go#ne...
environs/ec2/storage.go:192: // access to an EC2 storage like the juju-dist
storage.
What is location? A URL? A host name?

https://codereview.appspot.com/10296046/diff/14001/environs/ec2/storage.go#ne...
environs/ec2/storage.go:198: // that can be used to read its contents.
// Get implements environs.StorageReader.Get

and below. No need to duplicate all the comments
from environs.StorageReader (unless there's some
new information that needs to be added)

https://codereview.appspot.com/10296046/diff/14001/environs/ec2/storage_test.go
File environs/ec2/storage_test.go (right):

https://codereview.appspot.com/10296046/diff/14001/environs/ec2/storage_test....
environs/ec2/storage_test.go:22: func (s *storageSuite) SetUpTest(c *C) {
is it worth having SetUpTest and TearDownTest when there's only one test?

[mue, 2013-06-21 15:35:30]

https://codereview.appspot.com/10296046/diff/14001/environs/ec2/storage.go
File environs/ec2/storage.go (right):

https://codereview.appspot.com/10296046/diff/14001/environs/ec2/storage.go#ne...
environs/ec2/storage.go:187: type HTTPStorageReader struct {
On 2013/06/21 12:17:09, rog wrote:
> any particular reason to export this type?

No, done. ;)

https://codereview.appspot.com/10296046/diff/14001/environs/ec2/storage.go#ne...
environs/ec2/storage.go:192: // access to an EC2 storage like the juju-dist
storage.
On 2013/06/21 12:17:09, rog wrote:
> What is location? A URL? A host name?

Done.

https://codereview.appspot.com/10296046/diff/14001/environs/ec2/storage.go#ne...
environs/ec2/storage.go:198: // that can be used to read its contents.
On 2013/06/21 12:17:09, rog wrote:
> // Get implements environs.StorageReader.Get
> 
> and below. No need to duplicate all the comments
> from environs.StorageReader (unless there's some
> new information that needs to be added)

Done.

https://codereview.appspot.com/10296046/diff/14001/environs/ec2/storage_test.go
File environs/ec2/storage_test.go (right):

https://codereview.appspot.com/10296046/diff/14001/environs/ec2/storage_test....
environs/ec2/storage_test.go:22: func (s *storageSuite) SetUpTest(c *C) {
On 2013/06/21 12:17:09, rog wrote:
> is it worth having SetUpTest and TearDownTest when there's only one test?

It ended with only one test, I thought it would get more. But the overhead is
not very high and it is more clear this way.