Merge lp:~axwalk/juju-core/1.16 into lp:juju-core/1.16

[Andrew Wilkins]

Fix upload-tools for released versions; fix tests

(Cherry picked from juju-core to juju-core/1.16)

There was an issue with sync.Upload/SyncTools for
release versions. Upload sets Dev to true in SyncContext,
and calls SyncTools. SyncTools then sets Dev to false
because MinorVersion==-1. This caused SyncTools to fail,
since uploaded tools are *always* dev (version.Build++).

Also, several bootstrap and upgrade-juju tests have been
fixed for running in release versions (tested with
version.version=="1.16.0").

Finally, some test cleanup (don't connect commands
to stdio in tests):
- Added nullContext helper
- Make runCommand panic if ctx is nil
- Replace runCommand(nil...) with runCommand(nullContext()...)

Fixes #1237123

https://codereview.appspot.com/14579044/

Fix upload-tools for released versions; fix tests

(Cherry picked from juju-core to juju-core/1.16)

There was an issue with sync.Upload/SyncTools for
release versions. Upload sets Dev to true in SyncContext,
and calls SyncTools. SyncTools then sets Dev to false
because MinorVersion==-1. This caused SyncTools to fail,
since uploaded tools are *always* dev (version.Build++).

Also, several bootstrap and upgrade-juju tests have been
fixed for running in release versions (tested with
version.version=="1.16.0").

Finally, some test cleanup (don't connect commands
to stdio in tests):
- Added nullContext helper
- Make runCommand panic if ctx is nil
- Replace runCommand(nil...) with runCommand(nullContext()...)

Fixes #1237123

https://codereview.appspot.com/14579044/

[Andrew Wilkins (axwalk)]

Reviewers: mp+190085_code.launchpad.net,

Message:
Please take a look.

Description:
Fix upload-tools for released versions; fix tests

(Cherry picked from juju-core to juju-core/1.16)

There was an issue with sync.Upload/SyncTools for
release versions. Upload sets Dev to true in SyncContext,
and calls SyncTools. SyncTools then sets Dev to false
because MinorVersion==-1. This caused SyncTools to fail,
since uploaded tools are *always* dev (version.Build++).

Also, several bootstrap and upgrade-juju tests have been
fixed for running in release versions (tested with
version.version=="1.16.0").

Finally, some test cleanup (don't connect commands
to stdio in tests):
- Added nullContext helper
- Make runCommand panic if ctx is nil
- Replace runCommand(nil...) with runCommand(nullContext()...)

Fixes #1237123

https://code.launchpad.net/~axwalk/juju-core/1.16/+merge/190085

(do not edit description out of merge proposal)

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

Affected files (+69, -24 lines):
   A [revision details]
   M cmd/juju/bootstrap_test.go
   M cmd/juju/cmd_test.go
   M environs/sync/sync.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-20131009063247-dtp5u8dlhoevxio4
+New revision: <email address hidden>

Index: cmd/juju/bootstrap_test.go
=== modified file 'cmd/juju/bootstrap_test.go'
--- cmd/juju/bootstrap_test.go 2013-10-03 01:56:15 +0000
+++ cmd/juju/bootstrap_test.go 2013-10-09 10:33:58 +0000
@@ -65,12 +65,30 @@
   args []string
   expectedAllowRetry []bool
   err string
+ // If version != "", version.Current will be
+ // set to it for the duration of the test.
+ version string
+ // If addVersionToSource is true, then "version"
+ // above will be populated in the tools source.
+ addVersionToSource bool
  }

  var bootstrapRetryTests = []bootstrapRetryTest{{
- info: "no tools uploaded, first check has no retries; check
after upload has retries",
- expectedAllowRetry: []bool{false, true},
- err: "tools not found",
+ info: "no tools uploaded, first check has no retries; no
matching binary in source; sync fails with no second attempt",
+ expectedAllowRetry: []bool{false},
+ err: "no matching tools available",
+ version: "1.16.0-precise-amd64",
+}, {
+ info: "no tools uploaded, first check has no retries;
matching binary in source; check after sync has retries",
+ expectedAllowRetry: []bool{false, true},
+ err: "tools not found",
+ version: "1.16.0-precise-amd64",
+ addVersionToSource: true,
+}, {
+ info: "no tools uploaded, first check has no retries; no
matching binary in source; check after upload has retries",
+ expectedAllowRetry: []bool{false, true},
+ err: "tools not found",
+ version: "1.15.1-precise-amd64", // dev version to force upload
  }, {
   info: "new tools uploaded, so we want to allow retries to
give them a cha...

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/14579044/

=== modified file 'cmd/juju/bootstrap_test.go'
--- cmd/juju/bootstrap_test.go	2013-10-03 01:56:15 +0000
+++ cmd/juju/bootstrap_test.go	2013-10-09 10:37:06 +0000
@@ -65,12 +65,30 @@
 	args               []string
 	expectedAllowRetry []bool
 	err                string
+	// If version != "", version.Current will be
+	// set to it for the duration of the test.
+	version string
+	// If addVersionToSource is true, then "version"
+	// above will be populated in the tools source.
+	addVersionToSource bool
 }
 
 var bootstrapRetryTests = []bootstrapRetryTest{{
-	info:               "no tools uploaded, first check has no retries; check after upload has retries",
-	expectedAllowRetry: []bool{false, true},
-	err:                "tools not found",
+	info:               "no tools uploaded, first check has no retries; no matching binary in source; sync fails with no second attempt",
+	expectedAllowRetry: []bool{false},
+	err:                "no matching tools available",
+	version:            "1.16.0-precise-amd64",
+}, {
+	info:               "no tools uploaded, first check has no retries; matching binary in source; check after sync has retries",
+	expectedAllowRetry: []bool{false, true},
+	err:                "tools not found",
+	version:            "1.16.0-precise-amd64",
+	addVersionToSource: true,
+}, {
+	info:               "no tools uploaded, first check has no retries; no matching binary in source; check after upload has retries",
+	expectedAllowRetry: []bool{false, true},
+	err:                "tools not found",
+	version:            "1.15.1-precise-amd64", // dev version to force upload
 }, {
 	info:               "new tools uploaded, so we want to allow retries to give them a chance at showing up",
 	args:               []string{"--upload-tools"},
@@ -87,12 +105,20 @@
 }
 
 func (s *BootstrapSuite) runAllowRetriesTest(c *gc.C, test bootstrapRetryTest) {
+	var extraVersions []version.Binary
+	if test.version != "" {
+		testVersion := version.MustParseBinary(test.version)
+		restore := testbase.PatchValue(&version.Current, testVersion)
+		defer restore()
+		if test.addVersionToSource {
+			extraVersions = append(extraVersions, testVersion)
+		}
+	}
 	_, fake := makeEmptyFakeHome(c)
 	defer fake.Restore()
-	defer createToolsStore(c)()
+	defer createToolsStore(c, extraVersions...)()
 
 	var findToolsRetryValues []bool
-
 	mockFindTools := func(cloudInst environs.ConfigGetter, majorVersion, minorVersion int,
 		filter coretools.Filter, allowRetry bool) (list coretools.List, err error) {
 		findToolsRetryValues = append(findToolsRetryValues, allowRetry)
@@ -102,7 +128,7 @@
 	restore := envtools.TestingPatchBootstrapFindTools(mockFindTools)
 	defer restore()
 
-	_, errc := runCommand(nil, new(BootstrapCommand), test.args...)
+	_, errc := runCommand(nullContext(), new(BootstrapCommand), test.args...)
 	err := <-errc
 	c.Check(findToolsRetryValues, gc.DeepEquals, test.expectedAllowRetry)
 	c.Assert(err, gc.ErrorMatches, test.err)
@@ -155,7 +181,7 @@
 	}
 
 	// Run command and check for uploads.
-	opc, errc := runCommand(nil, new(BootstrapCommand), test.args...)
+	opc, errc := runCommand(nullContext(), new(BootstrapCommand), test.args...)
 	if uploadCount > 0 {
 		for i := 0; i < uploadCount; i++ {
 			c.Check((<-opc).(dummy.OpPutFile).Env, gc.Equals, "peckham")
@@ -230,9 +256,10 @@
 	args: []string{"--series", "fine"},
 	err:  `--series requires --upload-tools`,
 }, {
-	info: "bad environment",
-	args: []string{"-e", "brokenenv"},
-	err:  `dummy.Bootstrap is broken`,
+	info:    "bad environment",
+	version: "1.2.3-precise-amd64",
+	args:    []string{"-e", "brokenenv"},
+	err:     `dummy.Bootstrap is broken`,
 }, {
 	info:        "constraints",
 	args:        []string{"--constraints", "mem=4G cpu-cores=4"},
@@ -270,10 +297,12 @@
 }}
 
 func (s *BootstrapSuite) TestBootstrapTwice(c *gc.C) {
-	restore := createToolsStore(c)
+	env, fake := makeEmptyFakeHome(c)
+	defer fake.Restore()
+	defaultSeriesVersion := version.Current
+	defaultSeriesVersion.Series = env.Config().DefaultSeries()
+	restore := createToolsStore(c, defaultSeriesVersion)
 	defer restore()
-	_, fake := makeEmptyFakeHome(c)
-	defer fake.Restore()
 
 	ctx := coretesting.Context(c)
 	code := cmd.Main(&BootstrapCommand{}, ctx, nil)
@@ -385,7 +414,7 @@
 	}
 	env := s.setupAutoUploadTest(c, "1.7.3", otherSeries)
 	// Run command and check for that upload has been run for tools matching the current juju version.
-	opc, errc := runCommand(nil, new(BootstrapCommand))
+	opc, errc := runCommand(nullContext(), new(BootstrapCommand))
 	c.Assert(<-errc, gc.IsNil)
 	c.Assert((<-opc).(dummy.OpPutFile).Env, gc.Equals, "peckham")
 	list, err := envtools.FindTools(env, version.Current.Major, version.Current.Minor, coretools.Filter{}, false)
@@ -406,7 +435,7 @@
 
 func (s *BootstrapSuite) TestAutoUploadOnlyForDev(c *gc.C) {
 	s.setupAutoUploadTest(c, "1.8.3", "precise")
-	_, errc := runCommand(nil, new(BootstrapCommand))
+	_, errc := runCommand(nullContext(), new(BootstrapCommand))
 	err := <-errc
 	c.Assert(err, gc.ErrorMatches, "no matching tools available")
 }
@@ -439,7 +468,7 @@
 }
 
 // createToolsStore creates the fake tools store.
-func createToolsStore(c *gc.C) func() {
+func createToolsStore(c *gc.C, additionalBinaries ...version.Binary) func() {
 	stor, err := envtesting.NewEC2HTTPTestStorage("127.0.0.1")
 	c.Assert(err, gc.IsNil)
 	origLocation := sync.DefaultToolsLocation
@@ -447,6 +476,9 @@
 	for _, vers := range vAll {
 		stor.PutBinary(vers)
 	}
+	for _, vers := range additionalBinaries {
+		stor.PutBinary(vers)
+	}
 	restore := func() {
 		sync.DefaultToolsLocation = origLocation
 	}

=== modified file 'cmd/juju/cmd_test.go'
--- cmd/juju/cmd_test.go	2013-09-30 20:01:49 +0000
+++ cmd/juju/cmd_test.go	2013-10-09 10:37:06 +0000
@@ -5,6 +5,8 @@
 
 import (
 	"bytes"
+	"io"
+	"io/ioutil"
 	"os"
 	"reflect"
 
@@ -126,7 +128,18 @@
 	}
 }
 
+func nullContext() *cmd.Context {
+	ctx := cmd.DefaultContext()
+	ctx.Stdin = io.LimitReader(nil, 0)
+	ctx.Stdout = ioutil.Discard
+	ctx.Stderr = ioutil.Discard
+	return ctx
+}
+
 func runCommand(ctx *cmd.Context, com cmd.Command, args ...string) (opc chan dummy.Operation, errc chan error) {
+	if ctx == nil {
+		panic("ctx == nil")
+	}
 	errc = make(chan error, 1)
 	opc = make(chan dummy.Operation, 200)
 	dummy.Listen(opc)
@@ -140,9 +153,6 @@
 			return
 		}
 
-		if ctx == nil {
-			ctx = cmd.DefaultContext()
-		}
 		err = com.Run(ctx)
 		errc <- err
 	}()
@@ -161,7 +171,7 @@
 	c.Assert(err, gc.IsNil)
 
 	// normal destroy
-	opc, errc := runCommand(nil, new(DestroyEnvironmentCommand), "--yes")
+	opc, errc := runCommand(nullContext(), new(DestroyEnvironmentCommand), "--yes")
 	c.Check(<-errc, gc.IsNil)
 	c.Check((<-opc).(dummy.OpDestroy).Env, gc.Equals, "peckham")
 
@@ -173,7 +183,7 @@
 	c.Assert(err, gc.IsNil)
 
 	// destroy with broken environment
-	opc, errc = runCommand(nil, new(DestroyEnvironmentCommand), "--yes", "-e", "brokenenv")
+	opc, errc = runCommand(nullContext(), new(DestroyEnvironmentCommand), "--yes", "-e", "brokenenv")
 	c.Check(<-opc, gc.IsNil)
 	c.Check(<-errc, gc.ErrorMatches, "dummy.Destroy is broken")
 	c.Check(<-opc, gc.IsNil)

=== modified file 'environs/sync/sync.go'
--- environs/sync/sync.go	2013-09-27 02:31:45 +0000
+++ environs/sync/sync.go	2013-10-09 10:37:06 +0000
@@ -70,9 +70,10 @@
 		if !syncContext.AllVersions {
 			syncContext.MinorVersion = version.Current.Minor
 		}
-	} else {
+	} else if !syncContext.Dev && syncContext.MinorVersion != -1 {
 		// If a major.minor version is specified, we allow dev versions.
-		syncContext.Dev = syncContext.MinorVersion != -1
+		// If Dev is already true, leave it alone.
+		syncContext.Dev = true
 	}
 	sourceTools, err := envtools.ReadList(sourceStorage, syncContext.MajorVersion, syncContext.MinorVersion)
 	if err != nil {
@@ -261,7 +262,7 @@
 		Source:       baseToolsDir,
 		Target:       stor,
 		AllVersions:  true,
-		Dev:          true,
+		Dev:          toolsVersion.IsDev(),
 		MajorVersion: toolsVersion.Major,
 		MinorVersion: -1,
 	}