environs: add Prepare method

environs: add Prepare method

This provides us with the hook that we will
need to enable providers to generate attributes
when bootstrapping (to be saved for later reconnection).

We change the dummy provider so that it creates
an environment's state object at Prepare time - this
catches all the places that we're expecting an environment
without preparing it. Most of the changes in this
CL are a consequence of making tests work after
this change.

There's one other drive-by change in the dummy provider - the
firewall mode is stored locally in the environ, rather than
in the state. This corresponds more closely with how
the non-dummy providers work (and the existing code
was also not thread safe)

Another change: rather than changing sync.SyncContext
to call Prepare, I thought it better to make it use the
minimal interface that it requires, leaving the Prepare
up to the caller - this seems like a better division of
responsibilities.

https://code.launchpad.net/~rogpeppe/juju-core/368-environs-prepare/+merge/181768

Requires: https://code.launchpad.net/~rogpeppe/juju-core/373-ec2-no-s3-retry/+merge/181759

(do not edit description out of merge proposal)

[rog, 2013-08-23 10:46:55]

Please take a look.

[gz, 2013-08-23 11:40:38]

LGTM.

https://codereview.appspot.com/13187043/diff/3001/agent/tools/storage_test.go
File agent/tools/storage_test.go (right):

https://codereview.appspot.com/13187043/diff/3001/agent/tools/storage_test.go...
agent/tools/storage_test.go:48: c.Assert(err, gc.IsNil)
Hm, rather than exploding this logic in tests, why not keep a trivial helper
than still bundles New and Prepare into one?

https://codereview.appspot.com/13187043/diff/3001/cmd/juju/synctools_test.go
File cmd/juju/synctools_test.go (right):

https://codereview.appspot.com/13187043/diff/3001/cmd/juju/synctools_test.go#...
cmd/juju/synctools_test.go:115: // makeEmptyFakeHome creates a faked home
without tools.
What's this comment about?

https://codereview.appspot.com/13187043/diff/3001/provider/dummy/environs.go
File provider/dummy/environs.go (right):

https://codereview.appspot.com/13187043/diff/3001/provider/dummy/environs.go#...
provider/dummy/environs.go:703: defer estate.mu.Unlock()
Okay, these lock changes are certainly an improvement, but it's still a lot of
boilerplate per function...

https://codereview.appspot.com/13187043/diff/3001/provider/local/environprovi...
File provider/local/environprovider.go (right):

https://codereview.appspot.com/13187043/diff/3001/provider/local/environprovi...
provider/local/environprovider.go:35: cfg, err =
cfg.Apply(map[string]interface{}{
Just curious, why var rather than :=? Doesn't take blame on the line below which
is nice I guess.

[rog, 2013-08-23 12:35:43]

Please take a look.

https://codereview.appspot.com/13187043/diff/3001/agent/tools/storage_test.go
File agent/tools/storage_test.go (right):

https://codereview.appspot.com/13187043/diff/3001/agent/tools/storage_test.go...
agent/tools/storage_test.go:48: c.Assert(err, gc.IsNil)
On 2013/08/23 11:40:38, gz wrote:
> Hm, rather than exploding this logic in tests, why not keep a trivial helper
> than still bundles New and Prepare into one?

I definitely considered that, but I was tending towards
the view that it's nicer to have a smaller, more coherent
surface area in the environs package, rather than
helpers for every possible combination of (Prepare|New)From(Name|Config|Attrs).

It's two more lines of code, it's obvious what's
going on (I hope) and this is really only an issue
for test code, but if you still feel strongly,
I'll add the extra function.

https://codereview.appspot.com/13187043/diff/3001/cmd/juju/synctools_test.go
File cmd/juju/synctools_test.go (right):

https://codereview.appspot.com/13187043/diff/3001/cmd/juju/synctools_test.go#...
cmd/juju/synctools_test.go:115: // makeEmptyFakeHome creates a faked home
without tools.
On 2013/08/23 11:40:38, gz wrote:
> What's this comment about?

An excellent question indeed.
Removed.

https://codereview.appspot.com/13187043/diff/3001/provider/dummy/environs.go
File provider/dummy/environs.go (right):

https://codereview.appspot.com/13187043/diff/3001/provider/dummy/environs.go#...
provider/dummy/environs.go:703: defer estate.mu.Unlock()
On 2013/08/23 11:40:38, gz wrote:
> Okay, these lock changes are certainly an improvement, but it's still a lot of
> boilerplate per function...

The old code had the lock too - we've got only one line more of
boilerplate (the line that retrieves the environState object).

I just looked at possible ways of improving matters
(lockState/unlockState methods; a stateMutex method; a withState
method) but none of them really help much - we've
got to get the environState object, and we've got to do the
locking. If you have a concrete suggestion for an improvement,
I'd like to hear it.

https://codereview.appspot.com/13187043/diff/3001/provider/local/environprovi...
File provider/local/environprovider.go (right):

https://codereview.appspot.com/13187043/diff/3001/provider/local/environprovi...
provider/local/environprovider.go:35: cfg, err =
cfg.Apply(map[string]interface{}{
On 2013/08/23 11:40:38, gz wrote:
> Just curious, why var rather than :=? Doesn't take blame on the line below
which
> is nice I guess.

Because we're deliberately replacing the config object.
We could do something like:

newCfg, err := cfg.Apply(map[string]interface{}{...})
cfg = newCfg

which is probably slightly more readable in fact.
I've done that.

[gz, 2013-08-23 15:51:27]

On 2013/08/23 12:35:43, rog wrote:
> 
> It's two more lines of code, it's obvious what's
> going on (I hope) and this is really only an issue
> for test code, but if you still feel strongly,
> I'll add the extra function.

It's not done quite as much later in the tests as I thought it would be (many
use PrepareFromName instead), but does still happen a few times. Your call.

> The old code had the lock too - we've got only one line more of
> boilerplate (the line that retrieves the environState object).
> 
> I just looked at possible ways of improving matters
> (lockState/unlockState methods; a stateMutex method; a withState
> method) but none of them really help much - we've
> got to get the environState object, and we've got to do the
> locking. If you have a concrete suggestion for an improvement,
> I'd like to hear it.

A withState style thing is what I'd tend towards, but mostly because that's an
idiom I'm comfortable with from python, and is a one-liner there. I don't think
this branch needs to improve anything on that front, it's already better than
the code it's replacing.

> On 2013/08/23 11:40:38, gz wrote:
> > Just curious, why var rather than :=? Doesn't take blame on the line below
> which
> > is nice I guess.
> 
> Because we're deliberately replacing the config object.
> We could do something like:
> 
> newCfg, err := cfg.Apply(map[string]interface{}{...})
> cfg = newCfg
> 
> which is probably slightly more readable in fact.
> I've done that.

Aha, that detail had completely passed me by, good job making it explicit.

[rog, 2013-08-24 13:51:35]

Please take a look.

[gz, 2013-08-24 14:56:27]

On 2013/08/24 13:51:35, rog wrote:
> Please take a look.

The diff on rietveld is now a little confused, but just seems to be existing
changes from trunk. From diff on launchpad looks good to go.

[rog, 2013-08-27 11:27:07]

Please take a look.