Code review - Issue 92440046: Log supercommand name.https://codereview.appspot.com/2014-05-21T20:04:06+00:00rietveld
Message from unknown
2014-05-19T16:05:49+00:00cmarsurn:md5:c42ded866b36a9ed97f1b535f4bcb99f
Message from casey.marshall@gmail.com
2014-05-19T16:05:54+00:00cmarsurn:md5:21171fb2b6f7f7d1a4015e37264fbf7a
Please take a look.
Message from menno.smits@canonical.com
2014-05-19T23:23:44+00:00menn0urn:md5:b55c95edce81e339cfef6ffff9251c7c
LGTM apart from the suggestion made in-line. I'm ok with the change as it stands if you don't think my proposal is a good idea.
https://codereview.appspot.com/92440046/diff/1/cmd/supercommand.go
File cmd/supercommand.go (right):
https://codereview.appspot.com/92440046/diff/1/cmd/supercommand.go#newcode301
cmd/supercommand.go:301: logger.Infof("running %s-%s [%s]", c.Name, version.Current, runtime.Compiler)
What about leaving juju in there too? I'm about to add a "user" super command and a log output of "running user-1.19.3 ..." doesn't make make as much sense as "running juju user-1.19.3 ..." IMHO.
Some special handling will be needed for the root juju command which is itself a supercommand - you don't really want to see "running juju juju-1.19.3 ..."
Message from casey.marshall@gmail.com
2014-05-19T23:57:25+00:00cmarsurn:md5:16bd4ba07eb8af5083d60948be9b26cd
https://codereview.appspot.com/92440046/diff/1/cmd/supercommand.go
File cmd/supercommand.go (right):
https://codereview.appspot.com/92440046/diff/1/cmd/supercommand.go#newcode301
cmd/supercommand.go:301: logger.Infof("running %s-%s [%s]", c.Name, version.Current, runtime.Compiler)
On 2014/05/19 23:23:44, menn0 wrote:
> What about leaving juju in there too? I'm about to add a "user" super command
> and a log output of "running user-1.19.3 ..." doesn't make make as much sense as
> "running juju user-1.19.3 ..." IMHO.
>
> Some special handling will be needed for the root juju command which is itself a
> supercommand - you don't really want to see "running juju juju-1.19.3 ..."
>
I could add a SuperCommand.Suite field, to identify a collection of commands.
If empty, you'd get "running {name}-{version}". If set, you get "running {suite} {name-version}".
What do you think?
Message from menno.smits@canonical.com
2014-05-20T00:08:50+00:00menn0urn:md5:00344c9faf947bd87b34b22a9c43bcff
On 2014/05/19 23:57:25, cmars wrote:
> https://codereview.appspot.com/92440046/diff/1/cmd/supercommand.go
> File cmd/supercommand.go (right):
>
> https://codereview.appspot.com/92440046/diff/1/cmd/supercommand.go#newcode301
> cmd/supercommand.go:301: logger.Infof("running %s-%s [%s]", c.Name,
> version.Current, runtime.Compiler)
> On 2014/05/19 23:23:44, menn0 wrote:
> > What about leaving juju in there too? I'm about to add a "user" super command
> > and a log output of "running user-1.19.3 ..." doesn't make make as much sense
> as
> > "running juju user-1.19.3 ..." IMHO.
> >
> > Some special handling will be needed for the root juju command which is itself
> a
> > supercommand - you don't really want to see "running juju juju-1.19.3 ..."
> >
>
> I could add a SuperCommand.Suite field, to identify a collection of commands.
>
> If empty, you'd get "running {name}-{version}". If set, you get "running {suite}
> {name-version}".
>
> What do you think?
That sounds good to me. I wonder if this could be set automatically when the SuperCommand is Register()ed with the parent? The parent's Suite could be used as the Suite for the child SuperCommand.
Message from menno.smits@canonical.com
2014-05-20T00:12:59+00:00menn0urn:md5:0170dc49979ba9878b878ce8e8e0ad54
On 2014/05/20 00:08:50, menn0 wrote:
> On 2014/05/19 23:57:25, cmars wrote:
> > https://codereview.appspot.com/92440046/diff/1/cmd/supercommand.go
> > File cmd/supercommand.go (right):
> >
> > https://codereview.appspot.com/92440046/diff/1/cmd/supercommand.go#newcode301
> > cmd/supercommand.go:301: logger.Infof("running %s-%s [%s]", c.Name,
> > version.Current, runtime.Compiler)
> > On 2014/05/19 23:23:44, menn0 wrote:
> > > What about leaving juju in there too? I'm about to add a "user" super
> command
> > > and a log output of "running user-1.19.3 ..." doesn't make make as much
> sense
> > as
> > > "running juju user-1.19.3 ..." IMHO.
> > >
> > > Some special handling will be needed for the root juju command which is
> itself
> > a
> > > supercommand - you don't really want to see "running juju juju-1.19.3 ..."
> > >
> >
> > I could add a SuperCommand.Suite field, to identify a collection of commands.
> >
> > If empty, you'd get "running {name}-{version}". If set, you get "running
> {suite}
> > {name-version}".
> >
> > What do you think?
>
> That sounds good to me. I wonder if this could be set automatically when the
> SuperCommand is Register()ed with the parent? The parent's Suite could be used
> as the Suite for the child SuperCommand.
I've just seen that SuperCommands already have a usagePrefix which has a similar function (and is already set to "juju" for various child SuperCommands). Could you just use that?
Message from unknown
2014-05-20T04:17:52+00:00cmarsurn:md5:161fc5c5410888b28dd1897f9399d359
Message from casey.marshall@gmail.com
2014-05-20T04:17:55+00:00cmarsurn:md5:79424e207532772659f7800b886f5fa0
Please take a look.
Message from fwereade@gmail.com
2014-05-20T06:40:12+00:00fwereadeurn:md5:dca658e4c5413347d9fbe81ed75efd75
I think we need to copy the value of runtime.Compiler into the version package, and use that, mainly so we can patch it out and get reliable output for the tests without .*s.
https://codereview.appspot.com/92440046/diff/20001/cmd/supercommand.go
File cmd/supercommand.go (right):
https://codereview.appspot.com/92440046/diff/20001/cmd/supercommand.go#newcode304
cmd/supercommand.go:304: logger.Infof("running %s %s-%s [%s]", c.usagePrefix, c.Name, version.Current, runtime.Compiler)
I'm not totally sold on the juju-1.2.3 business: could we maybe put the version and compiler into the brackets?
https://codereview.appspot.com/92440046/diff/20001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):
https://codereview.appspot.com/92440046/diff/20001/cmd/supercommand_test.go#newcode175
cmd/supercommand_test.go:175: var loggingTests = []struct {
Please move these global structs inside the tests that use them, as you come across them. (Sometimes one will be shared, and actually be legitimately global. It's pretty rare though.)
https://codereview.appspot.com/92440046/diff/20001/cmd/supercommand_test.go#newcode179
cmd/supercommand_test.go:179: {"juju", "juju", `^.* running juju-.*
This is why I hate ".*" in tests -- it looks like the actual change in behaviour is completely hidden behind them.
Please at least make the part we're testing part of the test -- you can patch out version.Current (and presumably the compiler, somehow?) in the test setup, and check them explicitly.
Message from unknown
2014-05-21T00:51:49+00:00cmarsurn:md5:32ba0e3a9cdf512a32593b4ee3c61464
Message from casey.marshall@gmail.com
2014-05-21T00:51:54+00:00cmarsurn:md5:ce987eb92ab659eef5e46caac5d13331
Please take a look.
https://codereview.appspot.com/92440046/diff/20001/cmd/supercommand.go
File cmd/supercommand.go (right):
https://codereview.appspot.com/92440046/diff/20001/cmd/supercommand.go#newcode304
cmd/supercommand.go:304: logger.Infof("running %s %s-%s [%s]", c.usagePrefix, c.Name, version.Current, runtime.Compiler)
On 2014/05/20 06:40:11, fwereade wrote:
> I'm not totally sold on the juju-1.2.3 business: could we maybe put the version
> and compiler into the brackets?
Done.
https://codereview.appspot.com/92440046/diff/20001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):
https://codereview.appspot.com/92440046/diff/20001/cmd/supercommand_test.go#newcode175
cmd/supercommand_test.go:175: var loggingTests = []struct {
On 2014/05/20 06:40:11, fwereade wrote:
> Please move these global structs inside the tests that use them, as you come
> across them. (Sometimes one will be shared, and actually be legitimately global.
> It's pretty rare though.)
Done. Also picked up a few in version/version_test.go.
https://codereview.appspot.com/92440046/diff/20001/cmd/supercommand_test.go#newcode179
cmd/supercommand_test.go:179: {"juju", "juju", `^.* running juju-.*
On 2014/05/20 06:40:12, fwereade wrote:
> This is why I hate ".*" in tests -- it looks like the actual change in behaviour
> is completely hidden behind them.
>
> Please at least make the part we're testing part of the test -- you can patch
> out version.Current (and presumably the compiler, somehow?) in the test setup,
> and check them explicitly.
Done.
Message from casey.marshall@gmail.com
2014-05-21T00:52:12+00:00cmarsurn:md5:36d1bbc862577c353ab0d0de775f08de
Message from fwereade@gmail.com
2014-05-21T07:42:25+00:00fwereadeurn:md5:3eea96a25fd684b277f4a3ff776af6aa
LGTM. Really nice, thanks :).
https://codereview.appspot.com/92440046/diff/40001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):
https://codereview.appspot.com/92440046/diff/40001/cmd/supercommand_test.go#newcode188
cmd/supercommand_test.go:188: version.Compiler = "llgo"
Search for PatchValue, you might find it helpful.
Message from unknown
2014-05-21T19:15:08+00:00cmarsurn:md5:ecbc72c4d43080936bd67031ebbbc807
Message from casey.marshall@gmail.com
2014-05-21T19:15:11+00:00cmarsurn:md5:946325f25cf1f923614fc193184bcfb7
Please take a look.
Message from casey.marshall@gmail.com
2014-05-21T19:16:31+00:00cmarsurn:md5:29446b7841bd4950af9c617b0d275d07
Updated to use CleanupSuite.PatchValue.
https://codereview.appspot.com/92440046/diff/40001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):
https://codereview.appspot.com/92440046/diff/40001/cmd/supercommand_test.go#newcode188
cmd/supercommand_test.go:188: version.Compiler = "llgo"
On 2014/05/21 07:42:25, fwereade wrote:
> Search for PatchValue, you might find it helpful.
Done.
Message from fwereade@gmail.com
2014-05-21T20:04:06+00:00fwereadeurn:md5:4db00efcfd25ac178bc1b1fc5b012093
still LGTM