Merge lp:~jameinel/juju-core/1.18-panic-parsing-jenv-1312136 into lp:juju-core/1.18

[John A Meinel]

juju/api.go: actually handle error (bug #1312136)

If there is a problem reading the .jenv file, it can return an error,
but we weren't paying attention to it. Instead we would end up getting a
panic() a few lines down when we go to use a *Config that is actually
nil.

This may not be perfect (because I wasn't seeing errors as I would
expect), but it is better than what we have.

https://codereview.appspot.com/92750043/

[John A Meinel (jameinel)]

Reviewers: mp+217036_code.launchpad.net,

Message:
Please take a look.

Description:
juju/api.go: actually handle error (bug #1312136)

If there is a problem reading the .jenv file, it can return an error,
but we weren't paying attention to it. Instead we would end up getting a
panic() a few lines down when we go to use a *Config that is actually
nil.

This may not be perfect (because I wasn't seeing errors as I would
expect), but it is better than what we have.

https://code.launchpad.net/~jameinel/juju-core/1.18-panic-parsing-jenv-1312136/+merge/217036

(do not edit description out of merge proposal)

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

Affected files (+6, -0 lines):
   A [revision details]
   M juju/api.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-20140412095413-8rps4bhva01cu0o9
+New revision: <email address hidden>

Index: juju/api.go
=== modified file 'juju/api.go'
--- juju/api.go 2014-03-19 03:18:41 +0000
+++ juju/api.go 2014-04-24 12:13:52 +0000
@@ -259,6 +259,10 @@
   var err error
   if info != nil && len(info.BootstrapConfig()) > 0 {
    cfg, err = config.New(config.NoDefaults, info.BootstrapConfig())
+ if err != nil {
+ logger.Debugf("failed to parse BootstrapConfig(): %v", err)
+ return apiState{}, err
+ }
   } else if envs != nil {
    cfg, err = envs.Config(envName)
    if errors.IsNotFoundError(err) {

[John A Meinel (jameinel)]

Please take a look.

https://codereview.appspot.com/92750043/

[Wayne Witzel III (wwitzel3)]

LGTM

https://codereview.appspot.com/92750043/

[Wayne Witzel III (wwitzel3)]

Question

https://codereview.appspot.com/92750043/diff/20001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/92750043/diff/20001/juju/api.go#newcode258
juju/api.go:258: var cfg *config.Config
What is the benefit of declaring these here? Instead of using := in the
config.New and envs.Config calls? It seems that declaring these here is
what prevents this being a compile time error (for unused err) instead
of a run time error for an unhandled panic?

https://codereview.appspot.com/92750043/

[John A Meinel (jameinel)]

Please take a look.

https://codereview.appspot.com/92750043/

[John A Meinel (jameinel)]

Please take a look.

https://codereview.appspot.com/92750043/

[Wayne Witzel III (wwitzel3)]

SGTM

https://codereview.appspot.com/92750043/

=== modified file 'juju/api.go'
--- juju/api.go	2014-03-19 03:18:41 +0000
+++ juju/api.go	2014-04-24 14:05:34 +0000
@@ -256,10 +256,15 @@
 // It returns nil if there was no configuration information found.
 func apiConfigConnect(info configstore.EnvironInfo, envs *environs.Environs, envName string, stop <-chan struct{}, delay time.Duration) (apiState, error) {
 	var cfg *config.Config
-	var err error
 	if info != nil && len(info.BootstrapConfig()) > 0 {
+		var err error
 		cfg, err = config.New(config.NoDefaults, info.BootstrapConfig())
+		if err != nil {
+			logger.Warningf("failed to parse bootstrap-config: %v", err)
+			return apiState{}, err
+		}
 	} else if envs != nil {
+		var err error
 		cfg, err = envs.Config(envName)
 		if errors.IsNotFoundError(err) {
 			return apiState{}, err

=== modified file 'juju/apiconn_test.go'
--- juju/apiconn_test.go	2014-04-03 19:11:11 +0000
+++ juju/apiconn_test.go	2014-04-24 14:05:34 +0000
@@ -300,6 +300,26 @@
 	}
 }
 
+type badBootstrapInfo struct {
+	configstore.EnvironInfo
+}
+
+// BootstrapConfig is returned as a map with real content, but the content
+// isn't actually valid configuration, causing config.New to fail
+func (m *badBootstrapInfo) BootstrapConfig() map[string]interface{} {
+	return map[string]interface{}{"something": "else"}
+}
+
+func (s *NewAPIClientSuite) TestBadConfigDoesntPanic(c *gc.C) {
+	stop := make(chan struct{})
+	badInfo := &badBootstrapInfo{}
+	_, err := juju.APIConfigConnect(badInfo, nil, "test", stop, 0)
+	// The specific error we get depends on what key is invalid, which is a
+	// bit spurious, but what we care about is that we didn't get a panic,
+	// but instead got an error
+	c.Assert(err, gc.ErrorMatches, ".*expected.*got nothing")
+}
+
 func setEndpointAddress(c *gc.C, store configstore.Storage, envName string, addr string) {
 	// Populate the environment's info with an endpoint
 	// with a known address.

=== modified file 'juju/export_test.go'
--- juju/export_test.go	2014-03-19 03:18:41 +0000
+++ juju/export_test.go	2014-04-24 14:05:34 +0000
@@ -5,4 +5,5 @@
 	APIClose             = &apiClose
 	ProviderConnectDelay = &providerConnectDelay
 	NewAPIFromStore      = newAPIFromStore
+	APIConfigConnect     = apiConfigConnect
 )