fix(cmdutil): standardize -o output-format dispatch across list commands#57
Merged
Merged
Conversation
Two inconsistencies in how list commands handle the global --output flag:
1. `a7 gateway-group list -o totally-not-a-format` accepted any value and
silently fell back to table rendering. Typos (e.g. `-o jzon`) gave no
feedback. `plugin list` had the same problem.
2. `a7 service list -o table` rejected the explicit value with
`Error: unsupported output format: table`, even though `table` is the
documented default and works when -o is omitted. Every list command
except gateway-group and plugin had this problem (9 commands).
Centralize the dispatch in cmdutil:
- `cmdutil.ValidateOutputFormat` accepts {empty, table, json, yaml} and
rejects anything else with a message listing the valid set.
- `cmdutil.IsStructuredOutput` returns true only for json/yaml so each
list command can decide between exporter and table renderer.
Every list command (12 files: consumer, context, credential, gateway-group,
global-rule, plugin, proto, route, secret, service, ssl, stream-route) now:
- Validates --output at the start of RunE (fails fast on typos).
- Dispatches via `cmdutil.IsStructuredOutput(opts.Output)` so explicit
`-o table` falls through to the table renderer like the empty default.
Adds unit tests for both helpers. Existing tests continue to pass.
📝 WalkthroughWalkthroughThis PR adds centralized output format validation and detection logic, then applies it uniformly across twelve list subcommands. Two new utility functions in ChangesOutput format validation and refactoring
🎯 2 (Simple) | ⏱️ ~12 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cmd/gateway-group/list/list.go (1)
72-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply
label=valuefiltering before the structured-output return.Line 72 returns early for JSON/YAML, so the value-side label filter (Line 86–Line 94) is skipped.
--label key=value -o json|yamlcurrently returns unfiltered-by-value results.Proposed fix
- if cmdutil.IsStructuredOutput(opts.Output) { - exp := cmdutil.NewExporter(opts.Output, opts.IO.Out) - var result api.ListResponse[api.GatewayGroup] - if err := json.Unmarshal(body, &result); err != nil { - return err - } - return exp.Write(result.List) - } - var result api.ListResponse[api.GatewayGroup] if err := json.Unmarshal(body, &result); err != nil { return err } if labelValue != "" { filtered := make([]api.GatewayGroup, 0) for _, item := range result.List { if item.Labels != nil && item.Labels[labelKey] == labelValue { filtered = append(filtered, item) } } result.List = filtered } + + if cmdutil.IsStructuredOutput(opts.Output) { + exp := cmdutil.NewExporter(opts.Output, opts.IO.Out) + return exp.Write(result.List) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cmd/gateway-group/list/list.go` around lines 72 - 79, The structured-output branch returns early after unmarshalling (the block using cmdutil.IsStructuredOutput, cmdutil.NewExporter, json.Unmarshal and exp.Write), which skips the value-side label filtering logic later; move or replicate the label=value filtering to run immediately after json.Unmarshal (before exp.Write) so the exported result.List is filtered the same way as the non-structured path—apply the same predicate used in the existing label-filtering code (the logic around the value-side label filter currently at lines 86–94) to result.List before calling exp.Write, or extract that predicate into a helper and reuse it here.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cmd/consumer/list/list.go`:
- Around line 42-47: The calls to c.Flags().GetString for opts.Output,
opts.GatewayGroup and opts.Label currently ignore errors; update the code in the
function that sets these fields so you capture and handle the returned error
from each GetString call (e.g., err := c.Flags().GetString("output"); if err !=
nil { return err }) before calling cmdutil.ValidateOutputFormat(opts.Output),
and likewise for "gateway-group" and "label" — return or propagate any flag
parsing error instead of discarding it to avoid silently using zero values.
---
Outside diff comments:
In `@pkg/cmd/gateway-group/list/list.go`:
- Around line 72-79: The structured-output branch returns early after
unmarshalling (the block using cmdutil.IsStructuredOutput, cmdutil.NewExporter,
json.Unmarshal and exp.Write), which skips the value-side label filtering logic
later; move or replicate the label=value filtering to run immediately after
json.Unmarshal (before exp.Write) so the exported result.List is filtered the
same way as the non-structured path—apply the same predicate used in the
existing label-filtering code (the logic around the value-side label filter
currently at lines 86–94) to result.List before calling exp.Write, or extract
that predicate into a helper and reuse it here.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86d82dbf-9b8b-4ec5-a920-e214e08cce5c
📒 Files selected for processing (14)
pkg/cmd/consumer/list/list.gopkg/cmd/context/list/list.gopkg/cmd/credential/list/list.gopkg/cmd/gateway-group/list/list.gopkg/cmd/global-rule/list/list.gopkg/cmd/plugin/list/list.gopkg/cmd/proto/list/list.gopkg/cmd/route/list/list.gopkg/cmd/secret/list/list.gopkg/cmd/service/list/list.gopkg/cmd/ssl/list/list.gopkg/cmd/stream-route/list/list.gopkg/cmdutil/exporter.gopkg/cmdutil/exporter_test.go
Comment on lines
42
to
47
| opts.Output, _ = c.Flags().GetString("output") | ||
| if err := cmdutil.ValidateOutputFormat(opts.Output); err != nil { | ||
| return err | ||
| } | ||
| opts.GatewayGroup, _ = c.Flags().GetString("gateway-group") | ||
| opts.Label, _ = c.Flags().GetString("label") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Handle flag parsing errors instead of discarding them.
Line 42, Line 46, and Line 47 ignore GetString errors. If flag registration changes, this silently falls back to zero values and masks the root cause.
Proposed fix
- opts.Output, _ = c.Flags().GetString("output")
+ var err error
+ opts.Output, err = c.Flags().GetString("output")
+ if err != nil {
+ return err
+ }
if err := cmdutil.ValidateOutputFormat(opts.Output); err != nil {
return err
}
- opts.GatewayGroup, _ = c.Flags().GetString("gateway-group")
- opts.Label, _ = c.Flags().GetString("label")
+ opts.GatewayGroup, err = c.Flags().GetString("gateway-group")
+ if err != nil {
+ return err
+ }
+ opts.Label, err = c.Flags().GetString("label")
+ if err != nil {
+ return err
+ }
return actionRun(opts)As per coding guidelines, **/*.go: Never suppress errors. Always handle and propagate errors explicitly; and **/*.{js,ts,tsx,go}: every function return value must be checked for errors when applicable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/cmd/consumer/list/list.go` around lines 42 - 47, The calls to
c.Flags().GetString for opts.Output, opts.GatewayGroup and opts.Label currently
ignore errors; update the code in the function that sets these fields so you
capture and handle the returned error from each GetString call (e.g., err :=
c.Flags().GetString("output"); if err != nil { return err }) before calling
cmdutil.ValidateOutputFormat(opts.Output), and likewise for "gateway-group" and
"label" — return or propagate any flag parsing error instead of discarding it to
avoid silently using zero values.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes --output handling for list commands by centralizing validation and structured-output detection in cmdutil.
Changes:
- Adds
ValidateOutputFormatandIsStructuredOutputhelpers. - Updates list commands to reject invalid output formats early and treat explicit
-o tableas table output. - Adds unit coverage for the new
cmdutilhelpers.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pkg/cmdutil/exporter.go |
Adds shared output-format validation and structured-output detection helpers. |
pkg/cmdutil/exporter_test.go |
Adds helper-level tests for valid/invalid formats and structured output detection. |
pkg/cmd/consumer/list/list.go |
Uses shared validation and structured-output dispatch. |
pkg/cmd/context/list/list.go |
Uses shared validation and structured-output dispatch. |
pkg/cmd/credential/list/list.go |
Uses shared validation and structured-output dispatch. |
pkg/cmd/gateway-group/list/list.go |
Uses shared validation and structured-output dispatch. |
pkg/cmd/global-rule/list/list.go |
Uses shared validation and structured-output dispatch. |
pkg/cmd/plugin/list/list.go |
Uses shared validation and enables YAML structured output. |
pkg/cmd/proto/list/list.go |
Uses shared validation and structured-output dispatch. |
pkg/cmd/route/list/list.go |
Uses shared validation and structured-output dispatch. |
pkg/cmd/secret/list/list.go |
Uses shared validation and structured-output dispatch. |
pkg/cmd/service/list/list.go |
Uses shared validation and structured-output dispatch. |
pkg/cmd/ssl/list/list.go |
Uses shared validation and structured-output dispatch. |
pkg/cmd/stream-route/list/list.go |
Uses shared validation and structured-output dispatch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+65
to
+95
| func TestValidateOutputFormat(t *testing.T) { | ||
| for _, ok := range []string{"", "table", "json", "yaml"} { | ||
| if err := ValidateOutputFormat(ok); err != nil { | ||
| t.Errorf("expected %q to be accepted, got %v", ok, err) | ||
| } | ||
| } | ||
| for _, bad := range []string{"jzon", "yml", "TABLE", "csv", "totally-not-a-format"} { | ||
| err := ValidateOutputFormat(bad) | ||
| if err == nil { | ||
| t.Errorf("expected %q to be rejected", bad) | ||
| continue | ||
| } | ||
| if !strings.Contains(err.Error(), "valid: table, json, yaml") { | ||
| t.Errorf("error for %q should list the valid set, got: %v", bad, err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestIsStructuredOutput(t *testing.T) { | ||
| cases := map[string]bool{ | ||
| "": false, | ||
| "table": false, | ||
| "json": true, | ||
| "yaml": true, | ||
| } | ||
| for format, want := range cases { | ||
| if got := IsStructuredOutput(format); got != want { | ||
| t.Errorf("IsStructuredOutput(%q) = %v, want %v", format, got, want) | ||
| } | ||
| } | ||
| } |
shreemaan-abhishek
added a commit
that referenced
this pull request
May 27, 2026
…gaps The first CI run on this branch surfaced two problems: 1. The suite ran inside the regular `make test-e2e` target. That target is the existing CI gate; bringing 290+ permutation cases into it both inflates the run time and tied PR merge to the suite reporting clean — which it does not yet, because three real CLI inconsistencies are still open (fixed in PR #56 and PR #57). The permutation matrix is meant to be a manual triage tool, not a CI gate. Fix: tag the Describe with Label("permutation") and update test-e2e / test-e2e-full to skip it via --label-filter='!permutation'. The dedicated test-e2e-permutation target inverts the filter to include only the permutation specs. Net effect: regular CI no longer sees the suite; `make test-e2e-permutation` is unchanged. 2. The CI EE rejected `stream-route create` with `Error: API error (status 400): can not create a Stream Route to the HTTP Service`. Stream routes need an L4 service on that deployment; my local EE accepted them attached to an HTTP service. The walker then cascaded 7 failures (get / list / export / delete all error with "resource not found"). Fix: detect known capability-gap stderr patterns after the create step and downgrade the rest of the walker to a single informational "skipped" record. Patterns cover stream-route on HTTP-only EEs plus the existing secret-provider / vault gaps already handled by local_stability. The walker now also stops short on any create failure (capability gap or otherwise) rather than producing cascade noise from "resource not found" on every follow-up step.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two inconsistencies in how list commands handled the global `--output` flag, both surfaced by the new combinatorial permutation suite (separate follow-up PR):
Approach
Centralize the dispatch in `cmdutil`:
Every list command (12 files: consumer, context, credential, gateway-group, global-rule, plugin, proto, route, secret, service, ssl, stream-route) now:
Behavior matrix
Test plan
Summary by CodeRabbit
--outputflag validation across all list commands to reject invalid formats with clear error messages early in execution.