Code review comment for lp:~wallyworld/juju-core/fix-local-bootstrap-tools

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.*")
+ s.assertUploadTools(c, vers, false, nil, "Juju cannot bootstrap because
no tools are available for your environment.*")
  }

  type bootstrapEnviron struct {

Index: environs/bootstrap/synctools.go
=== 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:02:00 +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()