Log supercommand name.

Log supercommand name.

In SuperCommand.Run, log the supercommand's name instead of hard-coded 'juju'.

https://code.launchpad.net/~cmars/juju-core/supercmd-logging/+merge/220097

(do not edit description out of merge proposal)

[cmars, 2014-05-19 16:05:54]

Please take a look.

[menn0, 2014-05-19 23:23:44]

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 ..."

[cmars, 2014-05-19 23:57:25]

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?

[menn0, 2014-05-20 00:08:50]

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.

[menn0, 2014-05-20 00:12:59]

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?

[cmars, 2014-05-20 04:17:55]

Please take a look.

[fwereade, 2014-05-20 06:40:12]

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#newcod...
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#n...
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#n...
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.

[cmars, 2014-05-21 00:51:54]

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#newcod...
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#n...
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#n...
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.

[fwereade, 2014-05-21 07:42:25]

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#n...
cmd/supercommand_test.go:188: version.Compiler = "llgo"
Search for PatchValue, you might find it helpful.

[cmars, 2014-05-21 19:15:11]

Please take a look.

[cmars, 2014-05-21 19:16:31]

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

[fwereade, 2014-05-21 20:04:06]

still LGTM