environs: make Prepare create config info

environs: make Prepare create config info

This is the next step on the path to storing environment
info at prepare time. We don't actually write
any information to the configuration yet.

https://code.launchpad.net/~rogpeppe/juju-core/403-prepare-createinfo/+merge/187250

(do not edit description out of merge proposal)

[rog, 2013-09-24 14:45:05]

Please take a look.

[mue, 2013-09-24 15:04:25]

LGTM

[gz, 2013-09-24 15:07:23]

LGTM.

https://codereview.appspot.com/13573046/diff/5001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/13573046/diff/5001/cmd/juju/cmd_test.go#newcod...
cmd/juju/cmd_test.go:182: func (*CmdSuite)
TestDestroyEnvironmentCommandConfirmation(c *gc.C) {
This is one giant test. Seems a shame to be adding more to it rather than
splitting it into smaller bits.

https://codereview.appspot.com/13573046/diff/5001/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (left):

https://codereview.appspot.com/13573046/diff/5001/environs/jujutest/livetests...
environs/jujutest/livetests.go:880: // already bootstrapped.
Which bit fixed this issue?

https://codereview.appspot.com/13573046/diff/5001/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/13573046/diff/5001/environs/open.go#newcode90
environs/open.go:90: return New(config)
Implicit tuple unpacking... this always makes me look twice for the second
return value.

[rog, 2013-09-24 16:04:03]

Please take a look.

https://codereview.appspot.com/13573046/diff/5001/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (left):

https://codereview.appspot.com/13573046/diff/5001/environs/jujutest/livetests...
environs/jujutest/livetests.go:880: // already bootstrapped.
On 2013/09/24 15:07:23, gz wrote:
> Which bit fixed this issue?

We're no longer writing to the environment's storage
after destroying it - we now Prepare it first.

https://codereview.appspot.com/13573046/diff/5001/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/13573046/diff/5001/environs/open.go#newcode90
environs/open.go:90: return New(config)
On 2013/09/24 15:07:23, gz wrote:
> Implicit tuple unpacking... this always makes me look twice for the second
> return value.

I *think* it reads OK here as New is so similar in usage and intent to Prepare.

[fwereade, 2013-09-24 16:59:11]

This is awesome, LGTM.

https://codereview.appspot.com/13573046/diff/5001/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/13573046/diff/5001/environs/open.go#newcode88
environs/open.go:88: return nil, fmt.Errorf("found uninitialized environment
info for %q; environment preparation probably in progress or interrupted",
config.Name())
`juju destroy-environment` will resolve this situation, right?

https://codereview.appspot.com/13573046/diff/5001/environs/open.go#newcode101
environs/open.go:101: // TODO(rog) 2013-09-19 add newly created attributes to
info.
Hmm. Will we be automatically storing the diff between config and env.Config()?
That seems like it may be altogether too much. I'm wondering about having
p.Prepare return the fields it wants to retain (independent of the fields that
happened to get set/changed as a result of interpreting config as a
provider-specific config in the first place). That is, in short, we don't want
to store inserted defaults.

That'd imply that this Prepare func would shortly be expected to handle
admin-secret, ca-cert, and ca-private-key; and that p.Prepare could then handle
generating the minimal info necessary to reconstruct the completed config. Does
this roughly fit with your thinking?

Hey hey *that* fits really well with what we'll need to do with firewalling
jobs. This func lets us do exactly what I was worried might be icky. I'll cc
you.

[natefinch, 2013-09-24 19:29:01]

LGTM with a couple small tweaks recommended.

https://codereview.appspot.com/13573046/diff/5001/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/13573046/diff/5001/environs/open.go#newcode81
environs/open.go:81: if err == configstore.ErrEnvironInfoAlreadyExists {
you should pop this out of the outer if statement, since it doesn't rely on it,
and always returns once you enter it. It would make the code here more clear.

https://codereview.appspot.com/13573046/diff/5001/environs/open.go#newcode103
environs/open.go:103: if err != nil {
I think this would be more clear if you combined the two lines... if err :=
info.Write(); err != nil {   is really standard.

[rog, 2013-09-25 09:10:29]

forgot to publish this

https://codereview.appspot.com/13573046/diff/5001/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/13573046/diff/5001/environs/open.go#newcode88
environs/open.go:88: return nil, fmt.Errorf("found uninitialized environment
info for %q; environment preparation probably in progress or interrupted",
config.Name())
On 2013/09/24 16:59:11, fwereade wrote:
> `juju destroy-environment` will resolve this situation, right?

yes.