Merge lp:~dave-cheney/juju-core/080-conn-update-secrets-only-if-needed into lp:~juju/juju-core/trunk

[Dave Cheney]

juju: update unknown config attrs once

Additional UnknownAttrs from the local environment
will only be applied if they do not already exist in
the state.

https://codereview.appspot.com/6497057/

[Dave Cheney (dave-cheney)]

Please hold off on reviewing this proposal. Tests pass, but an actual deployment fails because the common attributes of the config are not being populated.

[Gustavo Niemeyer (niemeyer)]

http://codereview.appspot.com/6497057/diff/3001/juju/conn.go
File juju/conn.go (right):

http://codereview.appspot.com/6497057/diff/3001/juju/conn.go#newcode92
juju/conn.go:92: env.Set(k, v)
Aug 30 22:15:25 <niemeyer> I suspect it's a bit more involved than
that
Aug 30 22:15:52 <niemeyer> As the comment suggests, we should only
push secrets when they are not yet set, and should push *only* secrets

http://codereview.appspot.com/6497057/

[Gustavo Niemeyer (niemeyer)]

Cool, I added some comments for the record, and.. well, not sure if we
should close this one?

http://codereview.appspot.com/6497057/

[Gustavo Niemeyer (niemeyer)]

Ah, nevermind.. it looks already rejected. I probably looked at
out-of-date information, sorry.

http://codereview.appspot.com/6497057/

Unmerged revisions

452. By Dave Cheney on 2012-08-31

merge from prereq

451. By Dave Cheney on 2012-08-31

wip

450. By Dave Cheney on 2012-08-30

only update non existing UnknownAttrs

449. By Dave Cheney on 2012-08-30

merge from trunk

448. By Dave Cheney on 2012-08-30

wip

447. By Dave Cheney on 2012-08-29

environs/dummy: add delay

This proposal implements a delay in most dummy operations. The delay
can be set test wide by setting JUJU_DUMMY_DELAY to a time.Duration
parsable value.

R=niemeyer
CC=
https://codereview.appspot.com/6482081

446. By Dave Cheney on 2012-08-29

environs/ec2: log open/close port details

https://bugs.launchpad.net/juju-core/+bug/1042073

Added logging of security group name to successful
open/close port operation.

R=niemeyer
CC=
https://codereview.appspot.com/6490049

445. By Dave Cheney on 2012-08-29

environs/cloudinit: deploy mongo debs

Install mongo debs alongside zookeeper ones.

R=niemeyer
CC=
https://codereview.appspot.com/6501054

444. By Dave Cheney on 2012-08-29

environs/cloudinit: ZooKeeper -> StateServer

This proposal is part of a set to add support for deploying
an environment on MongoDB in parallel with ZooKeeper.

R=niemeyer
CC=
https://codereview.appspot.com/6497051

443. By Roger Peppe on 2012-08-29

testing: define and use all fixture methods.

When a helper suite defines only a subset of the available
fixture methods, it invites problems if that subset changes.
By making all helper suites define all fixture methods,
we make it easy to mechanically check that
a helper suite is being used correctly without going
to look at the source for that suite.

R=niemeyer
CC=
https://codereview.appspot.com/6472053

=== modified file 'cmd/juju/cmd_test.go'
--- cmd/juju/cmd_test.go	2012-08-28 17:33:59 +0000
+++ cmd/juju/cmd_test.go	2012-08-31 02:04:18 +0000
@@ -149,7 +149,7 @@
 	}()
 	environs.VarDir = c.MkDir()
 
-	tools, err := environs.FindTools(env, version.Current)
+	tools, err := environs.FindTools(env, version.Current, environs.CompatVersion)
 	c.Assert(err, IsNil)
 	resp, err := http.Get(tools.URL)
 	c.Assert(err, IsNil)

=== modified file 'cmd/juju/main_test.go'
--- cmd/juju/main_test.go	2012-08-09 01:37:27 +0000
+++ cmd/juju/main_test.go	2012-08-31 02:04:18 +0000
@@ -121,7 +121,7 @@
 	c.Assert(out, Equals, "error: "+msg+"\n")
 	content, err := ioutil.ReadFile(logpath)
 	c.Assert(err, IsNil)
-	fullmsg := fmt.Sprintf(`.* JUJU:DEBUG juju bootstrap command failed: %s\n`, msg)
+	fullmsg := fmt.Sprintf(`(.|\n)*JUJU:DEBUG juju bootstrap command failed: %s\n`, msg)
 	c.Assert(string(content), Matches, fullmsg)
 }
 
@@ -134,7 +134,7 @@
 	c.Assert(out, Equals, "error: "+msg+"\n")
 	content, err := ioutil.ReadFile(logpath)
 	c.Assert(err, IsNil)
-	fullmsg := fmt.Sprintf(`.* JUJU:DEBUG juju bootstrap command failed: %s\n`, msg)
+	fullmsg := fmt.Sprintf(`(.|\n)*JUJU:DEBUG juju bootstrap command failed: %s\n`, msg)
 	c.Assert(string(content), Matches, fullmsg)
 }
 

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2012-08-29 22:46:46 +0000
+++ environs/cloudinit/cloudinit.go	2012-08-31 02:04:18 +0000
@@ -6,7 +6,7 @@
 	"launchpad.net/goyaml"
 	"launchpad.net/juju-core/cloudinit"
 	"launchpad.net/juju-core/environs"
-	"launchpad.net/juju-core/log"
+	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/upstart"
 	"path"
@@ -56,9 +56,8 @@
 	// commands cannot work.
 	AuthorizedKeys string
 
-	// Config is map that is provided to juju bootstrap-state's --env-config
-	// option for initializing the environment configuration.
-	Config map[string]interface{}
+	// Config holds the initial environment configuration.
+	Config *config.Config
 }
 
 type requiresError string
@@ -73,8 +72,8 @@
 	}
 }
 
-func base64yaml(m map[string]interface{}) string {
-	data, err := goyaml.Marshal(m)
+func base64yaml(m *config.Config) string {
+	data, err := goyaml.Marshal(m.AllAttrs())
 	if err != nil {
 		// can't happen, these values have been validated a number of times
 		panic(err)
@@ -119,9 +118,7 @@
 	)
 
 	debugFlag := ""
-	if log.Debug {
 		debugFlag = " --debug"
-	}
 
 	if cfg.StateServer {
 		// zookeeper scripts
@@ -223,6 +220,9 @@
 		if cfg.InstanceIdAccessor == "" {
 			return requiresError("instance id accessor")
 		}
+		if cfg.Config == nil {
+			return requiresError("environment configuration")
+		}
 	} else {
 		if cfg.StateInfo == nil || len(cfg.StateInfo.Addrs) == 0 {
 			return requiresError("zookeeper hosts")

=== modified file 'environs/cloudinit/cloudinit_test.go'
--- environs/cloudinit/cloudinit_test.go	2012-08-29 22:50:49 +0000
+++ environs/cloudinit/cloudinit_test.go	2012-08-31 02:04:18 +0000
@@ -4,6 +4,7 @@
 	"encoding/base64"
 	. "launchpad.net/gocheck"
 	"launchpad.net/goyaml"
+	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/cloudinit"
 	"launchpad.net/juju-core/state"
@@ -18,6 +19,21 @@
 
 var _ = Suite(cloudinitSuite{})
 
+var envConfig = mustNewConfig(map[string] interface{} {
+		"type": "ec2",
+		"name": "foo",
+		"default-series": "series",
+		"authorized-keys": "keys",
+	})
+
+func mustNewConfig(m map[string] interface{}) *config.Config {
+	cfg, err := config.New(m)
+	if err != nil {
+		panic(err)
+	}
+	return cfg
+}
+
 // Each test gives a cloudinit config - we check the
 // output to see if it looks correct.
 var cloudinitTests = []cloudinit.MachineConfig{
@@ -29,7 +45,7 @@
 		AuthorizedKeys:     "sshkey1",
 		Tools:              newSimpleTools("1.2.3-linux-amd64"),
 		StateServer:        true,
-		Config:             map[string]interface{}{"name": "foo", "zookeeper": true},
+		Config:             envConfig,
 	},
 	{
 		MachineId:      99,
@@ -65,15 +81,16 @@
 	if t.cfg.StateServer {
 		// TODO(dfc) remove this after the switch to mstate
 		t.checkPackage(c, "zookeeperd")
-
 		t.checkPackage(c, "mongodb-server")
 		t.checkScripts(c, "jujud bootstrap-state")
-		t.checkEnvConfig(c)
 		t.checkScripts(c, regexp.QuoteMeta(t.cfg.InstanceIdAccessor))
 		t.checkScripts(c, "JUJU_ZOOKEEPER='localhost"+cloudinit.ZkPortSuffix+"'")
 	} else {
 		t.checkScripts(c, "JUJU_ZOOKEEPER='"+strings.Join(t.cfg.StateInfo.Addrs, ",")+"'")
 	}
+	if t.cfg.Config != nil {
+		t.checkEnvConfig(c)
+	}
 	t.checkPackage(c, "libzookeeper-mt2")
 	t.checkScripts(c, "JUJU_MACHINE_ID=[0-9]+")
 
@@ -96,20 +113,20 @@
 		return
 	}
 	scripts := scripts0.([]interface{})
-	re := regexp.MustCompile(`--env-config '[\w,=]+'`)
+	re := regexp.MustCompile(`--env-config '([\w,=]+)'`)
 	found := false
 	for _, s0 := range scripts {
-		if s := re.FindString(s0.(string)); len(s) > 0 {
-			found = true
-			v := strings.Split(s, `'`)
-			c.Assert(len(v), Equals, 3)
-			buf, err := base64.StdEncoding.DecodeString(v[1])
-			c.Assert(err, IsNil)
-			actual := make(map[string]interface{})
-			err = goyaml.Unmarshal(buf, &actual)
-			c.Assert(err, IsNil)
-			c.Assert(t.cfg.Config, DeepEquals, actual)
+		m := re.FindStringSubmatch(s0.(string))
+		if m == nil {
+			continue
 		}
+		found = true
+		buf, err := base64.StdEncoding.DecodeString(m[1])
+		c.Assert(err, IsNil)
+		var actual map[string]interface{}
+		err = goyaml.Unmarshal(buf, &actual)
+		c.Assert(err, IsNil)
+		c.Assert(t.cfg.Config.AllAttrs(), DeepEquals, actual)
 	}
 	c.Assert(found, Equals, true)
 }
@@ -182,7 +199,7 @@
 // in cloudinitTests is well formed.
 func (cloudinitSuite) TestCloudInit(c *C) {
 	for i, cfg := range cloudinitTests {
-		c.Logf("check %d", i)
+		c.Logf("test %d, config %v, envConfig %v", i, cfg.Config, envConfig)
 		ci, err := cloudinit.New(&cfg)
 		c.Assert(err, IsNil)
 		c.Check(ci, NotNil)
@@ -217,9 +234,11 @@
 	{"negative machine id", func(cfg *cloudinit.MachineConfig) { cfg.MachineId = -1 }},
 	{"missing provider type", func(cfg *cloudinit.MachineConfig) { cfg.ProviderType = "" }},
 	{"missing instance id accessor", func(cfg *cloudinit.MachineConfig) {
-		cfg.StateServer = true
 		cfg.InstanceIdAccessor = ""
 	}},
+	{"missing environment configuration", func(cfg *cloudinit.MachineConfig) {
+		cfg.Config = nil
+	}},
 	{"missing zookeeper hosts", func(cfg *cloudinit.MachineConfig) {
 		cfg.StateServer = false
 		cfg.StateInfo = nil
@@ -250,6 +269,7 @@
 		Tools:              newSimpleTools("9.9.9-linux-arble"),
 		AuthorizedKeys:     "sshkey1",
 		StateInfo:          &state.Info{Addrs: []string{"zkhost"}},
+		Config: envConfig,
 	}
 	// check that the base configuration does not give an error
 	_, err := cloudinit.New(cfg)

=== modified file 'environs/config.go'
--- environs/config.go	2012-07-26 13:37:51 +0000
+++ environs/config.go	2012-08-31 02:04:18 +0000
@@ -6,6 +6,7 @@
 	"io/ioutil"
 	"launchpad.net/goyaml"
 	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/state"
 	"os"
 	"path/filepath"
 )
@@ -138,3 +139,21 @@
 	}
 	return e, nil
 }
+
+// BootstrapConfig returns an environment configuration suitable for
+// priming the juju state using the given provider, configuration and
+// tools.
+// 
+// The returned configuration contains no secret attributes.
+func BootstrapConfig(p EnvironProvider, cfg *config.Config, tools *state.Tools) (*config.Config, error) {
+	secrets, err := p.SecretAttrs(cfg)
+	if err != nil {
+		return nil, err
+	}
+	m := cfg.AllAttrs()
+	for k, _ := range secrets {
+		delete(m, k)
+	}
+	m["agent-version"] = tools.Number.String()
+	return config.New(m)
+}

=== modified file 'environs/config/config.go'
--- environs/config/config.go	2012-08-30 12:21:22 +0000
+++ environs/config/config.go	2012-08-31 02:04:18 +0000
@@ -15,6 +15,8 @@
 // Fields that are common to all environment providers are verified,
 // and authorized-keys-path is also translated into authorized-keys
 // by loading the content from respective file.
+//
+// The required keys are: "name", "type", "default-series" and "authorized-keys".
 func New(attrs map[string]interface{}) (*Config, error) {
 	m, err := checker.Coerce(attrs, nil)
 	if err != nil {
@@ -63,7 +65,7 @@
 	return c, nil
 }
 
-// Type returns the enviornment type.
+// Type returns the environment type.
 func (c *Config) Type() string {
 	return c.m["type"].(string)
 }

=== modified file 'environs/dummy/environs.go'
--- environs/dummy/environs.go	2012-08-30 02:34:58 +0000
+++ environs/dummy/environs.go	2012-08-31 02:04:18 +0000
@@ -29,6 +29,7 @@
 	"launchpad.net/juju-core/schema"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/version"
 	"net"
 	"net/http"
 	"os"
@@ -179,10 +180,24 @@
 	}
 	s.storage = newStorage(s, "/"+name+"/private")
 	s.publicStorage = newStorage(s, "/"+name+"/public")
+	putFakeTools(s.publicStorage)
 	s.listen()
 	return s
 }
 
+// putFakeTools writes something
+// that looks like a tools archive so Bootstrap can
+// find some tools and initialise the state correctly.
+func putFakeTools(s environs.StorageWriter) {
+	log.Printf("putting fake tools")
+	path := environs.ToolsStoragePath(version.Current)
+	toolsContents := "tools archive, honest guv"
+	err := s.Put(path, strings.NewReader(toolsContents), int64(len(toolsContents)))
+	if err != nil {
+		panic(err)
+	}
+}
+
 // listen starts a network listener listening for http
 // requests to retrieve files in the state's storage.
 func (s *environState) listen() {
@@ -340,8 +355,16 @@
 	if err := e.checkBroken("Bootstrap"); err != nil {
 		return err
 	}
+	var tools *state.Tools
+	var err error
 	if uploadTools {
-		_, err := environs.PutTools(e.Storage(), nil)
+		tools, err = environs.PutTools(e.Storage(), nil)
+		if err != nil {
+			return err
+		}
+	} else {
+		flags := environs.HighestVersion|environs.CompatVersion
+		tools, err = environs.FindTools(e, version.Current, flags)
 		if err != nil {
 			return err
 		}
@@ -354,13 +377,11 @@
 	}
 	if e.ecfg().zookeeper() {
 		info := stateInfo()
-		config := map[string]interface{}{
-			"type":            "dummy",
-			"zookeeper":       true,
-			"name":            e.ecfg().Name(),
-			"authorized-keys": e.ecfg().AuthorizedKeys(),
+		cfg, err := environs.BootstrapConfig(&providerInstance, e.ecfg().Config, tools)
+		if err != nil {
+			return err
 		}
-		st, err := state.Initialize(info, config)
+		st, err := state.Initialize(info, cfg.AllAttrs())
 		if err != nil {
 			panic(err)
 		}

=== modified file 'environs/ec2/ec2.go'
--- environs/ec2/ec2.go	2012-08-29 23:08:25 +0000
+++ environs/ec2/ec2.go	2012-08-31 02:04:18 +0000
@@ -123,25 +123,6 @@
 	return m, nil
 }
 
-func (environProvider) publicAttrs(cfg *config.Config) (map[string]interface{}, error) {
-	m := make(map[string]interface{})
-	ecfg, err := providerInstance.newConfig(cfg)
-	if err != nil {
-		return nil, err
-	}
-	for k, v := range ecfg.UnknownAttrs() {
-		m[k] = v
-	}
-	secret, err := providerInstance.SecretAttrs(cfg)
-	if err != nil {
-		return nil, err
-	}
-	for k, _ := range secret {
-		delete(m, k)
-	}
-	return m, nil
-}
-
 func (environProvider) PublicAddress() (string, error) {
 	return fetchMetadata("public-hostname")
 }
@@ -240,9 +221,14 @@
 		if err != nil {
 			return fmt.Errorf("cannot upload tools: %v", err)
 		}
+	} else {
+		flags := environs.HighestVersion|environs.CompatVersion
+		tools, err = environs.FindTools(e, version.Current, flags)
+		if err != nil {
+			return fmt.Errorf("cannot find tools: %v", err)
+		}
 	}
-
-	config, err := providerInstance.publicAttrs(e.Config())
+	config, err := environs.BootstrapConfig(providerInstance, e.Config(), tools)
 	if err != nil {
 		return fmt.Errorf("unable to determine inital configuration: %v", err)
 	}
@@ -311,7 +297,7 @@
 	return e.startInstance(machineId, info, tools, false, nil)
 }
 
-func (e *environ) userData(machineId int, info *state.Info, tools *state.Tools, master bool, config map[string]interface{}) ([]byte, error) {
+func (e *environ) userData(machineId int, info *state.Info, tools *state.Tools, master bool, config *config.Config) ([]byte, error) {
 	cfg := &cloudinit.MachineConfig{
 		Provisioner:        master,
 		StateServer:        master,
@@ -333,10 +319,11 @@
 // startInstance is the internal version of StartInstance, used by Bootstrap
 // as well as via StartInstance itself. If master is true, a bootstrap
 // instance will be started.
-func (e *environ) startInstance(machineId int, info *state.Info, tools *state.Tools, master bool, config map[string]interface{}) (environs.Instance, error) {
+func (e *environ) startInstance(machineId int, info *state.Info, tools *state.Tools, master bool, config *config.Config) (environs.Instance, error) {
 	if tools == nil {
 		var err error
-		tools, err = environs.FindTools(e, version.Current)
+		flags := environs.HighestVersion|environs.CompatVersion
+		tools, err = environs.FindTools(e, version.Current, flags)
 		if err != nil {
 			return nil, err
 		}

=== modified file 'environs/jujutest/test.go'
--- environs/jujutest/test.go	2012-07-06 05:32:25 +0000
+++ environs/jujutest/test.go	2012-08-31 02:04:18 +0000
@@ -4,6 +4,7 @@
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/testing"
 	"time"
 )
 
@@ -19,6 +20,7 @@
 // is opened once for each test, and some potentially expensive operations
 // may be executed.
 type Tests struct {
+	testing.LoggingSuite
 	Environs *environs.Environs
 	Name     string
 	Env      environs.Environ
@@ -32,25 +34,22 @@
 	return e
 }
 
-func (t *Tests) SetUpSuite(*C) {
-}
-
-func (t *Tests) TearDownSuite(*C) {
-}
-
 func (t *Tests) SetUpTest(c *C) {
+	t.LoggingSuite.SetUpTest(c)
 	t.Env = t.Open(c)
 }
 
-func (t *Tests) TearDownTest(*C) {
+func (t *Tests) TearDownTest(c *C) {
 	t.Env.Destroy(nil)
 	t.Env = nil
+	t.LoggingSuite.TearDownTest(c)
 }
 
 // LiveTests contains tests that are designed to run against a live server
 // (e.g. Amazon EC2).  The Environ is opened once only for all the tests
 // in the suite, stored in Env, and Destroyed after the suite has completed.
 type LiveTests struct {
+	testing.LoggingSuite
 	Environs *environs.Environs
 	Name     string
 	Env      environs.Environ
@@ -70,12 +69,20 @@
 }
 
 func (t *LiveTests) SetUpSuite(c *C) {
+	t.LoggingSuite.SetUpSuite(c)
 	e, err := t.Environs.Open(t.Name)
 	c.Assert(err, IsNil, Commentf("opening environ %q", t.Name))
 	c.Assert(e, NotNil)
 	t.Env = e
 }
 
+func (t *LiveTests) TearDownSuite(c *C) {
+	err := t.Env.Destroy(nil)
+	c.Check(err, IsNil)
+	t.Env = nil
+	t.LoggingSuite.TearDownSuite(c)
+}
+
 func (t *LiveTests) BootstrapOnce(c *C) {
 	if t.bootstrapped {
 		return
@@ -90,15 +97,3 @@
 	c.Assert(err, IsNil)
 	t.bootstrapped = false
 }
-
-func (t *LiveTests) TearDownSuite(c *C) {
-	err := t.Env.Destroy(nil)
-	c.Check(err, IsNil)
-	t.Env = nil
-}
-
-func (t *LiveTests) SetUpTest(*C) {
-}
-
-func (t *LiveTests) TearDownTest(*C) {
-}

=== modified file 'environs/tools.go'
--- environs/tools.go	2012-08-29 16:54:01 +0000
+++ environs/tools.go	2012-08-31 02:04:18 +0000
@@ -204,27 +204,29 @@
 
 // BestTools returns the most recent version
 // from the set of tools in the ToolsList that are
-// compatible with the given version, and with a version
-// number <= vers.Number, or nil if no such tools are found.
-// If dev is true, it will consider development versions of the tools
-// even if vers is not a development version.
-func BestTools(list *ToolsList, vers version.Binary, dev bool) *state.Tools {
-	if tools := bestTools(list.Private, vers, dev); tools != nil {
+// compatible with the given version, using flags
+// to determine possible candidates.
+// It returns nil if no such tools are found.
+func BestTools(list *ToolsList, vers version.Binary, flags ToolsSearchFlags) *state.Tools {
+	if flags&CompatVersion == 0 {
+		panic("CompatVersion not implemented")
+	}
+	if tools := bestTools(list.Private, vers, flags); tools != nil {
 		return tools
 	}
-	return bestTools(list.Public, vers, dev)
+	return bestTools(list.Public, vers, flags)
 }
 
 // bestTools is like BestTools but operates on a single list of tools.
-func bestTools(toolsList []*state.Tools, vers version.Binary, dev bool) *state.Tools {
+func bestTools(toolsList []*state.Tools, vers version.Binary, flags ToolsSearchFlags) *state.Tools {
 	var bestTools *state.Tools
-	allowDev := vers.IsDev() || dev
+	allowDev := vers.IsDev() || flags&DevVersion != 0
 	for _, t := range toolsList {
 		if t.Major != vers.Major ||
 			t.Series != vers.Series ||
 			t.Arch != vers.Arch ||
 			!allowDev && t.IsDev() ||
-			vers.Number.Less(t.Number) {
+			flags&HighestVersion == 0 && vers.Number.Less(t.Number) {
 			continue
 		}
 		if bestTools == nil || bestTools.Number.Less(t.Number) {
@@ -379,19 +381,38 @@
 	return path.Join(VarDir, "tools", agentName)
 }
 
+// ToolsSearchFlags gives options when searching
+// for tools.
+type ToolsSearchFlags int
+const (
+	// HighestVersion specifies that the highest (minor, patch) version number should
+	// be chosen. The default is to choose a version no higher than specified.
+	HighestVersion ToolsSearchFlags = 1<<iota
+
+	// DevVersion specifies "dev mode" - it allows a development version to
+	// be chosen even when the specified version is not a development
+	// version.
+	DevVersion
+
+	// CompatVersion specifies that the major version number
+	// must be the same as specified. It is not yet implemented.
+	CompatVersion
+)
+
 // FindTools tries to find a set of tools compatible with the given
-// version from the given environment.  The latest version found with a
-// number <= vers.Number will be used.
+// version from the given environment, using flags to determine
+// possible candidates.
 // 
 // If no tools are found and there's no other error, a NotFoundError is
 // returned.  If there's anything compatible in the environ's Storage,
 // it gets precedence over anything in its PublicStorage.
-func FindTools(env Environ, vers version.Binary) (*state.Tools, error) {
+func FindTools(env Environ, vers version.Binary, flags ToolsSearchFlags) (*state.Tools, error) {
 	toolsList, err := ListTools(env, vers.Major)
 	if err != nil {
 		return nil, err
 	}
-	tools := BestTools(toolsList, vers, false)
+	log.Printf("findTools got tools list %v", toolsList)
+	tools := BestTools(toolsList, vers, flags)
 	if tools == nil {
 		return tools, &NotFoundError{fmt.Errorf("no compatible tools found")}
 	}

=== modified file 'environs/tools_test.go'
--- environs/tools_test.go	2012-08-29 16:54:01 +0000
+++ environs/tools_test.go	2012-08-31 02:04:18 +0000
@@ -384,6 +384,7 @@
 
 var findToolsTests = []struct {
 	version        version.Number // version to assume is current for the test.
+	flags environs.ToolsSearchFlags
 	contents       []string       // names in private storage.
 	publicContents []string       // names in public storage.
 	expect         string         // the name we expect to find (if no error).
@@ -391,35 +392,34 @@
 }{{
 	// current version should be satisfied by current tools path.
 	version:  version.Current.Number,
+	flags: environs.CompatVersion,
 	contents: []string{environs.ToolsStoragePath(version.Current)},
 	expect:   environs.ToolsStoragePath(version.Current),
 }, {
-	// major versions don't match.
-	version: version.MustParse("1.0.0"),
+	// highest version of tools is chosen.
+	version: version.MustParse("0.0.0"),
+	flags: environs.HighestVersion|environs.DevVersion|environs.CompatVersion,
 	contents: []string{
 		toolsStoragePath("0.0.9"),
-	},
-	err: "no compatible tools found",
-}, {
-	// major versions don't match.
-	version: version.MustParse("1.0.0"),
-	contents: []string{
-		toolsStoragePath("2.0.9"),
-	},
-	err: "no compatible tools found",
+		toolsStoragePath("0.1.9"),
+	},
+	expect: toolsStoragePath("0.1.9"),
 }, {
 	// fall back to public storage when nothing found in private.
-	version: version.MustParse("1.0.0"),
+	version: version.MustParse("1.0.2"),
+	flags: environs.DevVersion|environs.CompatVersion,
 	contents: []string{
 		toolsStoragePath("0.0.9"),
 	},
 	publicContents: []string{
 		toolsStoragePath("1.0.0"),
+		toolsStoragePath("1.0.1"),
 	},
-	expect: "public-" + toolsStoragePath("1.0.0"),
+	expect: "public-" + toolsStoragePath("1.0.1"),
 }, {
 	// always use private storage in preference to public storage.
 	version: version.MustParse("1.9.0"),
+	flags: environs.DevVersion|environs.CompatVersion,
 	contents: []string{
 		toolsStoragePath("1.0.2"),
 	},
@@ -428,43 +428,9 @@
 	},
 	expect: toolsStoragePath("1.0.2"),
 }, {
-	// we'll use an earlier version if the major version number matches.
-	version: version.MustParse("1.99.99"),
-	contents: []string{
-		toolsStoragePath("1.0.0"),
-	},
-	expect: toolsStoragePath("1.0.0"),
-}, {
-	// check that version comparing is numeric, not alphabetical.
-	version: version.MustParse("1.0.99"),
-	contents: []string{
-		toolsStoragePath("1.0.9"),
-		toolsStoragePath("1.0.10"),
-		toolsStoragePath("1.0.11"),
-	},
-	expect: toolsStoragePath("1.0.11"),
-}, {
-	// minor version wins over patch version.
-	version: version.MustParse("1.99.99"),
-	contents: []string{
-		toolsStoragePath("1.9.11"),
-		toolsStoragePath("1.10.10"),
-		toolsStoragePath("1.11.9"),
-	},
-	expect: toolsStoragePath("1.11.9"),
-}, {
-	// only earlier versions are chosen.
-	version: version.MustParse("1.10.9"),
-	contents: []string{
-		toolsStoragePath("1.9.10"),
-		toolsStoragePath("1.9.11"),
-		toolsStoragePath("1.10.10"),
-		toolsStoragePath("1.11.9"),
-	},
-	expect: toolsStoragePath("1.9.11"),
-}, {
 	// mismatching series or architecture is ignored.
 	version: version.MustParse("1.0.0"),
+	flags: environs.CompatVersion,
 	contents: []string{
 		environs.ToolsStoragePath(version.Binary{
 			Number: version.MustParse("1.9.9"),
@@ -508,10 +474,11 @@
 			Series: version.Current.Series,
 			Arch:   version.Current.Arch,
 		}
-		tools, err := environs.FindTools(t.env, vers)
+		tools, err := environs.FindTools(t.env, vers, tt.flags)
 		if tt.err != "" {
 			c.Assert(err, ErrorMatches, tt.err)
 		} else {
+			c.Assert(err, IsNil)
 			assertURLContents(c, tools.URL, tt.expect)
 		}
 		t.env.Destroy(nil)
@@ -613,15 +580,53 @@
 var bestToolsTests = []struct {
 	list   *environs.ToolsList
 	vers   version.Binary
-	dev    bool
+	flags    environs.ToolsSearchFlags
 	expect *state.Tools
+	expectDev *state.Tools
+	expectHighest *state.Tools
+	expectDevHighest *state.Tools
 }{{
-	&environs.ToolsList{},
-	binaryVersion("1.2.3-precise-amd64"),
-	true,
-	nil,
-}, {
-	&environs.ToolsList{
+	// 0. Check that we don't get anything from an empty list.
+	list: &environs.ToolsList{},
+	vers: binaryVersion("1.2.3-precise-amd64"),
+	flags: environs.DevVersion|environs.CompatVersion,
+	expect: nil,
+}, {
+	// 1. Check that we can choose the same development version.
+	list: &environs.ToolsList{
+		Private: []*state.Tools{
+			newTools("1.0.0-precise-amd64", ""),
+		},
+	},
+	vers: binaryVersion("1.0.0-precise-amd64"),
+	expect: newTools("1.0.0-precise-amd64", ""),
+	expectDev: newTools("1.0.0-precise-amd64", ""),
+	expectHighest: newTools("1.0.0-precise-amd64", ""),
+	expectDevHighest: newTools("1.0.0-precise-amd64", ""),
+}, {
+	// 2. Check that major versions need to match.
+	list: &environs.ToolsList{
+		Private: []*state.Tools{
+			newTools("2.0.0-precise-amd64", ""),
+			newTools("6.0.0-precise-amd64", ""),
+		},
+	},
+	vers: binaryVersion("4.0.0-precise-amd64"),
+}, {
+	// 3. Check that we can choose the same released version.
+	list: &environs.ToolsList{
+		Private: []*state.Tools{
+			newTools("2.0.0-precise-amd64", ""),
+		},
+	},
+	vers: binaryVersion("2.0.0-precise-amd64"),
+	expect: newTools("2.0.0-precise-amd64", ""),
+	expectDev: newTools("2.0.0-precise-amd64", ""),
+	expectHighest: newTools("2.0.0-precise-amd64", ""),
+	expectDevHighest: newTools("2.0.0-precise-amd64", ""),
+},  {
+	// 4. Check that different arch/series are ignored.
+	list: &environs.ToolsList{
 		Private: []*state.Tools{
 			newTools("1.2.3-precise-amd64", ""),
 			newTools("1.2.4-precise-amd64", ""),
@@ -631,29 +636,31 @@
 			newTools("2.2.3-precise-amd64", ""),
 		},
 	},
-	binaryVersion("1.9.4-precise-amd64"),
-	true,
-	newTools("1.3.4-precise-amd64", ""),
+	vers: binaryVersion("1.9.4-precise-amd64"),
+	expect: newTools("1.3.4-precise-amd64", ""),
+	expectDev: newTools("1.3.4-precise-amd64", ""),
+	expectHighest: newTools("1.3.4-precise-amd64", ""),
+	expectDevHighest: newTools("1.3.4-precise-amd64", ""),
 }, {
-	// Check that we can't upgrade to a dev version from
+	// 5. Check that we can't upgrade to a dev version from
 	// a release version if dev is false.
-	&environs.ToolsList{
+	list: &environs.ToolsList{
 		Private: []*state.Tools{
 			newTools("2.2.3-precise-amd64", ""),
 			newTools("2.2.4-precise-amd64", ""),
 			newTools("2.3.4-precise-amd64", ""),
-			newTools("2.4.4-precise-i386", ""),
-			newTools("2.4.5-quantal-i386", ""),
 			newTools("3.2.3-precise-amd64", ""),
 		},
 	},
-	binaryVersion("2.8.8-precise-amd64"),
-	false,
-	newTools("2.2.4-precise-amd64", ""),
+	vers: binaryVersion("2.8.8-precise-amd64"),
+	expect: newTools("2.2.4-precise-amd64", ""),
+	expectDev: newTools("2.3.4-precise-amd64", ""),
+	expectHighest: newTools("2.2.4-precise-amd64", ""),
+	expectDevHighest: newTools("2.3.4-precise-amd64", ""),
 }, {
-	// Check that we can upgrade to a release version from
+	// 6. Check that we can upgrade to a release version from
 	// a dev version if dev is false.
-	&environs.ToolsList{
+	list: &environs.ToolsList{
 		Private: []*state.Tools{
 			newTools("2.2.3-precise-amd64", ""),
 			newTools("2.2.4-precise-amd64", ""),
@@ -662,12 +669,14 @@
 			newTools("3.2.3-precise-amd64", ""),
 		},
 	},
-	binaryVersion("2.8.8-precise-amd64"),
-	false,
-	newTools("2.4.4-precise-amd64", ""),
+	vers: binaryVersion("2.8.8-precise-amd64"),
+	expect: newTools("2.4.4-precise-amd64", ""),
+	expectDev: newTools("2.4.4-precise-amd64", ""),
+	expectHighest: newTools("2.4.4-precise-amd64", ""),
+	expectDevHighest: newTools("2.4.4-precise-amd64", ""),
 }, {
-	// Check that a different major version works ok.
-	&environs.ToolsList{
+	// 7. Check that a different minor version works ok.
+	list: &environs.ToolsList{
 		Private: []*state.Tools{
 			newTools("1.2.3-precise-amd64", ""),
 			newTools("1.2.4-precise-amd64", ""),
@@ -677,54 +686,96 @@
 			newTools("2.2.3-precise-amd64", ""),
 		},
 	},
-	binaryVersion("2.8.8-precise-amd64"),
-	true,
-	newTools("2.2.3-precise-amd64", ""),
+	vers: binaryVersion("2.8.8-precise-amd64"),
+	expect: nil,
+	expectDev: newTools("2.2.3-precise-amd64", ""),
+	expectHighest: nil,
+	expectDevHighest: newTools("2.2.3-precise-amd64", ""),
 }, {
-	// Check that the private tools are chosen even though
+	// 8. Check that the private tools are chosen even though
 	// they have a lower version number.
-	&environs.ToolsList{
+	list: &environs.ToolsList{
 		Private: []*state.Tools{
-			newTools("1.2.3-precise-amd64", ""),
+			newTools("1.2.2-precise-amd64", ""),
 		},
 		Public: []*state.Tools{
 			newTools("1.2.4-precise-amd64", ""),
 		},
 	},
-	binaryVersion("1.8.8-precise-amd64"),
-	true,
-	newTools("1.2.3-precise-amd64", ""),
+	vers: binaryVersion("1.8.8-precise-amd64"),
+	expect: newTools("1.2.2-precise-amd64", ""),
+	expectDev: newTools("1.2.2-precise-amd64", ""),
+	expectHighest: newTools("1.2.2-precise-amd64", ""),
+	expectDevHighest: newTools("1.2.2-precise-amd64", ""),
 }, {
-	// Check that the public tools can be chosen when
+	// 9. Check that the public tools can be chosen when
 	// there are no private tools.
-	&environs.ToolsList{
+	list: &environs.ToolsList{
 		Public: []*state.Tools{
 			newTools("1.2.4-precise-amd64", ""),
 		},
 	},
-	binaryVersion("1.8.9-precise-amd64"),
-	true,
-	newTools("1.2.4-precise-amd64", ""),
-}, {
-	// Check that we don't choose a version later
-	// than requested.
-	&environs.ToolsList{
-		Public: []*state.Tools{
-			newTools("1.2.2-precise-amd64", ""),
-			newTools("1.2.3-precise-amd64", ""),
-			newTools("1.3.4-precise-amd64", ""),
-		},
-	},
-	binaryVersion("1.3.3-precise-amd64"),
-	true,
-	newTools("1.2.3-precise-amd64", ""),
+	vers: binaryVersion("1.8.9-precise-amd64"),
+	expect: newTools("1.2.4-precise-amd64", ""),
+	expectDev: newTools("1.2.4-precise-amd64", ""),
+	expectHighest: newTools("1.2.4-precise-amd64", ""),
+	expectDevHighest: newTools("1.2.4-precise-amd64", ""),
+}, {
+	// 10. One test giving different values for all flag combinations.
+	list: &environs.ToolsList{
+		Public: []*state.Tools{
+			newTools("0.2.0-precise-amd64", ""),
+			newTools("0.2.1-precise-amd64", ""),
+			newTools("0.4.2-precise-amd64", ""),
+			newTools("0.4.3-precise-amd64", ""),
+		},
+	},
+	vers: binaryVersion("0.2.2-precise-amd64"),
+	expect: newTools("0.2.0-precise-amd64", ""),
+	expectDev: newTools("0.2.1-precise-amd64", ""),
+	expectHighest: newTools("0.4.2-precise-amd64", ""),
+	expectDevHighest: newTools("0.4.3-precise-amd64", ""),
+}, {
+	// 11. check that version comparing is numeric, not alphabetical.
+	list: &environs.ToolsList{
+		Public: []*state.Tools{
+			newTools("0.0.9-precise-amd64", ""),
+			newTools("0.0.10-precise-amd64", ""),
+			newTools("0.0.11-precise-amd64", ""),
+		},
+	},
+	vers: binaryVersion("0.0.98-precise-amd64"),
+	expect: newTools("0.0.10-precise-amd64", ""),
+	expectDev: newTools("0.0.11-precise-amd64", ""),
+	expectHighest: newTools("0.0.10-precise-amd64", ""),
+	expectDevHighest: newTools("0.0.11-precise-amd64", ""),
+}, {
+	// 12. check that minor version wins over patch version.
+	list: &environs.ToolsList{
+		Public: []*state.Tools{
+			newTools("0.9.11-precise-amd64", ""),
+			newTools("0.10.10-precise-amd64", ""),
+			newTools("0.11.9-precise-amd64", ""),
+		},
+	},
+	vers: binaryVersion("0.10.10-precise-amd64"),
+	expect: newTools("0.10.10-precise-amd64", ""),
+	expectDev: newTools("0.10.10-precise-amd64", ""),
+	expectHighest: newTools("0.10.10-precise-amd64", ""),
+	expectDevHighest: newTools("0.11.9-precise-amd64", ""),
 },
 }
 
 func (t *ToolsSuite) TestBestTools(c *C) {
 	for i, t := range bestToolsTests {
 		c.Logf("test %d", i)
-		tools := environs.BestTools(t.list, t.vers, t.dev)
+		tools := environs.BestTools(t.list, t.vers, environs.CompatVersion)
 		c.Assert(tools, DeepEquals, t.expect)
+		tools = environs.BestTools(t.list, t.vers, environs.DevVersion|environs.CompatVersion)
+		c.Assert(tools, DeepEquals, t.expectDev)
+		tools = environs.BestTools(t.list, t.vers, environs.HighestVersion|environs.CompatVersion)
+		c.Assert(tools, DeepEquals, t.expectHighest)
+		tools = environs.BestTools(t.list, t.vers, environs.DevVersion|environs.HighestVersion|environs.CompatVersion)
+		c.Assert(tools, DeepEquals, t.expectDevHighest)
 	}
 }

=== modified file 'juju/conn.go'
--- juju/conn.go	2012-08-30 13:21:00 +0000
+++ juju/conn.go	2012-08-31 02:04:18 +0000
@@ -82,11 +82,31 @@
 // from the local configuration.
 func (c *Conn) updateSecrets() error {
 	cfg := c.Environ.Config()
+<<<<<<< TREE
 	// This is wrong. This will _always_ overwrite the secrets
 	// in the state with the local secrets. To fix this properly
 	// we need to ensure that the config, minus secrets, is always
 	// pushed on bootstrap, then we can fill in the secrets here.
 	return c.state.SetEnvironConfig(cfg)
+=======
+	env, err := c.state.EnvironConfig()
+	if err != nil {
+		return err
+	}
+	for k, v := range cfg.UnknownAttrs() {
+		if _, exists := env.Get(k); !exists {
+			env.Set(k, v)
+		}
+	}
+	n, err := env.Write()
+	if err != nil {
+		return err
+	}
+	if len(n) > 0 {
+		log.Debugf("Updating %d secret(s) in environment %q", len(n), c.Environ.Name())
+	}
+	return nil
+>>>>>>> MERGE-SOURCE
 }
 
 // Close terminates the connection to the environment and releases

=== modified file 'juju/conn_test.go'
--- juju/conn_test.go	2012-08-30 13:21:00 +0000
+++ juju/conn_test.go	2012-08-31 02:04:18 +0000
@@ -92,7 +92,7 @@
 	c.Assert(err, ErrorMatches, "dummy environment not bootstrapped")
 }
 
-func (cs *ConnSuite) TestConnStateSecretsSideEffect(c *C) {
+func (*ConnSuite) TestConnStateSecretsSideEffect(c *C) {
 	env, err := environs.NewFromAttrs(map[string]interface{}{
 		"name":            "erewhemos",
 		"type":            "dummy",
@@ -129,6 +129,29 @@
 	c.Assert(cfg.UnknownAttrs()["secret"], Equals, "pork")
 }
 
+func (cs *ConnSuite) TestConnStateDoesNotUpdateExistingSecrets(c *C) {
+	cs.TestConnStateSecretsSideEffect(c)
+	conn, err := juju.NewConnFromAttrs(map[string]interface{}{
+		"name":            "erewhemos",
+		"type":            "dummy",
+		"zookeeper":       true,
+		"authorized-keys": "i-am-a-key",
+		"secret":          "squirrel",
+	})
+	c.Assert(err, IsNil)
+	defer conn.Close()
+	st, err := conn.State()
+	c.Assert(err, IsNil)
+	cfg, err := st.EnvironConfig()
+	c.Assert(err, IsNil)
+	err = cfg.Read()
+	c.Assert(err, IsNil)
+	// check that the secret has not changed
+	secret, ok := cfg.Get("secret")
+	c.Assert(ok, Equals, true)
+	c.Assert(secret, Equals, "pork")
+}
+
 func (*ConnSuite) TestValidRegexps(c *C) {
 	assertService := func(s string, expect bool) {
 		c.Assert(juju.ValidService.MatchString(s), Equals, expect)

=== modified file 'state/machine.go'
--- state/machine.go	2012-08-17 12:10:32 +0000
+++ state/machine.go	2012-08-31 02:04:18 +0000
@@ -71,7 +71,7 @@
 		"instance id of machine %v", m.String())
 	if _, ok := err.(*NotFoundError); ok || (err == nil && instanceId == "") {
 		return "", &NotFoundError{
-			fmt.Sprintf("instance id for machine %d is not set", m.Id()),
+			fmt.Sprintf("instance id for machine %d", m.Id()),
 		}
 	}
 	return instanceId, err