Add RemoveAll() to environs.StorageWriter.

Add RemoveAll() to environs.StorageWriter.

There was one method that just about all providers need from their storage
implementations, but which wasn't on the interface: "delete all files."  To
work around this, several providers took their Storage() and downcast it to
their own specific Storage implementation, just to call an internal
deleteAll() method.  The EC2 provider solved it by referring to the
implementation type, rather than the interface type, from the environment
object.  And the Azure one solved it by going straight into the internals from
its Environ.Destroy() implementation.

Adding a "delete all" method to the interface solves all that.  (I provided a
shared basic implementation for the dummy and local storage implementations.
It can't easily be tested on its own, but it's covered indirectly by these
uses.)

The new method is called RemoveAll, for consistency with the existing Remove()
method.  It should now be possible to jack a local storage object into an EC2
provider instance, and indeed into any other provider, to simplify testing.
And some ugly HTTP-level mocking and testing disappears from the Azure
provider.

https://code.launchpad.net/~jtv/juju-core/providers-deleteall/+merge/173857

(do not edit description out of merge proposal)

[jtv.canonical, 2013-07-10 05:47:55]

Please take a look.

[fwereade, 2013-07-10 09:38:48]

This is awesome, LGTM apart from the rogue paste marked below.

https://codereview.appspot.com/11087043/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/11087043/diff/1/environs/interface.go#newcode119
environs/interface.go:119: // Bootstrap initializes the state for the
ehttp://e.linkedin.com/pub/cc?_ri_=X0Gzc2X%3DWQpglLjHJlYQGnWM9Wmzczb5jcumzdzbUza0krpJ1IHy1zbKM6oudBVXtpKX%3DSACTCD&_ei_=ElhY7pRVQKMkByKfU5j5nyTdqpv_9wz7Jge06DvYsd6c0nIJozNk4cNMOPC1Urjg7VaJ00kOxaaDHWP4Pi6FQTEX7HtnX6dCCY5_xSJOUbu4ajHGueKYqV91dfEPm_weAS30DdZlQpvjwJZconUle6z6-oOidmOLrLjX70.nvironment,
possibly
/me raises an eyebrow

[jtv.canonical, 2013-07-10 10:11:46]

On 2013/07/10 09:38:48, fwereade wrote:
> This is awesome, LGTM apart from the rogue paste marked below.

Whoops!  Thanks for spotting that.  No idea how it happened...  Hope it wasn't
anything too embarrassing.  :-)

[jtv.canonical, 2013-07-10 10:15:55]

Please take a look.

[dimitern, 2013-07-10 10:27:49]

Very nice cleanup! LGTM.

[jameinel, 2013-07-10 10:30:23]

LGTM

Given this is a new interface attribute, I'd like to see an associated interface
test. Something in environs/jujutest/livetests.go should work well.

Otherwise, I like the change a lot.

https://codereview.appspot.com/11087043/diff/6001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/11087043/diff/6001/environs/ec2/ec2.go#newcode545
environs/ec2/ec2.go:545: // holding e.ecfgMutex. e.Storage() does this for us.
I think you can remove the comment completely as we aren't pretending to touch
e.storageUnlocked directly.

https://codereview.appspot.com/11087043/diff/6001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/11087043/diff/6001/environs/interface.go#newco...
environs/interface.go:91: RemoveAll() error
If we are adding an API to the official interface, can you add an associated
test to

environs/jujutest/livetests.go ?

That will end up run against every provider (using their double, and when
activated against the actual live instance.)

It doesn't have to be very complex. Start an Environ, Put a file or two, List it
to ensure that they are there, RemoveAll and ensure that they are all gone.

https://codereview.appspot.com/11087043/diff/6001/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/11087043/diff/6001/environs/storage.go#newcode14
environs/storage.go:14: func RemoveAll(stor Storage) error {
Is this used? it looks more like all the implementations already did their own
thing.

[jtv.canonical, 2013-07-10 11:14:07]

On 2013/07/10 10:30:23, jameinel wrote:

> Given this is a new interface attribute, I'd like to see an associated
interface
> test. Something in environs/jujutest/livetests.go should work well.

Thanks for the review!  Some good, critical points here.

Since I'll be reviewing one of yours as well, and to manage scope, I'll add the
test in a separate branch.


>
https://codereview.appspot.com/11087043/diff/6001/environs/ec2/ec2.go#newcode545
> environs/ec2/ec2.go:545: // holding e.ecfgMutex. e.Storage() does this for us.
> I think you can remove the comment completely as we aren't pretending to touch
> e.storageUnlocked directly.

You're right.  It did feel a bit superfluous, but I figured I might be seeing it
less clearly than someone more familiar with the design.  Removed.


>
https://codereview.appspot.com/11087043/diff/6001/environs/interface.go#newco...
> environs/interface.go:91: RemoveAll() error
> If we are adding an API to the official interface, can you add an associated
> test to
> 
> environs/jujutest/livetests.go ?

Will do, after I've reviewed your branch!


>
https://codereview.appspot.com/11087043/diff/6001/environs/storage.go#newcode14
> environs/storage.go:14: func RemoveAll(stor Storage) error {
> Is this used? it looks more like all the implementations already did their own
> thing.

The localstorage implementation uses it.  It didn't have an equivalent of
RemoveAll.