Code review - Issue 94060044: cmd/juju: refactor --series to use StringsValuehttps://codereview.appspot.com/2014-05-08T05:20:48+00:00rietveld
Message from unknown
2014-05-02T20:38:09+00:00menn0urn:md5:d68c829011fab8b33dd57357cdf8995b
Message from menno.smits@canonical.com
2014-05-02T20:38:12+00:00menn0urn:md5:ec42b26623e0f901a3268023470353e0
Please take a look.
Message from fwereade@gmail.com
2014-05-06T07:37:55+00:00fwereadeurn:md5:3ccd0105625e2e2d73a911d4a13a7389
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.
Message from unknown
2014-05-06T23:05:14+00:00menn0urn:md5:03480bf07115340e897aa2be989f4d17
Message from menno.smits@canonical.com
2014-05-06T23:05:20+00:00menn0urn:md5:73573e29c52b74d9ca3bce7c731d2e12
Please take a look.
Message from unknown
2014-05-08T01:35:12+00:00menn0urn:md5:db2a8a89b39fb9363f10b9454c6845a8
Message from menno.smits@canonical.com
2014-05-08T01:35:18+00:00menn0urn:md5:3d25a9f7333d4b42b20ef6dedaed483b
Please take a look.
Message from tim.penhey@canonical.com
2014-05-08T01:41:19+00:00thumperurn:md5:e4736bbb0073b1341d26e852ad5ecb9c
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.
Message from tim.penhey@canonical.com
2014-05-08T01:53:05+00:00thumperurn:md5:837e80c0387b538d8280e947e5374c0a
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)
Message from menno.smits@canonical.com
2014-05-08T02:35:48+00:00menn0urn:md5:542270c596e766409d6b8fb8df1da242
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)
Message from tim.penhey@canonical.com
2014-05-08T04:53:52+00:00thumperurn:md5:f11dd9cd7c57b8f6ec50123172f5c73e
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.
Message from menno.smits@canonical.com
2014-05-08T05:20:48+00:00menn0urn:md5:b315940f177e033852b6ad8aed3cca91
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...