Merge lp:~wallyworld/juju-core/fix-local-bootstrap-tools into lp:~go-bot/juju-core/trunk

[Ian Booth]

Fix tools upload for local provider.

Local provider inserts agent-version into config
when the environment is opened and this was
preventing tools from being uploaded.

https://codereview.appspot.com/77960043/

Fix tools upload for local provider.

Local provider inserts agent-version into config
when the environment is opened and this was
preventing tools from being uploaded.

https://codereview.appspot.com/77960043/

[Ian Booth (wallyworld)]

Reviewers: mp+211831_code.launchpad.net,

Message:
Please take a look.

Description:
Fix tools upload for local provider.

Local provider inserts agent-version into config
when the environment is opened and this was
preventing tools from being uploaded.

https://code.launchpad.net/~wallyworld/juju-core/fix-local-bootstrap-tools/+merge/211831

(do not edit description out of merge proposal)

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

Affected files (+34, -20 lines):
   A [revision details]
   M environs/bootstrap/bootstrap_test.go
   M environs/bootstrap/synctools.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-20140319193533-t6r49574qjs0jkqw
+New revision: <email address hidden>

Index: environs/bootstrap/bootstrap_test.go
=== modified file 'environs/bootstrap/bootstrap_test.go'
--- environs/bootstrap/bootstrap_test.go 2014-03-19 01:20:14 +0000
+++ environs/bootstrap/bootstrap_test.go 2014-03-19 22:02:00 +0000
@@ -331,11 +331,13 @@
   c.Assert(bootstrap.SeriesToUpload(cfg, nil), gc.DeepEquals,
[]string{"quantal", "precise", "lucid"})
  }

-func (s *bootstrapSuite) assertUploadTools(c *gc.C, vers version.Binary,
allowRelease bool, errMessage string) {
+func (s *bootstrapSuite) assertUploadTools(c *gc.C, vers version.Binary,
allowRelease bool,
+ extraConfig map[string]interface{}, errMessage string) {
+
   s.PatchValue(&version.Current, vers)
   // If we allow released tools to be uploaded, the build number is
incremented so in that case
   // we need to ensure the environment is set up to allow dev tools to be
used.
- env := newEnviron("foo", useDefaultKeys,
map[string]interface{}{"development": allowRelease})
+ env := newEnviron("foo", useDefaultKeys, extraConfig)
   s.setDummyStorage(c, env)
   envtesting.RemoveFakeTools(c, env.Storage())

@@ -372,17 +374,24 @@

  func (s *bootstrapSuite) TestUploadTools(c *gc.C) {
   vers := version.MustParseBinary("1.19.0-trusty-arm64")
- s.assertUploadTools(c, vers, false, "")
-}
-
-func (s *bootstrapSuite) TestUploadToolsReleaseVersionAllowed(c *gc.C) {
- vers := version.MustParseBinary("1.18.0-trusty-arm64")
- s.assertUploadTools(c, vers, true, "")
+ s.assertUploadTools(c, vers, false, nil, "")
+}
+
+func (s *bootstrapSuite) TestUploadToolsForceVersionAllowsReleaseTools(c
*gc.C) {
+ vers := version.MustParseBinary("1.18.0-trusty-arm64")
+ extraCfg := map[string]interface{}{"development": true}
+ s.assertUploadTools(c, vers, true, extraCfg, "")
+}
+
+func (s *bootstrapSuite)
TestUploadToolsForceVersionAllowsAgentVersionSet(c *gc.C) {
+ vers := version.MustParseBinary("1.18.0-trusty-arm64")
+ extraCfg :=
map[string]interface{}{"agent-version": "1.18.0", "development": true}
+ 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, "Juju cannot bootstrap because no
tools are available for your environment.*")
+ ...

[Tim Penhey (thumper)]

On 2014/03/19 22:04:26, wallyworld wrote:
> Please take a look.

LGTM

https://codereview.appspot.com/77960043/

=== modified file 'environs/bootstrap/bootstrap_test.go'
--- environs/bootstrap/bootstrap_test.go	2014-03-19 01:20:14 +0000
+++ environs/bootstrap/bootstrap_test.go	2014-03-19 22:04:18 +0000
@@ -331,11 +331,13 @@
 	c.Assert(bootstrap.SeriesToUpload(cfg, nil), gc.DeepEquals, []string{"quantal", "precise", "lucid"})
 }
 
-func (s *bootstrapSuite) assertUploadTools(c *gc.C, vers version.Binary, allowRelease bool, errMessage string) {
+func (s *bootstrapSuite) assertUploadTools(c *gc.C, vers version.Binary, allowRelease bool,
+	extraConfig map[string]interface{}, errMessage string) {
+
 	s.PatchValue(&version.Current, vers)
 	// If we allow released tools to be uploaded, the build number is incremented so in that case
 	// we need to ensure the environment is set up to allow dev tools to be used.
-	env := newEnviron("foo", useDefaultKeys, map[string]interface{}{"development": allowRelease})
+	env := newEnviron("foo", useDefaultKeys, extraConfig)
 	s.setDummyStorage(c, env)
 	envtesting.RemoveFakeTools(c, env.Storage())
 
@@ -372,17 +374,24 @@
 
 func (s *bootstrapSuite) TestUploadTools(c *gc.C) {
 	vers := version.MustParseBinary("1.19.0-trusty-arm64")
-	s.assertUploadTools(c, vers, false, "")
-}
-
-func (s *bootstrapSuite) TestUploadToolsReleaseVersionAllowed(c *gc.C) {
-	vers := version.MustParseBinary("1.18.0-trusty-arm64")
-	s.assertUploadTools(c, vers, true, "")
+	s.assertUploadTools(c, vers, false, nil, "")
+}
+
+func (s *bootstrapSuite) TestUploadToolsForceVersionAllowsReleaseTools(c *gc.C) {
+	vers := version.MustParseBinary("1.18.0-trusty-arm64")
+	extraCfg := map[string]interface{}{"development": true}
+	s.assertUploadTools(c, vers, true, extraCfg, "")
+}
+
+func (s *bootstrapSuite) TestUploadToolsForceVersionAllowsAgentVersionSet(c *gc.C) {
+	vers := version.MustParseBinary("1.18.0-trusty-arm64")
+	extraCfg := map[string]interface{}{"agent-version": "1.18.0", "development": true}
+	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, "Juju cannot bootstrap because no tools are available for your environment.*")
+	s.assertUploadTools(c, vers, false, nil, "Juju cannot bootstrap because no tools are available for your environment.*")
 }
 
 type bootstrapEnviron struct {

=== modified file 'environs/bootstrap/synctools.go'
--- environs/bootstrap/synctools.go	2014-03-19 02:55:15 +0000
+++ environs/bootstrap/synctools.go	2014-03-19 22:04:18 +0000
@@ -27,8 +27,10 @@
 `
 
 // UploadTools uploads tools for the specified series and any other relevant series to
-// the environment storage, after which it sets the agent-version.
-func UploadTools(env environs.Environ, toolsArch *string, allowRelease bool, bootstrapSeries ...string) error {
+// 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.
+func UploadTools(env environs.Environ, toolsArch *string, forceVersion bool, bootstrapSeries ...string) error {
 	logger.Infof("checking that upload is possible")
 	// Check the series are valid.
 	for _, series := range bootstrapSeries {
@@ -37,15 +39,15 @@
 		}
 	}
 	// See that we are allowed to upload the tools.
-	if err := validateUploadAllowed(env, toolsArch, allowRelease); err != nil {
+	if err := validateUploadAllowed(env, toolsArch, forceVersion); err != nil {
 		return err
 	}
 
 	cfg := env.Config()
-	forceVersion := uploadVersion(version.Current.Number, nil)
+	explicitVersion := uploadVersion(version.Current.Number, nil)
 	uploadSeries := SeriesToUpload(cfg, bootstrapSeries)
 	logger.Infof("uploading tools for series %s", uploadSeries)
-	tools, err := sync.Upload(env.Storage(), &forceVersion, uploadSeries...)
+	tools, err := sync.Upload(env.Storage(), &explicitVersion, uploadSeries...)
 	if err != nil {
 		return err
 	}
@@ -91,13 +93,14 @@
 
 // validateUploadAllowed returns an error if an attempt to upload tools should
 // not be allowed.
-func validateUploadAllowed(env environs.Environ, toolsArch *string, allowRelease bool) error {
-	// 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 || (!allowRelease && !version.Current.IsDev()) {
-		return fmt.Errorf(noToolsNoUploadMessage)
+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() {
+			return fmt.Errorf(noToolsNoUploadMessage)
+		}
 	}
-
 	// Now check that the architecture for which we are setting up an
 	// environment matches that from which we are bootstrapping.
 	hostArch := arch.HostArch()