environs/dummy: add state-id

environs/dummy: add state-id

This makes the dummy provider more like the other
providers in that the Environ does not refer
directly to the bootstrapped state, but is looked
up when operations are performed on it.

This is also a test case for adding attributes at Prepare
time - many tests needed changing to correctly
deal with this.

https://code.launchpad.net/~rogpeppe/juju-core/375-dummy-prepare-state-id/+merge/188643

Requires: https://code.launchpad.net/~rogpeppe/juju-core/426-environs-add-store-to-new/+merge/188409

(do not edit description out of merge proposal)

[rog, 2013-10-01 16:54:44]

Please take a look.

[fwereade, 2013-10-02 09:40:49]

LGTM, although I'm a bit concerned about ErrNotPrepared. I kinda feel it's more
of a panic..?

https://codereview.appspot.com/14207046/diff/3001/environs/bootstrap/bootstra...
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/14207046/diff/3001/environs/bootstrap/bootstra...
environs/bootstrap/bootstrap.go:102: // be moved into provider and called by the
providers themselves.
I agree this is a smell. Saw it last night, didn't deal with it at the time
because bootstrap state handling is all over the shop anyway :-/.

https://codereview.appspot.com/14207046/diff/3001/environs/config_test.go
File environs/config_test.go (left):

https://codereview.appspot.com/14207046/diff/3001/environs/config_test.go#old...
environs/config_test.go:224: env, err := environs.New(cfg)
AFAICT this is just testing the dummy environ and has no ability to tell us
anything useful about real code.

Individual tests for the individual provider configs, that unknown fields are
preserved through all Prepare/Validate/SetConfig/Config sorts of ops, might be
good; but I think they're out of scope here.

https://codereview.appspot.com/14207046/diff/3001/environs/open_test.go
File environs/open_test.go (right):

https://codereview.appspot.com/14207046/diff/3001/environs/open_test.go#newco...
environs/open_test.go:104: c.Assert(err, gc.ErrorMatches, "environment is not
prepared")
This is a bit of a baffling message if it leaks out to where a user can see it.

https://codereview.appspot.com/14207046/diff/3001/provider/error.go
File provider/error.go (right):

https://codereview.appspot.com/14207046/diff/3001/provider/error.go#newcode11
provider/error.go:11: ErrNotPrepared = errors.New("environment is not prepared")
Do we definitely prevent this from leaking out? Because there is literally no
way for a user to infer a reasonable corrective action if presented with this
error...

[rog, 2013-10-02 12:24:50]

Please take a look.

https://codereview.appspot.com/14207046/diff/3001/environs/config_test.go
File environs/config_test.go (left):

https://codereview.appspot.com/14207046/diff/3001/environs/config_test.go#old...
environs/config_test.go:224: env, err := environs.New(cfg)
On 2013/10/02 09:40:49, fwereade wrote:
> AFAICT this is just testing the dummy environ and has no ability to tell us
> anything useful about real code.
> 
> Individual tests for the individual provider configs, that unknown fields are
> preserved through all Prepare/Validate/SetConfig/Config sorts of ops, might be
> good; but I think they're out of scope here.

i've removed the test

https://codereview.appspot.com/14207046/diff/3001/environs/open_test.go
File environs/open_test.go (right):

https://codereview.appspot.com/14207046/diff/3001/environs/open_test.go#newco...
environs/open_test.go:104: c.Assert(err, gc.ErrorMatches, "environment is not
prepared")
On 2013/10/02 09:40:49, fwereade wrote:
> This is a bit of a baffling message if it leaks out to where a user can see
it.

It should not leak out to the user, but *can* do so in some situations, for
backward compatibility reasons (we want to allow a user to talk to an existing
bootstrapped environment with a current environments.yaml file even though they
didn't bootstrap locally and therefore do not have any environment info).

As discussed online, we'll leave this as is for the time being.