Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable alias on register command #2581

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Enable alias on register command #2581

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6a423f0
Enable alias on register command
RaphaelVRossi Oct 18, 2022
6e0cb0e
Fix tests
RaphaelVRossi Oct 18, 2022
ecf9826
Add me on Contributors
RaphaelVRossi Oct 18, 2022
c46898f
Do not show alias on help message
RaphaelVRossi Oct 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CONTRIBUTORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ Rafael Eyng <rafaeleyng@gmail.com>
Rafael Fragoso <rafael.fragoso@corp.globo.com>
Raniere Fernandes <Raninho@Raninho.com.br>
Raphael Amorim <rapha850@gmail.com>
Raphael Rossi <raphael.vieira.rossi@gmail.com>
Raul Gabrich <raul.freitas@corp.globo.com> <raulgbrmf@gmail.com>
Renan Mendes Carvalho <renan.carvalho@corp.globo.com>
Renan Oliveira <renan@corp.globo.com> <renan.gpa@gmail.com>
Expand Down
9 changes: 9 additions & 0 deletions cmd/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,14 @@ func (m *Manager) Register(command Command) {
m.Commands = make(map[string]Command)
}
name := command.Info().Name
m.RegisterCommandByName(name, command)
alias := command.Info().Alias
if alias != "" {
m.RegisterCommandByName(alias, command)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure but it could print the command info twice on help message - in different lines, though.

I'd take a better look on how we produce that help message and I'd show the comand + alias in same line - separeted by comma. For example:

app log, app logs                    Shows log entries for an application

Or maybe register the alias as a hidden command so it won't be generated on the help message even though it works fine when you fire on the command line.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, with this changes, help message show twice app log

./bin/tsuru app
App is a program source code running on Tsuru

The following commands are available in the "app" topic:

[...]
  app log                    Shows log entries for an application
  app log                    Shows log entries for an application
[...]

I tried to change where is printed help message, but didn't find.

So I didn't append command alias inside m.topicCommand, with that change, only show once at help message:

./bin/tsuru app
App is a program source code running on Tsuru

The following commands are available in the "app" topic:

  [...]
  app list                   Lists all apps that you have access to
  app log                    Shows log entries for an application
  app metadata get           Retrieves metadata for an application
  [...]

What do you think @nettoclaudio ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My last comment was about when you call tsuru app to show all available commands.

But this other test, I only call tsuru and the real command and the alias was in the list, is that correct?!

./bin/tsuru
Client version: dev.

Usage: tsuru command [args]

Available commands:
  [...]
  app list                   Lists all apps that you have access to
  app log                    Shows log entries for an application
  app logs                   Shows log entries for an application
  app metadata get           Retrieves metadata for an application
  [...]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK on this approach.

To be honest, I don't like to maintain this code on our side anymore. There are a lot of better CLI modules (e.g. spf13/cobra, urfave/cli and so on) than ours. I'm in favor to use them and remove this bunch of code. I don't know if all Tsuru maintainers agree, just my opnion.

}

func (m *Manager) RegisterCommandByName(name string, command Command) {
_, found := m.Commands[name]
if found {
panic(fmt.Sprintf("command already registered: %s", name))
Expand Down Expand Up @@ -621,6 +629,7 @@ func (c *Context) RawOutput() {

type Info struct {
Name string
Alias string
MinArgs int
MaxArgs int
Usage string
Expand Down
33 changes: 29 additions & 4 deletions cmd/cmd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ func (s *S) TestRegister(c *check.C) {
c.Assert(badCall, check.PanicMatches, "command already registered: foo")
}

func (s *S) TestRegisterCommandByName(c *check.C) {
globalManager.RegisterCommandByName("foobar", &TestCommand{})
badCall := func() { globalManager.RegisterCommandByName("foobar", &TestCommand{}) }
c.Assert(badCall, check.PanicMatches, "command already registered: foobar")
}

func (s *S) TestRegisterDeprecated(c *check.C) {
originalCmd := &TestCommand{}
globalManager.RegisterDeprecated(originalCmd, "bar")
Expand Down Expand Up @@ -107,13 +113,15 @@ func (s *S) TestRegisterTopicMultiple(c *check.C) {

type TopicCommand struct {
name string
alias string
executed bool
args []string
}

func (c *TopicCommand) Info() *Info {
return &Info{
Name: c.name,
Alias: c.alias,
Desc: "desc " + c.name,
Usage: "usage",
}
Expand Down Expand Up @@ -165,10 +173,10 @@ Use glb help <commandname> to get more information about a command.

func (s *S) TestNormalizedCommandsExec(c *check.C) {
cmds := map[string]*TopicCommand{
"foo": {name: "foo"},
"foo-bar": {name: "foo-bar"},
"foo-bar-zzz": {name: "foo-bar-zzz"},
"foo-bar-zzz-a-b": {name: "foo-bar-zzz-a-b"},
"foo": {name: "foo", alias: "fooalias"},
"foo-bar": {name: "foo-bar", alias: "fooalias-bar"},
"foo-bar-zzz": {name: "foo-bar-zzz", alias: "fooalias-bar-zzz"},
"foo-bar-zzz-a-b": {name: "foo-bar-zzz-a-b", alias: "fooalias-bar-zzz-a-b"},
}
for _, v := range cmds {
globalManager.Register(v)
Expand All @@ -195,6 +203,23 @@ func (s *S) TestNormalizedCommandsExec(c *check.C) {
{args: []string{"foo-bar-zzz-a-b", "x"}, expected: "foo-bar-zzz-a-b", expectedArgs: []string{"x"}},
{args: []string{"foo", "bar", "zzz", "a", "b"}, expected: "foo-bar-zzz-a-b"},
{args: []string{"foo", "bar", "zzz", "a", "b", "x"}, expected: "foo-bar-zzz-a-b", expectedArgs: []string{"x"}},
{args: []string{"foalias"}, expected: ""},
{args: []string{"fooalias"}, expected: "foo"},
{args: []string{"fooalias", "ba"}, expected: "foo", expectedArgs: []string{"ba"}},
{args: []string{"fooalias-bar"}, expected: "foo-bar"},
{args: []string{"fooalias-bar", "zz"}, expected: "foo-bar", expectedArgs: []string{"zz"}},
{args: []string{"fooalias", "bar"}, expected: "foo-bar"},
{args: []string{"fooalias", "bar", "zz"}, expected: "foo-bar", expectedArgs: []string{"zz"}},
{args: []string{"fooalias-bar-zzz"}, expected: "foo-bar-zzz"},
{args: []string{"fooalias-bar-zzz", "x"}, expected: "foo-bar-zzz", expectedArgs: []string{"x"}},
{args: []string{"fooalias-bar", "zzz"}, expected: "foo-bar-zzz"},
{args: []string{"fooalias", "bar-zzz"}, expected: "foo-bar-zzz"},
{args: []string{"fooalias", "bar", "zzz"}, expected: "foo-bar-zzz"},
{args: []string{"fooalias", "bar", "zzz", "x"}, expected: "foo-bar-zzz", expectedArgs: []string{"x"}},
{args: []string{"fooalias-bar-zzz-a-b"}, expected: "foo-bar-zzz-a-b"},
{args: []string{"fooalias-bar-zzz-a-b", "x"}, expected: "foo-bar-zzz-a-b", expectedArgs: []string{"x"}},
{args: []string{"fooalias", "bar", "zzz", "a", "b"}, expected: "foo-bar-zzz-a-b"},
{args: []string{"fooalias", "bar", "zzz", "a", "b", "x"}, expected: "foo-bar-zzz-a-b", expectedArgs: []string{"x"}},
}
for i, tt := range tests {
globalManager.Run(tt.args)
Expand Down