Code review comment for lp:~axwalk/juju-core/lp1247152-destroy-jenv-failed-prepare

Reviewers: mp+205678_code.launchpad.net,

Message:
Please take a look.

Description:
environs: destroy jenv if prepare fails

If Prepare fails mid-way, the .jenv file
can be left behind in some cases.

Fixes lp:1247152

https://code.launchpad.net/~axwalk/juju-core/lp1247152-destroy-jenv-failed-prepare/+merge/205678

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/61470046/

Affected files (+20, -10 lines):
   A [revision details]
   M environs/open.go
   M environs/open_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140210125047-3525mlq3garclj3q
+New revision: <email address hidden>

Index: environs/open.go
=== modified file 'environs/open.go'
--- environs/open.go 2014-02-03 14:31:54 +0000
+++ environs/open.go 2014-02-11 02:50:17 +0000
@@ -175,15 +175,7 @@
   if err != nil {
    return nil, fmt.Errorf("cannot create new info for environment %q: %v",
cfg.Name(), err)
   }
- cfg, err = ensureAdminSecret(cfg)
- if err != nil {
- return nil, fmt.Errorf("cannot generate admin-secret: %v", err)
- }
- cfg, err = ensureCertificate(cfg)
- if err != nil {
- return nil, fmt.Errorf("cannot ensure CA certificate: %v", err)
- }
- env, err := p.Prepare(cfg)
+ env, err := prepare(cfg, info, p)
   if err != nil {
    if err := info.Destroy(); err != nil {
     logger.Warningf("cannot destroy newly created environment info: %v",
err)
@@ -197,6 +189,18 @@
   return env, nil
  }

+func prepare(cfg *config.Config, info configstore.EnvironInfo, p
EnvironProvider) (Environ, error) {
+ cfg, err := ensureAdminSecret(cfg)
+ if err != nil {
+ return nil, fmt.Errorf("cannot generate admin-secret: %v", err)
+ }
+ cfg, err = ensureCertificate(cfg)
+ if err != nil {
+ return nil, fmt.Errorf("cannot ensure CA certificate: %v", err)
+ }
+ return p.Prepare(cfg)
+}
+
  // ensureAdminSecret returns a config with a non-empty admin-secret.
  func ensureAdminSecret(cfg *config.Config) (*config.Config, error) {
   if cfg.AdminSecret() != "" {

Index: environs/open_test.go
=== modified file 'environs/open_test.go'
--- environs/open_test.go 2013-12-20 02:38:56 +0000
+++ environs/open_test.go 2014-02-11 02:50:17 +0000
@@ -245,9 +245,13 @@
    },
   ))
   c.Assert(err, gc.IsNil)
- env, err := environs.Prepare(cfg, configstore.NewMem())
+ store := configstore.NewMem()
+ env, err := environs.Prepare(cfg, store)
   c.Assert(err, gc.ErrorMatches, "cannot ensure CA certificate: environment
configuration with a certificate but no CA private key")
   c.Assert(env, gc.IsNil)
+ // Ensure that the config storage info is cleaned up.
+ _, err = store.ReadInfo(cfg.Name())
+ c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
  }

  func (*OpenSuite) TestPrepareWithExistingKeyPair(c *gc.C) {