Merge lp:~menno.smits/juju-core/1302005-bootstrap_series into lp:~go-bot/juju-core/trunk

[Menno Finlay-Smits]

cmd/juju: deprecate --series, added --upload-series

Emit a deprecation warning to stderr when --series is used

To make testing easier an interface is now used to allow the calls
made by BootstrapCommand.Run to environs/bootstrap to be mocked out.

https://codereview.appspot.com/94060044/

R=thumper

cmd/juju: refactor --series to use StringsValue

seriesVar is now called seriesValue and is implemented using
StringsValue. This reuses StringValue while preserving the series
validation logic.

https://codereview.appspot.com/94060044/

[Menno Finlay-Smits (menno.smits)]

Reviewers: mp+218152_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: refactor --series to use StringsValue

seriesVar is now called seriesValue and is implemented using
StringsValue. This reuses StringValue while preserving the series
validation logic.

https://code.launchpad.net/~menno.smits/juju-core/1302005-bootstrap_series/+merge/218152

(do not edit description out of merge proposal)

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

Affected files (+28, -22 lines):
   A [revision details]
   M cmd/juju/bootstrap.go
   M cmd/juju/upgradejuju.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-20140501192736-36p3425ohtpy4np7
+New revision: <email address hidden>

Index: cmd/juju/bootstrap.go
=== modified file 'cmd/juju/bootstrap.go'
--- cmd/juju/bootstrap.go 2014-04-30 22:05:31 +0000
+++ cmd/juju/bootstrap.go 2014-05-02 19:25:05 +0000
@@ -6,7 +6,6 @@
  import (
   "fmt"
   "os"
- "strings"

   "launchpad.net/gnuflag"

@@ -81,7 +80,7 @@
   c.EnvCommandBase.SetFlags(f)
   f.Var(constraints.ConstraintsValue{Target:
&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.Var(newSeriesValue(nil, &c.Series), "series", "upload tools for
supplied comma-separated series list")
   f.StringVar(&c.MetadataSource, "metadata-source", "", "local path to use
as tools and/or metadata source")
   f.StringVar(&c.Placement, "to", "", "a placement directive indicating an
instance to bootstrap")
  }
@@ -106,6 +105,30 @@
   return cmd.CheckEmpty(args)
  }

+type seriesValue struct {
+ *cmd.StringsValue
+}
+
+// newSeriesValue is used to create the type passed into the
gnuflag.FlagSet Var function.
+func newSeriesValue(defaultValue []string, target *[]string) *seriesValue {
+ v := seriesValue{(*cmd.StringsValue)(target)}
+ *(v.StringsValue) = defaultValue
+ return &v
+}
+
+// Implements gnuflag.Value Set.
+func (v *seriesValue) Set(s string) error {
+ if err := v.StringsValue.Set(s); err != nil {
+ return err
+ }
+ for _, name := range *(v.StringsValue) {
+ if !charm.IsValidSeries(name) {
+ return fmt.Errorf("invalid series name %q", name)
+ }
+ }
+ return nil
+}
+
  // Run connects to the environment specified on the command line and
bootstraps
  // a juju in that environment if none already exists. If there is as yet
no environments.yaml file,
  // the user is informed how to create one.
@@ -174,22 +197,3 @@
    Placement: c.Placement,
   })
  }
-
-type seriesVar struct {
- target *[]string
-}
-
-func (v seriesVar) Set(value string) error {
- names := strings.Split(value, ",")
- for _, name := range names {
- if !charm.IsValidSeries(name) {
- return fmt.Errorf("invalid series name %q", name)
- }
- }
- *v.target = names
- return nil
-}
-
-func (v seriesVar) String() string {
- return strings.Join(*v....

[John A Meinel (jameinel)]

Is there a specific advantage to using StringValue ? It looks like just code reshuffling without actually adding much. I'm happy to have it if there is an actual improvement in common handling (say StringsValue provides better parsing, etc).

So LGTM, though I'd like to understand what this actually helps with.

[William Reade (fwereade)]

LGTM

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

https://codereview.appspot.com/94060044/diff/1/cmd/juju/bootstrap.go#newcode128
cmd/juju/bootstrap.go:128: }
Quibble quibble whine: I'd rather validate before setting on the
StringsValue.

https://codereview.appspot.com/94060044/

[John A Meinel (jameinel)]

Don't you have to do the set first so that it parses the values into a
slice? Otherwise you just have the raw string and you are reimplementing
all the parsing anyway.

On Tue, May 6, 2014 at 11:39 AM, William Reade
<email address hidden>wrote:

> LGTM
>
>
> https://codereview.appspot.com/94060044/diff/1/cmd/juju/bootstrap.go
> File cmd/juju/bootstrap.go (right):
>
>
> https://codereview.appspot.com/94060044/diff/1/cmd/juju/bootstrap.go#newcode128
> cmd/juju/bootstrap.go:128: }
> Quibble quibble whine: I'd rather validate before setting on the
> StringsValue.
>
> https://codereview.appspot.com/94060044/
>
> --
>
> https://code.launchpad.net/~menno.smits/juju-core/1302005-bootstrap_series/+merge/218152
> You are subscribed to branch lp:juju-core.
>

[William Reade (fwereade)]

Dammit, ofc. Nil out the value on error?

[Menno Finlay-Smits (menno.smits)]

> Dammit, ofc. Nil out the value on error?

Done!

[Menno Finlay-Smits (menno.smits)]

Please take a look.

https://codereview.appspot.com/94060044/

[Menno Finlay-Smits (menno.smits)]

Please take a look.

https://codereview.appspot.com/94060044/

[Tim Penhey (thumper)]

On 2014/05/08 01:35:18, menn0 wrote:
> Please take a look.

Firstly, please don't reuse branches. It confuses things somewhat.

For new pieces of work, even related pieces, use a different branch.

https://codereview.appspot.com/94060044/

[Tim Penhey (thumper)]

LGTM with a few minor comments.

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go#newcode85
cmd/juju/bootstrap.go:85: f.Var(newSeriesValue(nil, &c.seriesOld),
"series", "upload tools for supplied comma-separated series list
(DEPRECATED, see --upload-series)")
Wondering if we should move the deprecated bit to the front of the help
string. Thoughts?

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go#newcode174
cmd/juju/bootstrap.go:174: _bootstrap := getBootstrapFuncs()
a small part of me recoils at this prefix, but I can't really think of a
better one that makes more sense, so ok with it.

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

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap_test.go#newcode438
cmd/juju/bootstrap_test.go:438: code := cmd.Main(&bootstrapCommand, ctx,
[]string{"--upload-tools", argVariant, "foo,bar"})
A better call would be the testing.RunCommand:

   RunCommand(c *gc.C, com cmd.Command, args []string) (*cmd.Context,
error)

ctx, err := testing.RunCommand(c, &BootstrapCommand{},
[]string{"--upload-tools", argVariant, "foo,bar"})
c.Assert(err, gc.IsNil)

https://codereview.appspot.com/94060044/

[Menno Finlay-Smits (menno.smits)]

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go#newcode85
cmd/juju/bootstrap.go:85: f.Var(newSeriesValue(nil, &c.seriesOld),
"series", "upload tools for supplied comma-separated series list
(DEPRECATED, see --upload-series)")
On 2014/05/08 01:53:05, thumper wrote:
> Wondering if we should move the deprecated bit to the front of the
help string.
> Thoughts?

I have no strong opinion on this. I was just following what was already
done for the -u flag on the juju deploy command.

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go#newcode174
cmd/juju/bootstrap.go:174: _bootstrap := getBootstrapFuncs()
On 2014/05/08 01:53:05, thumper wrote:
> a small part of me recoils at this prefix, but I can't really think of
a better
> one that makes more sense, so ok with it.

I didn't like this much either. Alternatives I also considered where:

* bootstrap (shadowing the module): potentially confusing for the casual
reader.
* bootstrapApi: but it's not really an official API of any sort
* bootstrapFuncs or similar: collides with the struct but could change
the struct name.

Do any of these appeal more than _bootstrap?

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

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap_test.go#newcode438
cmd/juju/bootstrap_test.go:438: code := cmd.Main(&bootstrapCommand, ctx,
[]string{"--upload-tools", argVariant, "foo,bar"})
On 2014/05/08 01:53:05, thumper wrote:
> A better call would be the testing.RunCommand:

> RunCommand(c *gc.C, com cmd.Command, args []string) (*cmd.Context,
error)

> ctx, err := testing.RunCommand(c, &BootstrapCommand{},
> []string{"--upload-tools", argVariant, "foo,bar"})
> c.Assert(err, gc.IsNil)

I got cmd.Main pattern this from the other tests in the file. RunCommand
is much more sensible. I'll change the test.

Should I do the others in this file as well? (as a separate change)

https://codereview.appspot.com/94060044/

[Tim Penhey (thumper)]

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go#newcode85
cmd/juju/bootstrap.go:85: f.Var(newSeriesValue(nil, &c.seriesOld),
"series", "upload tools for supplied comma-separated series list
(DEPRECATED, see --upload-series)")
On 2014/05/08 02:35:48, menn0 wrote:
> On 2014/05/08 01:53:05, thumper wrote:
> > Wondering if we should move the deprecated bit to the front of the
help
> string.
> > Thoughts?

> I have no strong opinion on this. I was just following what was
already done for
> the -u flag on the juju deploy command.

I think it'll be OK... since we are emitting a deprecation warning
anyway.

If it was the ONLY deprecation warning there was then I'd say this would
be different.

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go#newcode174
cmd/juju/bootstrap.go:174: _bootstrap := getBootstrapFuncs()
On 2014/05/08 02:35:48, menn0 wrote:
> On 2014/05/08 01:53:05, thumper wrote:
> > a small part of me recoils at this prefix, but I can't really think
of a
> better
> > one that makes more sense, so ok with it.

> I didn't like this much either. Alternatives I also considered where:

> * bootstrap (shadowing the module): potentially confusing for the
casual reader.
> * bootstrapApi: but it's not really an official API of any sort
> * bootstrapFuncs or similar: collides with the struct but could change
the
> struct name.

> Do any of these appeal more than _bootstrap?

Shadowing the outer struct shouldn't be a big issue. I think of those
options, I'd prefer
   bootstrapFuncs

I know others would prefer "bf" or similar, but not me.

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

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap_test.go#newcode438
cmd/juju/bootstrap_test.go:438: code := cmd.Main(&bootstrapCommand, ctx,
[]string{"--upload-tools", argVariant, "foo,bar"})
On 2014/05/08 02:35:48, menn0 wrote:
> On 2014/05/08 01:53:05, thumper wrote:
> > A better call would be the testing.RunCommand:
> >
> > RunCommand(c *gc.C, com cmd.Command, args []string) (*cmd.Context,
error)
> >
> > ctx, err := testing.RunCommand(c, &BootstrapCommand{},
> > []string{"--upload-tools", argVariant, "foo,bar"})
> > c.Assert(err, gc.IsNil)

> I got cmd.Main pattern this from the other tests in the file.
RunCommand is much
> more sensible. I'll change the test.

> Should I do the others in this file as well? (as a separate change)

Sure, that would be good.

https://codereview.appspot.com/94060044/

[Menno Finlay-Smits (menno.smits)]

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go#newcode174
cmd/juju/bootstrap.go:174: _bootstrap := getBootstrapFuncs()
On 2014/05/08 04:53:52, thumper wrote:
> On 2014/05/08 02:35:48, menn0 wrote:
> > On 2014/05/08 01:53:05, thumper wrote:
> > > a small part of me recoils at this prefix, but I can't really
think of a
> > better
> > > one that makes more sense, so ok with it.
> >
> > I didn't like this much either. Alternatives I also considered
where:
> >
> > * bootstrap (shadowing the module): potentially confusing for the
casual
> reader.
> > * bootstrapApi: but it's not really an official API of any sort
> > * bootstrapFuncs or similar: collides with the struct but could
change the
> > struct name.
> >
> > Do any of these appeal more than _bootstrap?

> Shadowing the outer struct shouldn't be a big issue. I think of those
options,
> I'd prefer
> bootstrapFuncs

> I know others would prefer "bf" or similar, but not me.

The only (minor) I have with bootstrapFuncs is that it's a type in the
outer scope and an instance of the type inside. Changing it anyway...

https://codereview.appspot.com/94060044/

=== modified file 'cmd/juju/bootstrap.go'
--- cmd/juju/bootstrap.go	2014-05-06 20:39:36 +0000
+++ cmd/juju/bootstrap.go	2014-05-08 05:19:19 +0000
@@ -64,6 +64,7 @@
 	Constraints    constraints.Value
 	UploadTools    bool
 	Series         []string
+	seriesOld      []string
 	MetadataSource string
 	Placement      string
 }
@@ -80,7 +81,8 @@
 	c.EnvCommandBase.SetFlags(f)
 	f.Var(constraints.ConstraintsValue{Target: &c.Constraints}, "constraints", "set environment constraints")
 	f.BoolVar(&c.UploadTools, "upload-tools", false, "upload local version of tools before bootstrapping")
-	f.Var(newSeriesValue(nil, &c.Series), "series", "upload tools for supplied comma-separated series list")
+	f.Var(newSeriesValue(nil, &c.Series), "upload-series", "upload tools for supplied comma-separated series list")
+	f.Var(newSeriesValue(nil, &c.seriesOld), "series", "upload tools for supplied comma-separated series list (DEPRECATED, see --upload-series)")
 	f.StringVar(&c.MetadataSource, "metadata-source", "", "local path to use as tools and/or metadata source")
 	f.StringVar(&c.Placement, "to", "", "a placement directive indicating an instance to bootstrap")
 }
@@ -91,8 +93,18 @@
 		return
 	}
 	if len(c.Series) > 0 && !c.UploadTools {
+		return fmt.Errorf("--upload-series requires --upload-tools")
+	}
+	if len(c.seriesOld) > 0 && !c.UploadTools {
 		return fmt.Errorf("--series requires --upload-tools")
 	}
+	if len(c.Series) > 0 && len(c.seriesOld) > 0 {
+		return fmt.Errorf("--upload-series and --series can't be used together")
+	}
+	if len(c.seriesOld) > 0 {
+		c.Series = c.seriesOld
+	}
+
 	// Parse the placement directive. Bootstrap currently only
 	// supports provider-specific placement directives.
 	if c.Placement != "" {
@@ -130,10 +142,41 @@
 	return nil
 }
 
+// bootstrap functionality that Run calls to support cleaner testing
+type BootstrapInterface interface {
+	EnsureNotBootstrapped(env environs.Environ) error
+	UploadTools(environs.BootstrapContext, environs.Environ, *string, bool, ...string) error
+	Bootstrap(ctx environs.BootstrapContext, environ environs.Environ, args environs.BootstrapParams) error
+}
+
+type bootstrapFuncs struct{}
+
+func (b bootstrapFuncs) EnsureNotBootstrapped(env environs.Environ) error {
+	return bootstrap.EnsureNotBootstrapped(env)
+}
+
+func (b bootstrapFuncs) UploadTools(ctx environs.BootstrapContext, env environs.Environ, toolsArch *string, forceVersion bool, bootstrapSeries ...string) error {
+	return bootstrap.UploadTools(ctx, env, toolsArch, forceVersion, bootstrapSeries...)
+}
+
+func (b bootstrapFuncs) Bootstrap(ctx environs.BootstrapContext, env environs.Environ, args environs.BootstrapParams) error {
+	return bootstrap.Bootstrap(ctx, env, args)
+}
+
+var getBootstrapFuncs = func() BootstrapInterface {
+	return &bootstrapFuncs{}
+}
+
 // Run connects to the environment specified on the command line and bootstraps
 // a juju in that environment if none already exists. If there is as yet no environments.yaml file,
 // the user is informed how to create one.
 func (c *BootstrapCommand) Run(ctx *cmd.Context) (resultErr error) {
+	bootstrapFuncs := getBootstrapFuncs()
+
+	if len(c.seriesOld) > 0 {
+		fmt.Fprintln(ctx.Stderr, "Use of --series is deprecated. Please use --upload-series instead.")
+	}
+
 	environ, cleanup, err := environFromName(ctx, c.EnvName, &resultErr, "Bootstrap")
 	if err != nil {
 		return err
@@ -150,7 +193,7 @@
 	}
 
 	defer cleanup()
-	if err := bootstrap.EnsureNotBootstrapped(environ); err != nil {
+	if err := bootstrapFuncs.EnsureNotBootstrapped(environ); err != nil {
 		return err
 	}
 
@@ -188,12 +231,12 @@
 		c.UploadTools = true
 	}
 	if c.UploadTools {
-		err = bootstrap.UploadTools(ctx, environ, c.Constraints.Arch, true, c.Series...)
+		err = bootstrapFuncs.UploadTools(ctx, environ, c.Constraints.Arch, true, c.Series...)
 		if err != nil {
 			return err
 		}
 	}
-	return bootstrap.Bootstrap(ctx, environ, environs.BootstrapParams{
+	return bootstrapFuncs.Bootstrap(ctx, environ, environs.BootstrapParams{
 		Constraints: c.Constraints,
 		Placement:   c.Placement,
 	})

=== modified file 'cmd/juju/bootstrap_test.go'
--- cmd/juju/bootstrap_test.go	2014-04-30 22:05:31 +0000
+++ cmd/juju/bootstrap_test.go	2014-05-08 05:19:19 +0000
@@ -305,6 +305,14 @@
 	args: []string{"--series", "fine"},
 	err:  `--series requires --upload-tools`,
 }, {
+	info: "lonely --upload-series",
+	args: []string{"--upload-series", "fine"},
+	err:  `--upload-series requires --upload-tools`,
+}, {
+	info: "--upload-series with --series",
+	args: []string{"--upload-tools", "--upload-series", "foo", "--series", "bar"},
+	err:  `--upload-series and --series can't be used together`,
+}, {
 	info:    "bad environment",
 	version: "1.2.3-%LTS%-amd64",
 	args:    []string{"-e", "brokenenv"},
@@ -351,7 +359,7 @@
 }, {
 	info:    "--upload-tools rejects invalid series",
 	version: "1.2.3-saucy-amd64",
-	args:    []string{"--upload-tools", "--series", "ping,ping,pong"},
+	args:    []string{"--upload-tools", "--upload-series", "ping,ping,pong"},
 	err:     `invalid series "ping"`,
 }, {
 	info:     "--upload-tools rejects mismatched arch",
@@ -407,6 +415,33 @@
 	c.Check(coretesting.Stdout(ctx2), gc.Equals, "")
 }
 
+func (s *BootstrapSuite) TestSeriesDeprecation(c *gc.C) {
+	ctx := s.checkSeriesArg(c, "--series")
+	c.Check(coretesting.Stderr(ctx), gc.Equals,
+		"Use of --series is deprecated. Please use --upload-series instead.\n")
+}
+
+func (s *BootstrapSuite) TestNoDeprecationWithUploadSeries(c *gc.C) {
+	ctx := s.checkSeriesArg(c, "--upload-series")
+	c.Check(coretesting.Stderr(ctx), gc.Equals, "")
+}
+
+func (s *BootstrapSuite) checkSeriesArg(c *gc.C, argVariant string) *cmd.Context {
+	_bootstrap := &fakeBootstrapFuncs{}
+	s.PatchValue(&getBootstrapFuncs, func() BootstrapInterface {
+		return _bootstrap
+	})
+	_, fake := makeEmptyFakeHome(c)
+	defer fake.Restore()
+
+	ctx, err := coretesting.RunCommand(c, &BootstrapCommand{},
+		[]string{"--upload-tools", argVariant, "foo,bar"})
+
+	c.Assert(err, gc.IsNil)
+	c.Check(_bootstrap.uploadToolsSeries, gc.DeepEquals, []string{"foo", "bar"})
+	return ctx
+}
+
 func (s *BootstrapSuite) TestBootstrapJenvWarning(c *gc.C) {
 	env, fake := makeEmptyFakeHome(c)
 	defer fake.Restore()
@@ -704,3 +739,24 @@
 	}
 	return all
 }
+
+// TODO(menn0): This fake BootstrapInterface implementation is
+// currently quite minimal but could be easily extended to cover more
+// test scenarios. This could help improve some of the tests in this
+// file which execute large amounts of external functionality.
+type fakeBootstrapFuncs struct {
+	uploadToolsSeries []string
+}
+
+func (fake *fakeBootstrapFuncs) EnsureNotBootstrapped(env environs.Environ) error {
+	return nil
+}
+
+func (fake *fakeBootstrapFuncs) UploadTools(ctx environs.BootstrapContext, env environs.Environ, toolsArch *string, forceVersion bool, bootstrapSeries ...string) error {
+	fake.uploadToolsSeries = bootstrapSeries
+	return nil
+}
+
+func (fake fakeBootstrapFuncs) Bootstrap(ctx environs.BootstrapContext, env environs.Environ, args environs.BootstrapParams) error {
+	return nil
+}