Merge lp:~natefinch/juju-core/juju-core into lp:~go-bot/juju-core/trunk

[Nate Finch]

Show same error for no default env

When you run juju with no default config file, it should give the same error regardless of the command you're running. To do that, I have ReadEnviron wrap the file not found error it used to return in a new type, so that further up the stack we can check for it, and notify the user appropriately.

https://codereview.appspot.com/12347044/

[Nate Finch (natefinch)]

Note, this is a proposed fix for https://bugs.launchpad.net/juju-core/+bug/1154938 "juju status should give the same help as juju bootstrap"

[Dimiter Naydenov (dimitern)]

LGTM, apart from a few trivial suggestions below

https://codereview.appspot.com/12347044/diff/1/cmd/juju/init.go
File cmd/juju/init.go (right):

https://codereview.appspot.com/12347044/diff/1/cmd/juju/init.go#newcode8
cmd/juju/init.go:8: "launchpad.net/gnuflag"
While you're at it, please reformat the imports here:

"fmt"

"launchpad.net/gnuflag"

"launchpad.net/juju-core/..."

We accepted this style of formatting imports some time ago and we should
take the time to fix old code to use the new convention: 1) standard
pacakges, 2) third-parties, 3) juju-core stuff, each group separated
with a blank line from the previous.

https://codereview.appspot.com/12347044/diff/1/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/12347044/diff/1/environs/config.go#newcode141
environs/config.go:141: if path == "" && os.IsNotExist(err) {
I don't think you need to check path == "" here, because environsPath
will actually handle that and set it to the default.

https://codereview.appspot.com/12347044/diff/1/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/12347044/diff/1/errors/errors.go#newcode68
errors/errors.go:68: // NoEnvError indicates the default environment
config file is missing
s/missing/missing./

https://codereview.appspot.com/12347044/diff/1/errors/errors.go#newcode73
errors/errors.go:73: // IsNoEnv returns true if err is a NoEnvError
By convention, such functions we usually document as "F returns
if/whether/etc ...", no need to say it returns true. Also there should
be a full stop at the end.

https://codereview.appspot.com/12347044/

[Nate Finch (natefinch)]

Reviewers: mp+178288_code.launchpad.net, dimitern,

Message:
Fixed dimitern's suggestions, also took rogpeppe's suggestions to remove
the changes to cmd.go and put the error check in each command instead.

Description:
Show same error for no default env

When you run juju with no default config file, it should give the same
error regardless of the command you're running. To do that, I have
ReadEnviron wrap the file not found error it used to return in a new
type, so that further up the stack we can check for it, and notify the
user appropriately.

https://code.launchpad.net/~natefinch/juju-core/juju-core/+merge/178288

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/juju/addmachine.go
   M cmd/juju/addrelation.go
   M cmd/juju/addunit.go
   M cmd/juju/bootstrap.go
   M cmd/juju/constraints.go
   M cmd/juju/deploy.go
   M cmd/juju/destroyenvironment.go
   M cmd/juju/destroymachine.go
   M cmd/juju/destroyrelation.go
   M cmd/juju/destroyservice.go
   M cmd/juju/destroyunit.go
   M cmd/juju/environment.go
   M cmd/juju/environmentcommand.go
   M cmd/juju/expose.go
   M cmd/juju/get.go
   M cmd/juju/init.go
   M cmd/juju/publish.go
   M cmd/juju/resolved.go
   M cmd/juju/scp.go
   M cmd/juju/set.go
   M cmd/juju/ssh.go
   M cmd/juju/status.go
   M cmd/juju/unexpose.go
   M cmd/juju/upgradecharm.go
   M cmd/juju/upgradejuju.go
   M environs/config.go
   M errors/errors.go

[John A Meinel (jameinel)]

Before you get too far, changing all the various places that import should probably be done in a different branch (so it is easier to review).

The other change is that:

 gc "launchpad.net/gocheck"

Is the "standard" way to import gocheck now, because otherwise 'gocheck' everywhere ends up being a bit too verbose.

[Nate Finch (natefinch)]

Thanks, I talked to Roger and Dimiter about that on irc just now, too. I'll
change these places over to gc, but definitely should be a separate branch
for the more sweeping changes.

On Thu, Aug 8, 2013 at 1:55 PM, John A Meinel <email address hidden>wrote:

> Before you get too far, changing all the various places that import should
> probably be done in a different branch (so it is easier to review).
>
> The other change is that:
>
> gc "launchpad.net/gocheck"
>
> Is the "standard" way to import gocheck now, because otherwise 'gocheck'
> everywhere ends up being a bit too verbose.
>
> --
> https://code.launchpad.net/~natefinch/juju-core/juju-core/+merge/178288
> You are the owner of lp:~natefinch/juju-core/juju-core.
>

[Nate Finch (natefinch)]

Please take a look.

https://codereview.appspot.com/12347044/

[Nate Finch (natefinch)]

Please take a look.

https://codereview.appspot.com/12347044/

[Nate Finch (natefinch)]

Please take a look.

https://codereview.appspot.com/12347044/

[Nate Finch (natefinch)]

Please take a look.

https://codereview.appspot.com/12347044/

=== modified file 'juju/api.go'
--- juju/api.go	2014-04-22 15:00:21 +0000
+++ juju/api.go	2014-04-24 19:52:39 +0000
@@ -193,7 +193,15 @@
 		logger.Debugf("no cached API connection settings found")
 	}
 	try.Start(func(stop <-chan struct{}) (io.Closer, error) {
+<<<<<<< TREE
 		return apiConfigConnect(info, envs, envName, apiOpen, stop, delay)
+=======
+		cfg, err := getConfig(info, envs, envName)
+		if err != nil {
+			return nil, err
+		}
+		return apiConfigConnect(cfg, stop, delay)
+>>>>>>> MERGE-SOURCE
 	})
 	try.Close()
 	val0, err := try.Result()
@@ -280,6 +288,7 @@
 // its endpoint. It only starts the attempt after the given delay,
 // to allow the faster apiInfoConnect to hopefully succeed first.
 // It returns nil if there was no configuration information found.
+<<<<<<< TREE
 func apiConfigConnect(info configstore.EnvironInfo, envs *environs.Environs, envName string, apiOpen apiOpenFunc, stop <-chan struct{}, delay time.Duration) (apiState, error) {
 	var cfg *config.Config
 	var err error
@@ -293,6 +302,9 @@
 	} else {
 		return nil, errors.NotFoundf("environment %q", envName)
 	}
+=======
+func apiConfigConnect(cfg *config.Config, stop <-chan struct{}, delay time.Duration) (apiState, error) {
+>>>>>>> MERGE-SOURCE
 	select {
 	case <-time.After(delay):
 	case <-stop:
@@ -314,6 +326,25 @@
 	return apiStateCachedInfo{st, apiInfo}, nil
 }
 
+// getConfig looks for configuration info on the given environment
+func getConfig(info configstore.EnvironInfo, envs *environs.Environs, envName string) (*config.Config, error) {
+	if info != nil && len(info.BootstrapConfig()) > 0 {
+		cfg, err := config.New(config.NoDefaults, info.BootstrapConfig())
+		if err != nil {
+			logger.Warningf("failed to parse bootstrap-config: %v", err)
+		}
+		return cfg, err
+	}
+	if envs != nil {
+		cfg, err := envs.Config(envName)
+		if err != nil && !errors.IsNotFoundError(err) {
+			logger.Warningf("failed to get config for environment %q: %v", envName, err)
+		}
+		return cfg, err
+	}
+	return nil, errors.NotFoundf("environment %q", envName)
+}
+
 func environAPIInfo(environ environs.Environ) (*api.Info, error) {
 	_, info, err := environ.StateInfo()
 	if err != nil {

=== modified file 'juju/apiconn_test.go'
--- juju/apiconn_test.go	2014-04-24 02:27:38 +0000
+++ juju/apiconn_test.go	2014-04-24 19:52:39 +0000
@@ -299,6 +299,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) {
+	badInfo := &badBootstrapInfo{}
+	cfg, err := juju.GetConfig(badInfo, nil, "test")
+	// 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")
+	c.Assert(cfg, gc.IsNil)
+}
+
 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-04-16 08:07:27 +0000
+++ juju/export_test.go	2014-04-24 19:52:39 +0000
@@ -7,6 +7,11 @@
 
 var (
 	ProviderConnectDelay = &providerConnectDelay
+<<<<<<< TREE
+=======
+	NewAPIFromStore      = newAPIFromStore
+	GetConfig            = getConfig
+>>>>>>> MERGE-SOURCE
 )
 
 type APIState apiState

=== modified file 'scripts/win-installer/setup.iss'
--- scripts/win-installer/setup.iss	2014-04-15 17:15:47 +0000
+++ scripts/win-installer/setup.iss	2014-04-24 19:52:39 +0000
@@ -2,7 +2,11 @@
 ; SEE THE DOCUMENTATION FOR DETAILS ON CREATING INNO SETUP SCRIPT FILES!
 
 #define MyAppName "Juju"
+<<<<<<< TREE
 #define MyAppVersion "1.19.1"
+=======
+#define MyAppVersion "1.18.2"
+>>>>>>> MERGE-SOURCE
 #define MyAppPublisher "Canonical, Ltd"
 #define MyAppURL "http://juju.ubuntu.com/"
 #define MyAppExeName "juju.exe"

=== modified file 'version/version.go'
--- version/version.go	2014-04-15 17:15:47 +0000
+++ version/version.go	2014-04-24 19:52:39 +0000
@@ -23,7 +23,11 @@
 // The presence and format of this constant is very important.
 // The debian/rules build recipe uses this value for the version
 // number of the release package.
+<<<<<<< TREE
 const version = "1.19.1"
+=======
+const version = "1.18.2"
+>>>>>>> MERGE-SOURCE
 
 // lsbReleaseFile is the name of the file that is read in order to determine
 // the release version of ubuntu.