-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
fix(args): Correct legacyArgs
logical errors
#1157
base: main
Are you sure you want to change the base?
Conversation
Close spf13#1156 Signed-off-by: Dong Gang <dong.gang@daocloud.io>
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. |
@marckhouzam I know you've done a lot of work on helm and could you please review this pr if you are free.😀 |
Ref #842. |
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 Lines 627 to 630 in 675ae5f
However, |
Signed-off-by: Dong Gang <dong.gang@daocloud.io>
@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:
After the PR:
The function I continue to view the code logic and find the root cause of problem. Lines 804 to 817 in 675ae5f
If switch the order of part1 and part2 , everythings will be ok!
I read the source code of cli which is using I think a command shouldn't accept any args if it has subcommand,or it's easy to confuse |
Thanks! |
Actually, it looks like they hack their way around this problem: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, but I have concerns about backwards compatibility.
If those concerns are valid, here is another idea.
There was another suggestion on how to fix this here:
#1056 (comment)
But in the end if was not done:
#1062 (comment)
and
#1062 (comment)
However, you may be able to revive that approach, as long not specifying a sub-command continued to not return an error. So something like
helm repo badcommand
would return an error
but
helm repo
would not.
This is just a suggestion, but I am not sure it would get accepted either.
// root command with subcommands, do subcommand checking. | ||
if !cmd.HasParent() && len(args) > 0 { | ||
// root command with subcommands, the args should be empty. | ||
if cmd.HasSubCommands() && len(args) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot make this change as it will break backwards-compatibility.
Cobra does support commands that take args and sub-commands.
For example helm v2 has the helm get
command:
Usage:
helm get [flags] RELEASE_NAME
helm get [command]
As you can see you can either pass it a sub-command, or you can pass it an argument.
As you mention, this can cause confusion so helm v3 no longer does that.
However it shows that some programs do use this feature and therefore we cannot change it.
@@ -816,6 +810,12 @@ func (c *Command) execute(a []string) (err error) { | |||
return err | |||
} | |||
|
|||
if !c.Runnable() { | |||
return flag.ErrHelp | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this after the parsing of the arguments seems right to me. The change will only impact commands that are not runnable which have the Args
field defined. The change means that the Args
field will be respected when it used to not be. I believe this can be justified as a change.
return flag.ErrHelp | ||
} | ||
|
||
c.preRun() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more worrisome. I don't understand enough about preRun()
and initializers
to know if a program would expect the preRun()
to be executed even if the call to Args
fails with an error, which used to happen before but will not with this change.
This could be a backwards-compatibility issue.
@jharshman do you think this could break backwards-compatibility?
This PR is being marked as stale due to a long period of inactivity |
This PR is being marked as stale due to a long period of inactivity |
Close #1156
Signed-off-by: Dong Gang dong.gang@daocloud.io