Change the way aliases to subcommands are handled.

Change the way aliases to subcommands are handled.

The primary motivation for this change is to be able to list all the
aliases for a command when asking for help on that command.

This led to changes in the way the command aliases work, and where the aliases
are stored.  The commands now specify their own aliases rather than them being
specified when the command is added to the super command.

https://code.launchpad.net/~thumper/juju-core/command-aliases/+merge/149461

(do not edit description out of merge proposal)

[thumper2, 2013-02-20 04:13:44]

Please take a look.

[dave_cheney.net, 2013-02-20 04:42:10]

One general comment. You can avoid the boilerplate of 

cmd.NewInfo(...) *Info

by using a struct literal like this

func (*somecommand) Info() *Info {
 return &Info {
    Name: "my name",
    Something: "something else",
 }
}

For the cases that do have aliases, include the 

   Aliases: []string{"this one", "and that one"},

[dimitern, 2013-02-20 08:07:05]

Overall LGTM, with some comments.

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

https://codereview.appspot.com/7370043/diff/1/cmd/juju/generateconfig.go#newc...
cmd/juju/generateconfig.go:17: return cmd.NewInfo("init", "", "generate
boilerplate configuration for juju environments", "", "generate-config")
Not sure about "init" - it's a bit misleading and it's only really needed when
you don't have environs.yaml. Calling it "init" could confuse the users it's a
required call.

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

https://codereview.appspot.com/7370043/diff/1/cmd/juju/main_test.go#newcode147
cmd/juju/main_test.go:147: "hide",
:) "obscure" ? not really

https://codereview.appspot.com/7370043/diff/1/cmd/juju/main_test.go#newcode155
cmd/juju/main_test.go:155: "stat",
How about detecting and adding support of unambiguous prefixes for each command.
So "st","sta","stat",etc == "status", like mercurial does for example.

https://codereview.appspot.com/7370043/diff/1/environs/boilerplate_config.go
File environs/boilerplate_config.go (right):

https://codereview.appspot.com/7370043/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:12: ## By default Juju ships AWS (default), HP
Cloud, OpenStack.
Good!

[dave_cheney.net, 2013-02-20 09:39:07]

>
https://codereview.appspot.com/7370043/diff/1/environs/boilerplate_config.go#...
> environs/boilerplate_config.go:12: ## By default Juju ships AWS
> (default), HP Cloud, OpenStack.
> Good!

If we really wanted to gold plate this, we could query the
EnvironProvider for a list of registered providers.

[rog, 2013-02-20 13:37:08]

On 2013/02/20 09:39:07, dfc wrote:
> >
>
https://codereview.appspot.com/7370043/diff/1/environs/boilerplate_config.go#...
> > environs/boilerplate_config.go:12: ## By default Juju ships AWS
> > (default), HP Cloud, OpenStack.
> > Good!
> 
> If we really wanted to gold plate this, we could query the
> EnvironProvider for a list of registered providers.

we do - the boilerplate config queries all the registered providers for their
boilerplate.

[rog, 2013-02-20 13:37:49]

a few minor remarks below - LGTM with those addressed.

https://codereview.appspot.com/7370043/diff/1/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/7370043/diff/1/cmd/cmd.go#newcode75
cmd/cmd.go:75: func NewInfo(name string, args string, purpose string, doc
string, aliases ...string) *Info {
+1 to dfc's remark about this function. a constructor used directly would be
more clear. if all the existing uses had used constructors with explicit field
names, as is recommended (and even checked for by go vet), we could have added
Aliases without changing any of the places that make a new Info.

i'd prefer to see a change that did that.

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

https://codereview.appspot.com/7370043/diff/1/cmd/juju/generateconfig.go#newc...
cmd/juju/generateconfig.go:17: return cmd.NewInfo("init", "", "generate
boilerplate configuration for juju environments", "", "generate-config")
On 2013/02/20 08:07:05, dimitern wrote:
> Not sure about "init" - it's a bit misleading and it's only really needed when
> you don't have environs.yaml. Calling it "init" could confuse the users it's a
> required call.

+1

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

https://codereview.appspot.com/7370043/diff/1/cmd/juju/main_test.go#newcode155
cmd/juju/main_test.go:155: "stat",
On 2013/02/20 08:07:05, dimitern wrote:
> How about detecting and adding support of unambiguous prefixes for each
command.
> So "st","sta","stat",etc == "status", like mercurial does for example.

-1, i think. it makes it difficult to add more commands to the name space later,
as we'd need to worry about clashing with commonly used prefixes.

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

https://codereview.appspot.com/7370043/diff/1/cmd/juju/unexpose.go#newcode18
cmd/juju/unexpose.go:18: return cmd.NewInfo("unexpose", "", "unexpose a
service", "", "hide")
i'm not sure we need this alias. well, not without general agreement anyway. the
other aliases were there only to preserve backward compatibility while enabling
more correct names when moving forwards. i think it may be best to keep their
use to a minimum.

[fwereade, 2013-02-20 15:54:30]

Nice change; LGTM assuming agreement with the below.

https://codereview.appspot.com/7370043/diff/1/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/7370043/diff/1/cmd/cmd.go#newcode75
cmd/cmd.go:75: func NewInfo(name string, args string, purpose string, doc
string, aliases ...string) *Info {
On 2013/02/20 13:37:49, rog wrote:
> +1 to dfc's remark about this function. a constructor used directly would be
> more clear. if all the existing uses had used constructors with explicit field
> names, as is recommended (and even checked for by go vet), we could have added
> Aliases without changing any of the places that make a new Info.
> 
> i'd prefer to see a change that did that.

Yeah... except I implemented all the Infos lazily and stupidly. What I really
should have done, and would ideally like to see here, is Info methods which all
use explicit field setting.

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

https://codereview.appspot.com/7370043/diff/1/cmd/juju/generateconfig.go#newc...
cmd/juju/generateconfig.go:17: return cmd.NewInfo("init", "", "generate
boilerplate configuration for juju environments", "", "generate-config")
On 2013/02/20 13:37:49, rog wrote:
> On 2013/02/20 08:07:05, dimitern wrote:
> > Not sure about "init" - it's a bit misleading and it's only really needed
when
> > you don't have environs.yaml. Calling it "init" could confuse the users it's
a
> > required call.
> 
> +1

Not so sure -- init turns an unenvironed home dir into an environed one. And
doesn't bootstrap suggest calling "generate-config"? Which would probably be
even better named "init" in that situation?

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

https://codereview.appspot.com/7370043/diff/1/cmd/juju/unexpose.go#newcode18
cmd/juju/unexpose.go:18: return cmd.NewInfo("unexpose", "", "unexpose a
service", "", "hide")
On 2013/02/20 13:37:49, rog wrote:
> i'm not sure we need this alias. well, not without general agreement anyway.
the
> other aliases were there only to preserve backward compatibility while
enabling
> more correct names when moving forwards. i think it may be best to keep their
> use to a minimum.

Yeah -- I think these deserve wider discussion. Please stick to the existing
aliases for now; maybe suggest aliases on the lists?

[thumper2, 2013-02-20 22:28:24]

So, lots of changes again, but this time mostly to the standard named field
constructor use and the removal of the NewInfo function.

Also I continue arguing for "init" below.

https://codereview.appspot.com/7370043/diff/1/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/7370043/diff/1/cmd/cmd.go#newcode75
cmd/cmd.go:75: func NewInfo(name string, args string, purpose string, doc
string, aliases ...string) *Info {
On 2013/02/20 15:54:30, fwereade wrote:
> On 2013/02/20 13:37:49, rog wrote:
> > +1 to dfc's remark about this function. a constructor used directly would be
> > more clear. if all the existing uses had used constructors with explicit
field
> > names, as is recommended (and even checked for by go vet), we could have
added
> > Aliases without changing any of the places that make a new Info.
> > 
> > i'd prefer to see a change that did that.
> 
> Yeah... except I implemented all the Infos lazily and stupidly. What I really
> should have done, and would ideally like to see here, is Info methods which
all
> use explicit field setting.

I've removed this method now, and used the named constructor.

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

https://codereview.appspot.com/7370043/diff/1/cmd/juju/generateconfig.go#newc...
cmd/juju/generateconfig.go:17: return cmd.NewInfo("init", "", "generate
boilerplate configuration for juju environments", "", "generate-config")
On 2013/02/20 15:54:30, fwereade wrote:
> On 2013/02/20 13:37:49, rog wrote:
> > On 2013/02/20 08:07:05, dimitern wrote:
> > > Not sure about "init" - it's a bit misleading and it's only really needed
> when
> > > you don't have environs.yaml. Calling it "init" could confuse the users
it's
> a
> > > required call.
> > 
> > +1
> 
> Not so sure -- init turns an unenvironed home dir into an environed one. And
> doesn't bootstrap suggest calling "generate-config"? Which would probably be
> even better named "init" in that situation?

I'm prepared to argue this case.  I've talked with jcastro about this as well. 
It seems that for many first time users, "init" is the command they expect. 
Also, when we have a local provider, we should have that set up to be the
default environment when someone goes "juju init".  That way there is nothing
else the user needs to do to start using juju.  This would need a change to the
generate-config command so it writes the environments.yaml file if there isn't
one existing, but that is outside the scope of this branch.

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

https://codereview.appspot.com/7370043/diff/1/cmd/juju/main_test.go#newcode147
cmd/juju/main_test.go:147: "hide",
On 2013/02/20 08:07:05, dimitern wrote:
> :) "obscure" ? not really
Removed.

https://codereview.appspot.com/7370043/diff/1/cmd/juju/main_test.go#newcode155
cmd/juju/main_test.go:155: "stat",
On 2013/02/20 13:37:49, rog wrote:
> On 2013/02/20 08:07:05, dimitern wrote:
> > How about detecting and adding support of unambiguous prefixes for each
> command.
> > So "st","sta","stat",etc == "status", like mercurial does for example.
> 
> -1, i think. it makes it difficult to add more commands to the name space
later,
> as we'd need to worry about clashing with commonly used prefixes.

While I think this could be a reasonable approach, it would require a little
more work, and outside the scope of this branch.

For example, a user goes
 juju st -> it is pretty obvious they want status

We need to make sure we handle exact matches first
 "juju set" should never be confused with "juju set-environment"

We could fairly easily have the following:
 $ juju remove foo
 error: could no disambiguate between 'remove-relation' and 'remove-unit'

That way, if we had someone use to typing "juju st" for status, and we add a
command like "stash", then once the command was added, the user would be
informed that "st" was no longer unique enough.

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

https://codereview.appspot.com/7370043/diff/1/cmd/juju/unexpose.go#newcode18
cmd/juju/unexpose.go:18: return cmd.NewInfo("unexpose", "", "unexpose a
service", "", "hide")
On 2013/02/20 15:54:30, fwereade wrote:
> On 2013/02/20 13:37:49, rog wrote:
> > i'm not sure we need this alias. well, not without general agreement anyway.
> the
> > other aliases were there only to preserve backward compatibility while
> enabling
> > more correct names when moving forwards. i think it may be best to keep
their
> > use to a minimum.
> 
> Yeah -- I think these deserve wider discussion. Please stick to the existing
> aliases for now; maybe suggest aliases on the lists?

Removed now, we can discuss newer aliases on the list.

[thumper2, 2013-02-20 22:29:39]

Please take a look.

[dave_cheney.net, 2013-02-21 01:35:12]

LGTM.

https://codereview.appspot.com/7370043/diff/3005/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/7370043/diff/3005/cmd/cmd.go#newcode69
cmd/cmd.go:69: // Aliases are other ways to call the Command.
// Aliases are other names for the Command.

https://codereview.appspot.com/7370043/diff/3005/cmd/juju/main_test.go
File cmd/juju/main_test.go (right):

https://codereview.appspot.com/7370043/diff/3005/cmd/juju/main_test.go#newcod...
cmd/juju/main_test.go:147: "init",
suggestion:

"init", // alias for ...

https://codereview.appspot.com/7370043/diff/3005/cmd/juju/main_test.go#newcod...
cmd/juju/main_test.go:154: "stat",
same

https://codereview.appspot.com/7370043/diff/3005/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/7370043/diff/3005/cmd/juju/ssh.go#newcode27
cmd/juju/ssh.go:27: const sshDoc = `
You need to collapse the next line here, otherwise sshDoc will start with a \n

https://codereview.appspot.com/7370043/diff/3005/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/7370043/diff/3005/cmd/supercommand.go#newcode38
cmd/supercommand.go:38: _, found := c.subcmds[name]
if _, found := c.subcmds[name]; found {
   ... panic
}

[thumper2, 2013-02-21 01:57:45]

https://codereview.appspot.com/7370043/diff/3005/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/7370043/diff/3005/cmd/cmd.go#newcode69
cmd/cmd.go:69: // Aliases are other ways to call the Command.
On 2013/02/21 01:35:12, dfc wrote:
> // Aliases are other names for the Command.

Done.

https://codereview.appspot.com/7370043/diff/3005/cmd/juju/main_test.go
File cmd/juju/main_test.go (right):

https://codereview.appspot.com/7370043/diff/3005/cmd/juju/main_test.go#newcod...
cmd/juju/main_test.go:147: "init",
On 2013/02/21 01:35:12, dfc wrote:
> suggestion:
> 
> "init", // alias for ...

Done.

https://codereview.appspot.com/7370043/diff/3005/cmd/juju/main_test.go#newcod...
cmd/juju/main_test.go:154: "stat",
On 2013/02/21 01:35:12, dfc wrote:
> same

Done.

https://codereview.appspot.com/7370043/diff/3005/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/7370043/diff/3005/cmd/juju/ssh.go#newcode27
cmd/juju/ssh.go:27: const sshDoc = `
On 2013/02/21 01:35:12, dfc wrote:
> You need to collapse the next line here, otherwise sshDoc will start with a \n

Pretty sure that all the doc strings are stripped using strings.TrimSpace
(cmd.go:97)

https://codereview.appspot.com/7370043/diff/3005/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/7370043/diff/3005/cmd/supercommand.go#newcode38
cmd/supercommand.go:38: _, found := c.subcmds[name]
On 2013/02/21 01:35:12, dfc wrote:
> if _, found := c.subcmds[name]; found {
>    ... panic
> }

Sure, why not.

[dave_cheney.net, 2013-02-21 02:01:14]

You've got more than enough LGTMs, commit that sukka.

https://codereview.appspot.com/7370043/diff/3005/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/7370043/diff/3005/cmd/juju/ssh.go#newcode27
cmd/juju/ssh.go:27: const sshDoc = `
Yeah, they are, my bad, reading further in the review I found others that follow
the same pattern. Ignore my bleating.

[thumper2, 2013-02-21 02:30:33]

Please take a look.

[fwereade, 2013-02-22 09:32:19]

LGTM, just trivials

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

https://codereview.appspot.com/7370043/diff/3007/cmd/juju/bootstrap.go#newcode48
cmd/juju/bootstrap.go:48: fmt.Fprintln(out, "    juju generate-config -w")
juju init -w

(is most correct for now, right?)

https://codereview.appspot.com/7370043/diff/3007/cmd/juju/constraints.go
File cmd/juju/constraints.go (right):

https://codereview.appspot.com/7370043/diff/3007/cmd/juju/constraints.go#newc...
cmd/juju/constraints.go:21: Args:    "[<service> [<constraints>...]",
[<service>] [<constraint> ...] maybe?

https://codereview.appspot.com/7370043/diff/3007/cmd/juju/constraints.go#newc...
cmd/juju/constraints.go:81: Args:    "[key=[value],...]",
not comma-separated

https://codereview.appspot.com/7370043/diff/3007/cmd/juju/destroymachine.go
File cmd/juju/destroymachine.go (right):

https://codereview.appspot.com/7370043/diff/3007/cmd/juju/destroymachine.go#n...
cmd/juju/destroymachine.go:20: Args:    "<machine> [, ...]",
I'm not sure about the commas again.

https://codereview.appspot.com/7370043/diff/3007/cmd/juju/generateconfig.go
File cmd/juju/generateconfig.go (right):

https://codereview.appspot.com/7370043/diff/3007/cmd/juju/generateconfig.go#n...
cmd/juju/generateconfig.go:18: Name:    "init",
I'm +1 on this change, but please change the type name (and filename) to match
"init" rather than "generate-command". I want to keep the help feature somewhat
bounded, but I'm +1 on your arguments re "init" and would appreciate a followup
that tweaks it.

https://codereview.appspot.com/7370043/diff/3007/cmd/juju/main_test.go
File cmd/juju/main_test.go (right):

https://codereview.appspot.com/7370043/diff/3007/cmd/juju/main_test.go#newcod...
cmd/juju/main_test.go:149: "remove-relation", // alias for destory-relation
s/destory/destroy/

https://codereview.appspot.com/7370043/diff/3007/cmd/juju/set.go
File cmd/juju/set.go (right):

https://codereview.appspot.com/7370043/diff/3007/cmd/juju/set.go#newcode26
cmd/juju/set.go:26: Args:    "<service> <config option> [<config option>...]",
Maybe s/config option/name=value/?

[thumper2, 2013-02-24 22:34:11]

Please take a look.

[thumper2, 2013-02-24 22:41:50]

*** Submitted:

Change the way aliases to subcommands are handled.

The primary motivation for this change is to be able to list all the
aliases for a command when asking for help on that command.

This led to changes in the way the command aliases work, and where the aliases
are stored.  The commands now specify their own aliases rather than them being
specified when the command is added to the super command.

R=dfc, dimitern, rog, fwereade
CC=
https://codereview.appspot.com/7370043