Repository navigation
fix: reject unsupported values for --output - #608
Conversation
--output was a plain string flag, so any value was accepted and the output code fell back to the human table. Make it a typed flag that only accepts json, human or custom, so a bad value fails while flags are parsed, before the config is loaded or any API call is made. Part of civo#398
fulviodenza
left a comment
There was a problem hiding this comment.
Thanks, this looks good. I checked that nothing that worked before breaks: across the codebase common.OutputFormat is only ever compared against json, human and custom, no subcommand defines its own -o, and every -o usage in the README/docs is json or custom. Values that are now rejected (JSON, yaml, table, empty) previously fell through silently to the table output, so erroring is the right behaviour. Help output still reads -o, --output string ... (default "human").
A couple of small things:
- Test depends on list order (
common/common_test.go,TestOutputFormatFlag): the rejection loop assertsformat == OutputFormatCustom, which only holds becausecustomhappens to be the last entry inOutputFormats. Capturing the value before the loop (e.g.before := format) and asserting it is unchanged would make the test robust to reordering. - Heads up on overlap with #599: both PRs touch the
common/common.goimport block and append to the end ofcommon/common_test.go, so whichever lands second will need a rebase. - Optional: registering completion for the flag (
RegisterFlagCompletionFunc("output", ...)returningOutputFormats) would be a nice touch, not needed for this PR.
I've approved the CI run; happy to approve once it is green and (1) is addressed.
|
@fulviodenza Thanks for the review! Pushed the test fix, it saves the value before the loop now and checks it stays the same. I'll rebase if #599 goes in first. Left completion out for now, I can do it in a separate PR. |
|
Awesome, LGTM 🚀 |
Part of #398
Problem
-otakes any string, and anything that isn'tjsonorcustomfalls through to the table output, so a typo likecivo kubernetes ls -o lefafajust prints the normal cluster table with no warning.Fix
--outputis now a typed flag (common.OutputFormatFlag) that only acceptsjson,humanorcustom. A bad value fails while cobra parses the flags, so it errors before the config is read or any API call is made, and no subcommand hook can skip it.common.OutputFormats, which also builds the help text. The help line is unchanged, so the copy in the README still matches.-o JSONis rejected too instead of silently printing the table.Tests
TestOutputFormatFlagincommon/common_test.goTestOutputFlagRejectsUnsupportedFormatsincmd/root_test.go, which checks the root command actually uses the typed flaggo test ./...and golangci-lint pass locallyI'll send the
--fieldcheck and thekubernetes createname handling as separate PRs, as discussed in the issue.