Merge lp:~wallyworld/juju-core/fix-tools-tests-1.18 into lp:juju-core/1.18

[Ian Booth]

A handful of unit tests which call bootstrap or functions which upload tools failed if the Juju version is a release number eg 1.18. This is because those tests did not first explicitly upload tools and auto upload for release tools is not allowed.

The affected tests patch the version.Current variable to change the version to a dev release.

Also, changes were made to allow release tools be uploaded (a version.Dev() check is removed). Tested live with EC2.

A handful of unit tests which call bootstrap or functions which upload tools failed if the Juju version is a release number eg 1.18. This is because those tests did not first explicitly upload tools and auto upload for release tools is not allowed.

The affected tests patch the version.Current variable to change the version to a dev release.

Also, changes were made to allow release tools be uploaded (a version.Dev() check is removed). Tested live with EC2.

[William Reade (fwereade)]

LGTM. Thank you very much: you have gone further above and beyond than anyone can or should expect, please don't burn yourself out.

=== modified file 'cmd/juju/bootstrap_test.go'
--- cmd/juju/bootstrap_test.go	2014-04-03 19:11:11 +0000
+++ cmd/juju/bootstrap_test.go	2014-04-04 14:36:15 +0000
@@ -303,9 +303,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-%LTS%-amd64",
+	args:    []string{"-e", "brokenenv"},
+	err:     `dummy.Bootstrap is broken`,
 }, {
 	info:        "constraints",
 	args:        []string{"--constraints", "mem=4G cpu-cores=4"},
@@ -369,6 +370,11 @@
 	defer fake.Restore()
 	defaultSeriesVersion := version.Current
 	defaultSeriesVersion.Series = config.PreferredSeries(env.Config())
+	// Force a dev version by having an odd minor version number.
+	// This is because we have not uploaded any tools and auto
+	// upload is only enabled for dev versions.
+	defaultSeriesVersion.Minor = 11
+	s.PatchValue(&version.Current, defaultSeriesVersion)
 
 	ctx := coretesting.Context(c)
 	code := cmd.Main(&BootstrapCommand{}, ctx, nil)
@@ -388,6 +394,11 @@
 	defer fake.Restore()
 	defaultSeriesVersion := version.Current
 	defaultSeriesVersion.Series = config.PreferredSeries(env.Config())
+	// Force a dev version by having an odd minor version number.
+	// This is because we have not uploaded any tools and auto
+	// upload is only enabled for dev versions.
+	defaultSeriesVersion.Minor = 11
+	s.PatchValue(&version.Current, defaultSeriesVersion)
 
 	store, err := configstore.Default()
 	c.Assert(err, gc.IsNil)
@@ -458,6 +469,12 @@
 	defer fake.Restore()
 
 	// Bootstrap the environment with the valid source.
+	// Force a dev version by having an odd minor version number.
+	// This is because we have not uploaded any tools and auto
+	// upload is only enabled for dev versions.
+	devVersion := version.Current
+	devVersion.Minor = 11
+	s.PatchValue(&version.Current, devVersion)
 	ctx := coretesting.Context(c)
 	code := cmd.Main(&BootstrapCommand{}, ctx, []string{"--metadata-source", sourceDir})
 	c.Check(code, gc.Equals, 0)
@@ -580,6 +597,12 @@
 func (s *BootstrapSuite) TestBootstrapDestroy(c *gc.C) {
 	_, fake := makeEmptyFakeHome(c)
 	defer fake.Restore()
+	devVersion := version.Current
+	// Force a dev version by having an odd minor version number.
+	// This is because we have not uploaded any tools and auto
+	// upload is only enabled for dev versions.
+	devVersion.Minor = 11
+	s.PatchValue(&version.Current, devVersion)
 	opc, errc := runCommand(nullContext(c), new(BootstrapCommand), "-e", "brokenenv")
 	err := <-errc
 	c.Assert(err, gc.ErrorMatches, "dummy.Bootstrap is broken")

=== modified file 'environs/bootstrap/bootstrap_test.go'
--- environs/bootstrap/bootstrap_test.go	2014-04-03 19:11:11 +0000
+++ environs/bootstrap/bootstrap_test.go	2014-04-04 14:36:15 +0000
@@ -180,8 +180,11 @@
 		}
 		err = bootstrap.Bootstrap(coretesting.Context(c), env, cons)
 		if test.Err != "" {
-			stripped := strings.Replace(err.Error(), "\n", "", -1)
-			c.Check(stripped, gc.Matches, ".*"+stripped)
+			c.Check(err, gc.NotNil)
+			if err != nil {
+				stripped := strings.Replace(err.Error(), "\n", "", -1)
+				c.Check(stripped, gc.Matches, ".*"+stripped)
+			}
 			continue
 		} else {
 			c.Check(err, gc.IsNil)
@@ -213,6 +216,12 @@
 	s.PatchValue(&arch.HostArch, func() string {
 		return "amd64"
 	})
+	// Force a dev version by having an odd minor version number.
+	// This is because we have not uploaded any tools and auto
+	// upload is only enabled for dev versions.
+	devVersion := version.Current
+	devVersion.Minor = 11
+	s.PatchValue(&version.Current, devVersion)
 	env := newEnviron("foo", useDefaultKeys, nil)
 	s.setDummyStorage(c, env)
 	envtesting.RemoveFakeTools(c, env.Storage())
@@ -230,6 +239,12 @@
 	s.PatchValue(&arch.HostArch, func() string {
 		return "ppc64"
 	})
+	// Force a dev version by having an odd minor version number.
+	// This is because we have not uploaded any tools and auto
+	// upload is only enabled for dev versions.
+	devVersion := version.Current
+	devVersion.Minor = 11
+	s.PatchValue(&version.Current, devVersion)
 	env := newEnviron("foo", useDefaultKeys, nil)
 	s.setDummyStorage(c, env)
 	envtesting.RemoveFakeTools(c, env.Storage())
@@ -255,7 +270,7 @@
 }
 
 func (s *bootstrapSuite) TestEnsureToolsAvailabilityNonDevVersion(c *gc.C) {
-	// Can't upload tools for released versions.
+	// Can't automatically upload tools for released versions.
 	s.PatchValue(&version.Current, version.MustParseBinary("1.18.0-trusty-arm64"))
 	env := newEnviron("foo", useDefaultKeys, nil)
 	s.setDummyStorage(c, env)
@@ -339,7 +354,7 @@
 	c.Assert(bootstrap.SeriesToUpload(cfg, nil), jc.SameContents, []string{"quantal", config.LatestLtsSeries(), "lucid"})
 }
 
-func (s *bootstrapSuite) assertUploadTools(c *gc.C, vers version.Binary, allowRelease bool,
+func (s *bootstrapSuite) assertUploadTools(c *gc.C, vers version.Binary, forceVersion bool,
 	extraConfig map[string]interface{}, errMessage string) {
 
 	s.PatchValue(&version.Current, vers)
@@ -360,8 +375,9 @@
 		return "arm64"
 	})
 	arch := "arm64"
-	err := bootstrap.UploadTools(coretesting.Context(c), env, &arch, allowRelease, "precise")
+	err := bootstrap.UploadTools(coretesting.Context(c), env, &arch, forceVersion, "precise")
 	if errMessage != "" {
+		c.Assert(err, gc.NotNil)
 		stripped := strings.Replace(err.Error(), "\n", "", -1)
 		c.Assert(stripped, gc.Matches, errMessage)
 		return
@@ -369,14 +385,14 @@
 	c.Assert(err, gc.IsNil)
 	params := envtools.BootstrapToolsParams{
 		Arch:   &arch,
-		Series: "precise",
+		Series: version.Current.Series,
 	}
 	agentTools, err := envtools.FindBootstrapTools(env, params)
 	c.Assert(err, gc.IsNil)
 	c.Assert(agentTools, gc.HasLen, 1)
 	expectedVers := vers
 	expectedVers.Number.Build++
-	expectedVers.Series = "precise"
+	expectedVers.Series = version.Current.Series
 	c.Assert(agentTools[0].Version, gc.DeepEquals, expectedVers)
 }
 
@@ -385,10 +401,10 @@
 	s.assertUploadTools(c, vers, false, nil, "")
 }
 
-func (s *bootstrapSuite) TestUploadToolsForceVersionAllowsReleaseTools(c *gc.C) {
+func (s *bootstrapSuite) TestUploadToolsReleaseToolsWithDevConfig(c *gc.C) {
 	vers := version.MustParseBinary("1.18.0-trusty-arm64")
 	extraCfg := map[string]interface{}{"development": true}
-	s.assertUploadTools(c, vers, true, extraCfg, "")
+	s.assertUploadTools(c, vers, false, extraCfg, "")
 }
 
 func (s *bootstrapSuite) TestUploadToolsForceVersionAllowsAgentVersionSet(c *gc.C) {
@@ -397,11 +413,6 @@
 	s.assertUploadTools(c, vers, true, extraCfg, "")
 }
 
-func (s *bootstrapSuite) TestUploadToolsReleaseVersionDisallowed(c *gc.C) {
-	vers := version.MustParseBinary("1.18.0-trusty-arm64")
-	s.assertUploadTools(c, vers, false, nil, "Juju cannot bootstrap because no tools are available for your environment.*")
-}
-
 type bootstrapEnviron struct {
 	name             string
 	cfg              *config.Config

=== modified file 'environs/bootstrap/synctools.go'
--- environs/bootstrap/synctools.go	2014-04-03 19:11:11 +0000
+++ environs/bootstrap/synctools.go	2014-04-04 14:36:15 +0000
@@ -29,8 +29,7 @@
 
 // UploadTools uploads tools for the specified series and any other relevant series to
 // the environment storage, after which it sets the agent-version. If forceVersion is true,
-// we allow uploading release tools versions and allow uploading even when the agent-version is
-// already set in the environment.
+// we allow uploading even when the agent-version is already set in the environment.
 func UploadTools(ctx environs.BootstrapContext, env environs.Environ, toolsArch *string, forceVersion bool, bootstrapSeries ...string) error {
 	logger.Infof("checking that upload is possible")
 	// Check the series are valid.
@@ -112,9 +111,8 @@
 // not be allowed.
 func validateUploadAllowed(env environs.Environ, toolsArch *string, forceVersion bool) error {
 	if !forceVersion {
-		// First, check that there isn't already an agent version specified, and that we
-		// are running a development version.
-		if _, hasAgentVersion := env.Config().AgentVersion(); hasAgentVersion || !version.Current.IsDev() {
+		// First, check that there isn't already an agent version specified.
+		if _, hasAgentVersion := env.Config().AgentVersion(); hasAgentVersion {
 			return fmt.Errorf(noToolsNoUploadMessage)
 		}
 	}
@@ -174,6 +172,11 @@
 		return nil, err
 	}
 
+	// Only automatically upload tools for dev versions.
+	if !version.Current.IsDev() {
+		return nil, fmt.Errorf("cannot upload bootstrap tools: %v", noToolsNoUploadMessage)
+	}
+
 	// No tools available so our only hope is to build locally and upload.
 	logger.Warningf("no prepackaged tools available")
 	uploadSeries := SeriesToUpload(cfg, nil)