Fail earlier when bootstrapping if up already.

Fail earlier when bootstrapping if up already.

I broke apart the VerifyBootstrap method into two:
  EnsureNotBootstrapped
  VerifyStorage
Well I didn't create the second, just didn't call it
from the EnsureNotBootstrapped call.

The ensure method is now called in the command bootstrap
prior to any upload tools checks.

However the implication of this was much futher reaching
than I expected.  Many tests assumed that bootstrap.Bootstrap
was enough to bootstrap and environment, and that it would
complain if already bootstrapped.  Since the complaining was
moved into the command function, some changes had to be made.
Since some of the tests were refactored to check the storage,
this required making the dummy provider actually write the
provider-state into storage like a normal provider.

I moved the VerifyStorage out of emptystorage.go and into open.go
as it seemed better fitting there (and the comment actually
makes sense now).

Also used the environs constant for the VerificationFilename.

https://code.launchpad.net/~thumper/juju-core/verify-storage-move/+merge/188974

(do not edit description out of merge proposal)

[thumper, 2013-10-03 02:06:53]

Please take a look.

[dave_cheney.net, 2013-10-03 03:19:01]

LGTM. Some nice boy scout work in there as well.

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.g...
environs/bootstrap/bootstrap.go:92: _, err := common.LoadState(env.Storage())
this method hurt my head, but it makes sense in a double negative sort of way.

[axw1, 2013-10-03 03:20:48]

On 2013/10/03 03:19:01, dfc wrote:
> LGTM. Some nice boy scout work in there as well.
> 
> https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go
> File environs/bootstrap/bootstrap.go (right):
> 
>
https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.g...
> environs/bootstrap/bootstrap.go:92: _, err := common.LoadState(env.Storage())
> this method hurt my head, but it makes sense in a double negative sort of way.

LGTM

[axw1, 2013-10-03 03:21:00]

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (left):

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.g...
environs/bootstrap/bootstrap.go:96: // TODO(rog) this feels like a layering
violation - providers
Is this TODO no longer applicable? Seems to be...

[thumper, 2013-10-03 03:38:01]

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (left):

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.g...
environs/bootstrap/bootstrap.go:96: // TODO(rog) this feels like a layering
violation - providers
On 2013/10/03 03:21:00, axw1 wrote:
> Is this TODO no longer applicable? Seems to be...

I took it out because everything expects it to be there.
It is how bootstrap checks to see if the environment
is already bootstrapped.  I think for consistency across
providers this comment no longer makes sense.

[rog, 2013-10-03 16:50:26]

I have reservations about this.
I don't think we should be further entrenching the arbitrary way in which the
ec2 provider happened to mark its server instances.
Some thoughts below.

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (left):

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.g...
environs/bootstrap/bootstrap.go:96: // TODO(rog) this feels like a layering
violation - providers
On 2013/10/03 03:38:01, thumper wrote:
> On 2013/10/03 03:21:00, axw1 wrote:
> > Is this TODO no longer applicable? Seems to be...
> 
> I took it out because everything expects it to be there.
> It is how bootstrap checks to see if the environment
> is already bootstrapped.  I think for consistency across
> providers this comment no longer makes sense.

The comment *is* still relevant IMHO. It has long been
the plan to move away from using file storage on ec2 to
indicate state servers (it could be done nicely with
instance tags) and therefore allow the possibility of
having an environment with no local storage at all.

It is *currently* how bootstrap checks to see if the
environment is already bootstrapped, but there's no
particular necessity for it to be this way.
It would be better to have an Environ.IsBootstrapped
call instead.

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.g...
environs/bootstrap/bootstrap.go:91: func EnsureNotBootstrapped(env
environs.Environ) error {
We usually use the word "ensure" to mean "ensure that something is so".
That's not the case here.

I'd prefer:

// IsBootstrapped returns whether the environment is
// bootstrapped.
func IsBootstrapped(env environs.Environ) (bool, error)

https://codereview.appspot.com/14279044/diff/1/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/14279044/diff/1/environs/open.go#newcode29
environs/open.go:29: VerifyStorageError error = fmt.Errorf(
s/ error//