Add putFakeTools to openstack tests

Add putFakeTools to openstack tests

Now that the Storage interface is implemented for the openstack provider, we can start using it.
This simple branch adds the putFakeTools functionality similar to what is done in the ec2 tests.
In addition, to save the tests from having to cast PublicStorage to Storage in order to upload tools, a writable
public storage instance is created for the tests to use.

https://code.launchpad.net/~wallyworld/juju-core/upload-fake-tools/+merge/143228

(do not edit description out of merge proposal)

[wallyworld, 2013-01-15 04:23:20]

Please take a look.

[rog, 2013-01-15 10:24:37]

looks mostly reasonable, but a couple of queries below.

https://codereview.appspot.com/7086055/diff/1/environs/openstack/live_test.go
File environs/openstack/live_test.go (right):

https://codereview.appspot.com/7086055/diff/1/environs/openstack/live_test.go...
environs/openstack/live_test.go:80: putFakeTools(c,
e.Storage().(environs.Storage))
i'm not sure why you can't use PublicStorage here. i don't think we need a
world-writable public container because in this case we know that the public
container has the same owner as the private container (it's created for the
purpose of this test).

https://codereview.appspot.com/7086055/diff/1/environs/openstack/live_test.go...
environs/openstack/live_test.go:89: err :=
openstack.DeleteStorageContent(t.Env.Storage().(environs.Storage))
shouldn't the environment destroy do this anyway?

[wallyworld, 2013-01-16 01:06:25]

Please take a look.

https://codereview.appspot.com/7086055/diff/1/environs/openstack/live_test.go
File environs/openstack/live_test.go (right):

https://codereview.appspot.com/7086055/diff/1/environs/openstack/live_test.go...
environs/openstack/live_test.go:80: putFakeTools(c,
e.Storage().(environs.Storage))
On 2013/01/15 10:24:38, rog wrote:
> i'm not sure why you can't use PublicStorage here. i don't think we need a
> world-writable public container because in this case we know that the public
> container has the same owner as the private container (it's created for the
> purpose of this test).

I have changed things so that PublicStorage can be used. The PublicStorage
instance is now created using an authenticating Goose client, which means the
test code can correctly write to it. The container is created using a PublicRead
ACL so that wget etc can read the contents without requiring authorisation.

https://codereview.appspot.com/7086055/diff/1/environs/openstack/live_test.go...
environs/openstack/live_test.go:89: err :=
openstack.DeleteStorageContent(t.Env.Storage().(environs.Storage))
On 2013/01/15 10:24:38, rog wrote:
> shouldn't the environment destroy do this anyway?

Yes, I didn't notice that, thanks. This code is removed now anyway since I made
the changes to make PublicStorage work.

[fwereade, 2013-01-16 08:24:54]

Just a thought...

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

https://codereview.appspot.com/7086055/diff/5001/environs/openstack/provider....
environs/openstack/provider.go:223: swift:         swift.New(e.client(ecfg,
authMethodCfg))}
Wait -- will this actually work with the "real" public storage? My gut is saying
that what we *want* here really is the publicClient, and that it's not
appropriate to add this auth business to the implementation for the convenience
of the tests.

Can we not, in the tests, create an authed "public bucket" and put tools in it;
and then write that public bucket's name/url to the environ config, and connect
to it unauthenticated as we originally would have?

[fwereade, 2013-01-16 21:04:57]

After discussion live, LGTM, assuming the points below are addressed without
perturbing the code too much.

1) I'm concerned about using the wrong credentials when accessing a public
bucket that's not part of the current cloud. If you've verified that it's fine
to use the wrong credentials for everything that's done with the PublicStorage,
then go ahead; please just add a comment pointing out that the credentials may
be wrong and explaining why it doesn't matter.

2) The casting to Storage in the tests is inappropriate, both here and in EC2;
in neither case should the tests have to use the SUT's internals to create the
test fixture. As long as a bug is created to track this infelicity, I think it's
reasonable to merge this branch without resolving it... but ofc I would be
happiest of all if this were fixed here and now :).

[wallyworld, 2013-01-17 05:20:42]

On 2013/01/16 21:04:57, fwereade wrote:
> After discussion live, LGTM, assuming the points below are addressed without
> perturbing the code too much.
> 
> 1) I'm concerned about using the wrong credentials when accessing a public
> bucket that's not part of the current cloud. If you've verified that it's fine
> to use the wrong credentials for everything that's done with the
PublicStorage,
> then go ahead; please just add a comment pointing out that the credentials may
> be wrong and explaining why it doesn't matter.
> 
> 2) The casting to Storage in the tests is inappropriate, both here and in EC2;
> in neither case should the tests have to use the SUT's internals to create the
> test fixture. As long as a bug is created to track this infelicity, I think
it's
> reasonable to merge this branch without resolving it... but ofc I would be
> happiest of all if this were fixed here and now :).
I have reworked the implementation to hopefully address the above concerns.

Unlike ec2, openstack provides true (read only) unauthenticated access to the
public bucket. So I ensured that e.PublicBucket() uses an non-authenticating
client and so is truly readonly. This means that the cast to Storage() in the
tests fails when trying to upload tools etc. So I reworked the test suite setup
to create and use a write enabled client to the public bucket, which is just
used as required in the tests to upload stuff. The implementation business logic
only uses the read only PublicBucket() instance. The effect of this is that the
cast to Storage is gone.

I used the same pattern to fix the ec2 side of things. However, since the public
bucket is writable, the cast to Storage still suffices. But it is now confined
to a test helper method which removes the cast ugliness from the test code
itself.

The other change is that unlike ec2, there is no well defined, universally
available public bucket for openstack yet. So if the specified public bucket
(via the public bucket url) doesn't exist, one is created on the authenticated
user's openstack instance.

[wallyworld, 2013-01-17 05:26:45]

On 2013/01/17 05:20:42, wallyworld wrote:
> On 2013/01/16 21:04:57, fwereade wrote:
> > After discussion live, LGTM, assuming the points below are addressed without
> > perturbing the code too much.
> > 
> > 1) I'm concerned about using the wrong credentials when accessing a public
> > bucket that's not part of the current cloud. If you've verified that it's
fine
> > to use the wrong credentials for everything that's done with the
> PublicStorage,
> > then go ahead; please just add a comment pointing out that the credentials
may
> > be wrong and explaining why it doesn't matter.
> > 
> > 2) The casting to Storage in the tests is inappropriate, both here and in
EC2;
> > in neither case should the tests have to use the SUT's internals to create
the
> > test fixture. As long as a bug is created to track this infelicity, I think
> it's
> > reasonable to merge this branch without resolving it... but ofc I would be
> > happiest of all if this were fixed here and now :).
> I have reworked the implementation to hopefully address the above concerns.
> 
> Unlike ec2, openstack provides true (read only) unauthenticated access to the
> public bucket. So I ensured that e.PublicBucket() uses an non-authenticating
> client and so is truly readonly. This means that the cast to Storage() in the
> tests fails when trying to upload tools etc. So I reworked the test suite
setup
> to create and use a write enabled client to the public bucket, which is just
> used as required in the tests to upload stuff. The implementation business
logic
> only uses the read only PublicBucket() instance. The effect of this is that
the
> cast to Storage is gone.
> 
> I used the same pattern to fix the ec2 side of things. However, since the
public
> bucket is writable, the cast to Storage still suffices. But it is now confined
> to a test helper method which removes the cast ugliness from the test code
> itself.
> 
> The other change is that unlike ec2, there is no well defined, universally
> available public bucket for openstack yet. So if the specified public bucket
> (via the public bucket url) doesn't exist, one is created on the authenticated
> user's openstack instance.

I should clarify the above comment: for the tests, the "public" bucket is
created on the authenticated user's openstack instance. Only the tests have
write access.

Item 1 in the review comment is addressed because credentials are no longer used
when accessing the public bucket.

[wallyworld, 2013-01-17 05:27:43]

Please take a look.

[dimitern, 2013-01-17 17:08:37]

LGTM, assuming others agree as well on the points discussed before.

[rog, 2013-01-17 17:29:54]

LGTM with a couple of minor remarks below.

https://codereview.appspot.com/7086055/diff/13001/environs/openstack/export_t...
File environs/openstack/export_test.go (right):

https://codereview.appspot.com/7086055/diff/13001/environs/openstack/export_t...
environs/openstack/export_test.go:56: swift:        
swift.New(e.(*environ).client(ecfg, authMethodCfg))}
it's more conventional to have the final } on its own line.

https://codereview.appspot.com/7086055/diff/13001/environs/openstack/export_t...
environs/openstack/export_test.go:59: err :=
writeablePublicStorage.makeContainer(ecfg.publicBucket(), swift.PublicRead)
i'd check the error here and panic if it's not nil. it should never fail, right?

then you can lose the error return from this function.

[wallyworld, 2013-01-18 00:59:12]

*** Submitted:

Add putFakeTools to openstack tests

Now that the Storage interface is implemented for the openstack provider, we can
start using it.
This simple branch adds the putFakeTools functionality similar to what is done
in the ec2 tests.
In addition, to save the tests from having to cast PublicStorage to Storage in
order to upload tools, a writable
public storage instance is created for the tests to use.

R=rog, fwereade, dimitern
CC=
https://codereview.appspot.com/7086055