fix(args): Correct `legacyArgs` logical errors

[donggangcj]

Close #1156 Signed-off-by: Dong Gang [email protected]

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Dong Gang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[donggangcj]

@marckhouzam I know you've done a lot of work on helm and could you please review this pr if you are free.😀

[umarcor]

Ref #842.

[marckhouzam]

Thanks @donggangcj. When I tried this fix with helm, it didn't address the problem described in #1156.

Could you provide a test program that shows the problem before the PR, and that the problem is resolved after the PR?

As for helm, this PR touches the legacyArgs function, however, notice that legacyArgs is only called when command.Args == nil: https://github.com/spf13/cobra/blob/675ae5f5a98cc705a6d54f4c487ab81230604137/command.go#L627-L630

However, helm always uses command.Args, except for its root command, so I'm under the impression a different approach will be required to fix this problem.

[donggangcj]

@marckhouzam My bad! I missed important part in previous commit and I‘ve commit again. I ran a simple test locally and the result is here: Before the PR:

# NOTE: It prints the help message
➜  cobra git:(fix-exit-code-error) helm repo sadd foo https://foo/bar 

This command consists of multiple subcommands to interact with chart repositories.

It can be used to add, remove, list, and index chart repositories.

Usage:
  helm repo [command]

Available Commands:
....

➜  cobra git:(fix-exit-code-error) echo $?
0

After the PR:

# NOTE: It prints the Command.Args error string instead of help message
➜  bin git:(temp/validate) ✗ ./helm repo sadd foo https://foo/bar
Error: "helm repo" accepts no arguments

Usage:  helm repo add|remove|list|index|update [ARGS] [flags]

➜  bin git:(temp/validate) ✗ echo $?
1

The function legacyArgs is only called when Comand.Args == nil ,so the previous commit doesn't work if Command.Args != nil. helm approach is making commands that contains subcommands (like get,repo...)Command.Args= require.NoArgs,so it skips validation legacyArgs .

I continue to view the code logic and find the root cause of problem.

https://github.com/spf13/cobra/blob/675ae5f5a98cc705a6d54f4c487ab81230604137/command.go#L804-L817

If switch the order of part1 and part2 , everythings will be ok!

        // part1
	if !c.Runnable() {
		return flag.ErrHelp
	}

	c.preRun()

       // part2
        argWoFlags := c.Flags().Args()
	if c.DisableFlagParsing {
		argWoFlags = a
	}

	if err := c.ValidateArgs(argWoFlags); err != nil {
		return err
	}

I read the source code of cli which is using cobra but it doesn't appear this problem.The related code is here. Although cli and kubectl （by add Run function ） avoids this problem successfully, but I think the root cause is from cobra.

I think a command shouldn't accept any args if it has subcommand，or it's easy to confuse args and subcommand string😀. Finally,Please forgive me for my bad English!

[donggangcj]

Ref #842.

Thanks!

[marckhouzam]

I read the source code of cli which is using cobra but it doesn't appear this problem.

Actually, it looks like they hack their way around this problem: https://github.com/cli/cli/blob/1ddb4d76a750748012c91204740d49991ad2f968/cmd/gh/main.go#L59 and https://github.com/cli/cli/blob/1ddb4d76a750748012c91204740d49991ad2f968/command/help.go#L69-L73

[github-actions[bot]]

This PR is being marked as stale due to a long period of inactivity

[github-actions[bot]]

This PR is being marked as stale due to a long period of inactivity