cmd/juju: refactor --series to use StringsValue

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)

[menn0, 2014-05-02 20:38:12]

Please take a look.

[fwereade, 2014-05-06 07:37:55]

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.

[menn0, 2014-05-06 23:05:20]

Please take a look.

[menn0, 2014-05-08 01:35:18]

Please take a look.

[thumper, 2014-05-08 01:41:19]

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.

[thumper, 2014-05-08 01:53:05]

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#newc...
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#newc...
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...
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)

[menn0, 2014-05-08 02:35:48]

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#newc...
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#newc...
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...
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)

[thumper, 2014-05-08 04:53:52]

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#newc...
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#newc...
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...
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.

[menn0, 2014-05-08 05:20:48]

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#newc...
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...