-
Notifications
You must be signed in to change notification settings - Fork 614
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 flux install command so it returns an error when unexpected arguments are passed #4404
Conversation
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.
There's a much simpler solution here by using the Args
field of Command
:
var installCmd = &cobra.Command{
Args: cobra.NoArgs,
@makkes just implemented your suggestion, thanks =] |
530fe94
to
20137fe
Compare
@VinGarcia can you please squash all commits into one and rebase. |
e7272b4
to
251caad
Compare
@stefanprodan and @makkes just squashed the commits, it should be ready now. |
One thing that bothers me a little here is that this PR only fixes this for the |
@makkes I could add these changes on either this PR on a separate one. I would just need the list of commands on which to do it. |
@makkes and @stefanprodan how should we proceed? |
Rebase with the main branch and force push please. |
Let's add a proper |
@makkes I started going over all instances of
The last option might be hard for me, and I don't know how much important you guys think 2. is. |
I don't think we need to test a builtin feature of cobra on each command. We can take this in for |
There is a certain chance that adding the wrong PositionalArgs to a command would result in the command not being usable, anymore, e.g. when adding I agree with Stefan that adding PositionalArgs to each command should be made in a subsequent PR and we keep this one focussed on what's said in the title. |
…ents are passed Co-authored-by: Max Jonas Werner <makkes@users.noreply.github.com> Signed-off-by: Vinícius Garcia <vingarcia00@gmail.com>
@makkes and @stefanprodan its rebased/squased into a single commit, I will continue the work on a separate PR then. Thanks |
Thanks @VinGarcia! 🤜🏻🤛🏻 |
This PR was implemented to address the issue reported by myself on #4403.
I made sure that it is working on the cases where there are only flag arguments, or no arguments.
I wanted to add a test for a successful run, but when I tried I actually got an "install failed" error. So I figured that maybe there was a reason why no successful tests were present.
I can add more tests, but I would need some guidance.