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

feat(cli): validate --output flag and refactor #13695

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

MasonM
Copy link
Contributor

@MasonM MasonM commented Oct 3, 2024

Motivation

This is a follow-up to #13656 (review). Many of the CLI commands take an --output flag, but this wasn't being validated, and there was a lot of duplication. The duplication has led to docs getting out-of-date, e.g. argo cluster-template get --help says Output format. Must be one of: json|yaml|wide, but it accepts name too.

Modifications

This introduces a simple EnumFlagValue type to represent enum flags like --output and changes all commands with an --output flag to use it. This will cause Cobra to give a helpful error if you pass an invalid value, e.g.:

$ ./dist/argo list -o INVALID
Error: invalid argument "INVALID" for "-o, --output" flag: One of: name|json|yaml|wide

Verification

Ran E2E tests and manually ran a few commands with ./dist/argo

This is a follow-up to
argoproj#13656 (review).
Many of the CLI commands take an `--output` flag, but this wasn't
being validated, and there was a lot of duplication. The duplication has
led to docs getting out-of-date, e.g. `argo cluster-template get --help`
says `Output format. Must be one of: json|yaml|wide`, but it accepts
`name` too.

This introduces a simple `EnumFlagValue` type to represent enum flags
like `--output` and changes all commands with an `--output` flag to use
it. This will cause Cobra to give a helpful error if you pass an invalid
value, e.g.:
```
$ ./dist/argo list -o INVALID
Error: invalid argument "INVALID" for "-o, --output" flag: One of: name|json|yaml|wide
```

Signed-off-by: Mason Malone <[email protected]>
@MasonM MasonM marked this pull request as ready for review October 3, 2024 01:37
@MasonM MasonM changed the title refactor(cli): consolidate and fix --output validation feat(cli): validate --output flag and refactor Oct 3, 2024
@isubasinghe
Copy link
Member

Ah @MasonM I think merging the previous PR introduced a bunch of merge conflicts, sorry about this but could you please address these? Thanks

@MasonM
Copy link
Contributor Author

MasonM commented Oct 4, 2024

@isubasinghe Sure, done.

@MasonM
Copy link
Contributor Author

MasonM commented Oct 4, 2024

Looks like E2E Tests (test-cli, mysql, true) failed because it couldn't download k3s:

2024-10-04T01:11:55.0066282Z [INFO]  Downloading hash https://github.com/k3s-io/k3s/releases/download/v1.31.0+k3s1/sha256sum-amd64.txt
2024-10-04T01:14:10.5622240Z [ERROR]  Download failed

@isubasinghe
Copy link
Member

/retest

@@ -28,7 +28,7 @@ argo archive list [flags]
```
--chunk-size int Return large lists in chunks rather than all at once. Pass 0 to disable.
-h, --help help for list
-o, --output string Output format. One of: json|yaml|wide (default "wide")
-o, --output string Output format. One of: name|json|yaml|wide (default "wide")
Copy link
Member

Choose a reason for hiding this comment

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

I was a bit confused initially but this makes sense, allows for code reuse.

@isubasinghe isubasinghe enabled auto-merge (squash) October 11, 2024 05:28
@isubasinghe isubasinghe merged commit f1347b6 into argoproj:main Oct 11, 2024
28 checks passed
@agilgur5 agilgur5 added the area/cli The `argo` CLI label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli The `argo` CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants