Merge lp:~axwalk/juju-core/reinstate-cmd-juju-bootstrap-source into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

cmd/juju: reinstate --source for juju bootstrap

In r2069 I removed the --source option of juju bootstrap.
It has since come to light that this remains a desirable
option for MAAS users, and that we should go through a
two-step deprecation anyway.

I've reinstated the option, but changed the way it's
implemented: it now simply overrides the value of
environs/sync.DefaultToolsLocation.

https://codereview.appspot.com/29990044/

cmd/juju: reinstate --source for juju bootstrap

In r2069 I removed the --source option of juju bootstrap.
It has since come to light that this remains a desirable
option for MAAS users, and that we should go through a
two-step deprecation anyway.

I've reinstated the option, but changed the way it's
implemented: it now simply overrides the value of
environs/sync.DefaultToolsLocation.

https://codereview.appspot.com/29990044/

[Andrew Wilkins (axwalk)]

Reviewers: mp+196040_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: reinstate --source for juju bootstrap

In r2069 I removed the --source option of juju bootstrap.
It has since come to light that this remains a desirable
option for MAAS users, and that we should go through a
two-step deprecation anyway.

I've reinstated the option, but changed the way it's
implemented: it now simply overrides the value of
environs/sync.DefaultToolsLocation.

https://code.launchpad.net/~axwalk/juju-core/reinstate-cmd-juju-bootstrap-source/+merge/196040

(do not edit description out of merge proposal)

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

Affected files (+48, -1 lines):
   A [revision details]
   M cmd/juju/bootstrap.go
   M cmd/juju/bootstrap_test.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-20131120142833-kh97nkpng5qaps4o
+New revision: <email address hidden>

Index: cmd/juju/bootstrap.go
=== modified file 'cmd/juju/bootstrap.go'
--- cmd/juju/bootstrap.go 2013-11-18 05:41:36 +0000
+++ cmd/juju/bootstrap.go 2013-11-21 01:18:17 +0000
@@ -52,6 +52,7 @@
   Constraints constraints.Value
   UploadTools bool
   Series []string
+ Source string
  }

  func (c *BootstrapCommand) Info() *cmd.Info {
@@ -67,6 +68,7 @@
   f.Var(constraints.ConstraintsValue{&c.Constraints}, "constraints", "set
environment constraints")
   f.BoolVar(&c.UploadTools, "upload-tools", false, "upload local version of
tools before bootstrapping")
   f.Var(seriesVar{&c.Series}, "series", "upload tools for supplied
comma-separated series list")
+ f.StringVar(&c.Source, "source", "", "local path to use as tools source")
  }

  func (c *BootstrapCommand) Init(args []string) error {
@@ -98,7 +100,11 @@
   if err := bootstrap.EnsureNotBootstrapped(environ); err != nil {
    return err
   }
-
+ // If --source is specified, override the default tools source.
+ if c.Source != "" {
+ logger.Infof("Setting default tools source: %s", c.Source)
+ sync.DefaultToolsLocation = c.Source
+ }
   // TODO (wallyworld): 2013-09-20 bug 1227931
   // We can set a custom tools data source instead of doing an
   // unecessary upload.

Index: cmd/juju/bootstrap_test.go
=== modified file 'cmd/juju/bootstrap_test.go'
--- cmd/juju/bootstrap_test.go 2013-11-18 06:27:04 +0000
+++ cmd/juju/bootstrap_test.go 2013-11-21 01:18:17 +0000
@@ -313,6 +313,45 @@
   c.Check(coretesting.Stdout(ctx2), gc.Equals, "")
  }

+func (s *BootstrapSuite) TestAutoSyncLocalSource(c *gc.C) {
+ // Prepare a tools directory for testing and store the
+ // dummy tools in there.
+ sourceDir := createToolsSource(c, vAll)
+
+ // Change the version and ensure its later restoring.
+ origVersion := version.Current
+ version.Current.Number = version.MustParse("1.2.0")
+ defer func() {
+ version.Current = origVersion
+ }()
+
+ // Create home with dummy provider and remove all
+ // of its envtools.
+ env, fake := makeEmptyFakeHome(c)
+ defer fake.Restore()
+
+ // Bootstrap the environment with an i...

[Ian Booth (wallyworld)]

LGTM with the test split up

https://codereview.appspot.com/29990044/diff/1/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/29990044/diff/1/cmd/juju/bootstrap_test.go#newcode316
cmd/juju/bootstrap_test.go:316: func (s *BootstrapSuite)
TestAutoSyncLocalSource(c *gc.C) {
I'd really like this split into 2 tests, one with invalid source, one
with valid source.

https://codereview.appspot.com/29990044/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/29990044/diff/1/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/29990044/diff/1/cmd/juju/bootstrap_test.go#newcode316
cmd/juju/bootstrap_test.go:316: func (s *BootstrapSuite)
TestAutoSyncLocalSource(c *gc.C) {
On 2013/11/22 00:02:02, wallyworld wrote:
> I'd really like this split into 2 tests, one with invalid source, one
with valid
> source.

Done.

https://codereview.appspot.com/29990044/

=== modified file 'cmd/juju/bootstrap.go'
--- cmd/juju/bootstrap.go	2013-11-18 05:41:36 +0000
+++ cmd/juju/bootstrap.go	2013-11-22 01:59:00 +0000
@@ -52,6 +52,7 @@
 	Constraints constraints.Value
 	UploadTools bool
 	Series      []string
+	Source      string
 }
 
 func (c *BootstrapCommand) Info() *cmd.Info {
@@ -67,6 +68,7 @@
 	f.Var(constraints.ConstraintsValue{&c.Constraints}, "constraints", "set environment constraints")
 	f.BoolVar(&c.UploadTools, "upload-tools", false, "upload local version of tools before bootstrapping")
 	f.Var(seriesVar{&c.Series}, "series", "upload tools for supplied comma-separated series list")
+	f.StringVar(&c.Source, "source", "", "local path to use as tools source")
 }
 
 func (c *BootstrapCommand) Init(args []string) error {
@@ -98,7 +100,11 @@
 	if err := bootstrap.EnsureNotBootstrapped(environ); err != nil {
 		return err
 	}
-
+	// If --source is specified, override the default tools source.
+	if c.Source != "" {
+		logger.Infof("Setting default tools source: %s", c.Source)
+		sync.DefaultToolsLocation = c.Source
+	}
 	// TODO (wallyworld): 2013-09-20 bug 1227931
 	// We can set a custom tools data source instead of doing an
 	// unecessary upload.

=== modified file 'cmd/juju/bootstrap_test.go'
--- cmd/juju/bootstrap_test.go	2013-11-18 06:27:04 +0000
+++ cmd/juju/bootstrap_test.go	2013-11-22 01:59:00 +0000
@@ -313,6 +313,40 @@
 	c.Check(coretesting.Stdout(ctx2), gc.Equals, "")
 }
 
+func (s *BootstrapSuite) TestInvalidLocalSource(c *gc.C) {
+	s.PatchValue(&version.Current.Number, version.MustParse("1.2.0"))
+	env, fake := makeEmptyFakeHome(c)
+	defer fake.Restore()
+
+	// Bootstrap the environment with an invalid source.
+	// The command returns with an error.
+	ctx := coretesting.Context(c)
+	code := cmd.Main(&BootstrapCommand{}, ctx, []string{"--source", c.MkDir()})
+	c.Check(code, gc.Equals, 1)
+
+	// Now check that there are no tools available.
+	_, err := envtools.FindTools(
+		env, version.Current.Major, version.Current.Minor, coretools.Filter{}, envtools.DoNotAllowRetry)
+	c.Assert(err, gc.FitsTypeOf, errors.NotFoundf(""))
+}
+
+func (s *BootstrapSuite) TestAutoSyncLocalSource(c *gc.C) {
+	sourceDir := createToolsSource(c, vAll)
+	s.PatchValue(&version.Current.Number, version.MustParse("1.2.0"))
+	env, fake := makeEmptyFakeHome(c)
+	defer fake.Restore()
+
+	// Bootstrap the environment with the valid source.
+	// The bootstrapping has to show no error, because the tools
+	// are automatically synchronized.
+	ctx := coretesting.Context(c)
+	code := cmd.Main(&BootstrapCommand{}, ctx, []string{"--source", sourceDir})
+	c.Check(code, gc.Equals, 0)
+
+	// Now check the available tools which are the 1.2.0 envtools.
+	checkTools(c, env, v120All)
+}
+
 func (s *BootstrapSuite) setupAutoUploadTest(c *gc.C, vers, series string) environs.Environ {
 	s.PatchValue(&sync.Upload, mockUploadTools)
 	sourceDir := createToolsSource(c, vAll)