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.

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

Merged
merged 1 commit into from Nov 27, 2023

Conversation

VinGarcia
Copy link
Contributor

@VinGarcia VinGarcia commented Nov 15, 2023

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.

cmd/flux/install.go Outdated Show resolved Hide resolved
Copy link
Member

@makkes makkes left a 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,

@VinGarcia
Copy link
Contributor Author

@makkes just implemented your suggestion, thanks =]

@stefanprodan
Copy link
Member

@VinGarcia can you please squash all commits into one and rebase.

cmd/flux/install_test.go Outdated Show resolved Hide resolved
@VinGarcia
Copy link
Contributor Author

@stefanprodan and @makkes just squashed the commits, it should be ready now.

@makkes
Copy link
Member

makkes commented Nov 17, 2023

One thing that bothers me a little here is that this PR only fixes this for the install command but all the others stay the same. Running flux check foobar still succeeds while flux install foobar fails. I personally would like to strive for consistency here. @stefanprodan wdyt?

@VinGarcia
Copy link
Contributor Author

@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.

@VinGarcia
Copy link
Contributor Author

@makkes and @stefanprodan how should we proceed?

@stefanprodan
Copy link
Member

Rebase with the main branch and force push please.

@makkes
Copy link
Member

makkes commented Nov 21, 2023

Let's add a proper Args setting to each command.

@VinGarcia
Copy link
Contributor Author

@makkes I started going over all instances of cobra.Command and checking flux [command] --help for each of them, the ones with no args I will add Args: cobra.NoArgs, but some of them like cmd/flux/bootstrap_bitbucket_server.go do not have unit tests, so I have three options:

  1. Not add unit tests at all
  2. Only add tests to the files that already have unit tests
  3. Write unit tests for all files even the ones that don't have _test.go files already.

The last option might be hard for me, and I don't know how much important you guys think 2. is.

@stefanprodan
Copy link
Member

I don't think we need to test a builtin feature of cobra on each command. We can take this in for install. @VinGarcia if you feel up to it, please do bootstrap and others commands in a followup PR. To get this merged you have to rebase with upstream main branch and force push, this PR should contain a single commit.

@makkes
Copy link
Member

makkes commented Nov 23, 2023

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 cobra.NoArgs to the get ks command. I suggest it is desirable writing a unit test that verifies this doesn't happen.

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>
@VinGarcia
Copy link
Contributor Author

@makkes and @stefanprodan its rebased/squased into a single commit, I will continue the work on a separate PR then.

Thanks

@makkes makkes merged commit f20fe76 into fluxcd:main Nov 27, 2023
7 checks passed
@makkes
Copy link
Member

makkes commented Nov 27, 2023

Thanks @VinGarcia! 🤜🏻🤛🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants