Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion pkg/cmd/config/validate/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ type Options struct {
Client func() (*http.Client, error)
Config func() (config.Config, error)

File string
File string
Output string
}

func NewCmdValidate(f *cmd.Factory) *cobra.Command {
Expand All @@ -44,6 +45,7 @@ func NewCmdValidate(f *cmd.Factory) *cobra.Command {
if opts.File == "" {
return &cmdutil.FlagError{Err: fmt.Errorf("required flag \"file\" not set")}
}
opts.Output, _ = c.Flags().GetString("output")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="pkg/cmd/config/validate/validate.go"
echo "== file =="
wc -l "$file"
echo

echo "== lines 1-140 (for context) =="
sed -n '1,140p' "$file" | nl -ba

echo
echo "== lines around 35-60 =="
sed -n '35,70p' "$file" | nl -ba

echo
echo "== lines around 80-110 =="
sed -n '80,120p' "$file" | nl -ba

echo
echo "== find where --output/-o is defined (c.Flags().GetString(\"output\")) =="
rg -n "GetString\\(\"output\"\\)" pkg/cmd/config/validate/validate.go pkg/cmd/config/validate -S || true

rg -n "persistent.*--output|persistent.*output|--output\"|StringVarP\\(|String\\(\"output\"" pkg/cmd/config -S || true

echo
echo "== find validate command wiring =="
rg -n "NewCmd\\(|validate" pkg/cmd/config/validate/validate.go -S || true

Repository: api7/a7

Length of output: 197


🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="pkg/cmd/config/validate/validate.go"

echo "== file =="
wc -l "$file"
echo

echo "== lines 1-140 =="
sed -n '1,140p' "$file" | cat -n
echo

echo "== lines 35-70 =="
sed -n '35,70p' "$file" | cat -n
echo

echo "== lines 80-120 =="
sed -n '80,120p' "$file" | cat -n
echo

echo "== find GetString(\"output\") in this file/folder =="
rg -n "GetString\\(\"output\"\\)" pkg/cmd/config/validate/validate.go pkg/cmd/config/validate -S || true
echo

echo "== find where --output/-o is defined elsewhere =="
rg -n "persistent.*--output|persistent.*output|--output\"|StringVarP\\(|String\\(\"output\"" pkg/cmd/config -S || true
echo

echo "== validate command wiring =="
rg -n "NewCmd\\(|validate" pkg/cmd/config/validate/validate.go -S || true

Repository: api7/a7

Length of output: 9674


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== rg where output flag is defined across pkg/cmd =="

rg -n "StringVarP\\(&[^\\n]*opts\\.Output[^\\n]*,\\s*\"output\"\\s*,\\s*\"o\"" pkg/cmd -S || true
rg -n "StringVarP\\(&[^\\n]*,\\s*\"output\"\\s*,\\s*\"o\"" pkg/cmd -S || true
rg -n "Persistent.*StringVarP\\(.*\"output\".*\"o\"" pkg/cmd -S || true
rg -n "Persistent.*--output|--output\\b|\"output\"\\s*,\\s*\"o\"" pkg/cmd -S || true

echo
echo "== rg for GetString(\"output\") across pkg/cmd =="

rg -n "GetString\\(\"output\"\\)" pkg/cmd -S || true

Repository: api7/a7

Length of output: 8978


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate cmdutil.NewExporter implementation =="
fd -a "exporter" pkg/cmdutil pkg -t f || true
rg -n "func NewExporter" -S pkg/cmdutil pkg || true
rg -n "NewExporter\\(" -S pkg/cmdutil pkg/cmd || true

Repository: api7/a7

Length of output: 9085


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== root persistent --output definition =="
sed -n '1,120p' pkg/cmd/root/root.go | cat -n

echo
echo "== cmdutil/exporter.go =="
sed -n '1,220p' pkg/cmdutil/exporter.go | cat -n

echo
echo "== validate tests =="
ls -la pkg/cmd/config/validate || true
sed -n '1,200p' pkg/cmd/config/validate/validate_test.go | cat -n

Repository: api7/a7

Length of output: 13353


🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "newCmdWithOutputFlag\\(" pkg/cmd/config/validate/validate_test.go
rg -n "NewCmdValidate\\(" pkg/cmd/config/validate/validate_test.go

echo
echo "== show remaining part of validate_test.go around newCmdWithOutputFlag usage =="
sed -n '1,120p' pkg/cmd/config/validate/validate_test.go | cat -n
sed -n '120,260p' pkg/cmd/config/validate/validate_test.go | cat -n
sed -n '260,520p' pkg/cmd/config/validate/validate_test.go | cat -n

Repository: api7/a7

Length of output: 15736


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== validate.go output handling =="
rg -n "switch opts\\.Output|case \"json\"|case \"yaml\"|fmt\\.Fprintln\\(opts\\.IO\\.Out" pkg/cmd/config/validate/validate.go
sed -n '80,110p' pkg/cmd/config/validate/validate.go | cat -n

echo
echo "== config/dump.go output handling =="
sed -n '1,140p' pkg/cmd/config/dump/dump.go | cat -n
rg -n "opts\\.Output|NewExporter\\(" pkg/cmd/config/dump/dump.go

echo
echo "== config/diff.go output handling =="
sed -n '1,140p' pkg/cmd/config/diff/diff.go | cat -n
rg -n "opts\\.Output|switch opts\\.Output|NewExporter\\(" pkg/cmd/config/diff/diff.go

echo
echo "== other commands: how they handle invalid opts.Output =="
rg -n "switch opts\\.Output" pkg/cmd -S --glob='*.go'
rg -n "case \"json\", \"yaml\"|case \"json\".*case \"yaml\"" pkg/cmd -S --glob='*.go'

Repository: api7/a7

Length of output: 6821


🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "output.*(invalid|unsupported|toml|table|xml|yaml|json)" pkg/cmd/config/validate/validate_test.go -S || true
rg -n "\"-o\"|\"--output\"" pkg/cmd/config/validate/validate_test.go -S || true
rg -n "SuccessOutput|Output" pkg/cmd/config/validate/validate_test.go -S || true

Repository: api7/a7

Length of output: 1446


🏁 Script executed:

#!/bin/bash
set -euo pipefail

for f in \
  pkg/cmd/context/list/list.go \
  pkg/cmd/stream-route/list/list.go \
  pkg/cmd/service/list/list.go \
  pkg/cmd/route/list/list.go
do
  echo "== $f =="
  rg -n "GetString\\(\"output\"\\)" "$f" -n || true
  # show nearby output handling
  # print switch/if on opts.Output if present
  rg -n "switch opts\\.Output|if opts\\.Output|case \"json\"|case \"yaml\"" "$f" -S || true
  sed -n '1,140p' "$f" | cat -n
  echo
done

Repository: api7/a7

Length of output: 15128


Handle --output flag error and reject unsupported output values in config validate

  • Line 48: opts.Output, _ = c.Flags().GetString("output") discards the returned error; return it instead to follow the “never suppress errors” rule.
  • Lines 89-97: if opts.Output is non-empty but not json/yaml, the command silently falls back to printing the human message ("Config is valid"), instead of erroring (in contrast to other commands that route through cmdutil.NewExporter, which returns unsupported output format).
Proposed fix
-			opts.Output, _ = c.Flags().GetString("output")
+			output, err := c.Flags().GetString("output")
+			if err != nil {
+				return fmt.Errorf("failed to read output flag: %w", err)
+			}
+			opts.Output = output
🤖 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/config/validate/validate.go` at line 48, Change the code that reads
the --output flag to handle and return the error from
c.Flags().GetString("output") instead of discarding it (assign to opts.Output
and check err, returning err up the call chain). After reading opts.Output
validate its value: if opts.Output is non-empty, either construct/validate via
cmdutil.NewExporter(opts.Output) (and return its error) or explicitly reject
values other than "json" or "yaml" by returning an error like "unsupported
output format"; ensure the function (the validate command handler where
opts.Output is used) returns that error rather than silently falling back to the
human message.

return validateRun(opts)
},
}
Expand All @@ -53,6 +55,14 @@ func NewCmdValidate(f *cmd.Factory) *cobra.Command {
return c
}

// validateResult is emitted on the success path when -o json or -o yaml is
// requested. Keeping it small and stable means downstream automation can pipe
// `a7 config validate -o json | jq '.valid'` without parsing English.
type validateResult struct {
Valid bool `json:"valid" yaml:"valid"`
Message string `json:"message,omitempty" yaml:"message,omitempty"`
}

func validateRun(opts *Options) error {
data, err := readFile(opts.File)
if err != nil {
Expand All @@ -76,6 +86,13 @@ func validateRun(opts *Options) error {
return fmt.Errorf("config validation failed:\n- %s", strings.Join(errs, "\n- "))
}

switch opts.Output {
case "json", "yaml":
return cmdutil.NewExporter(opts.Output, opts.IO.Out).Write(validateResult{
Valid: true,
Message: "Config is valid",
})
}
Comment on lines +89 to +95
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Return an error for unsupported non-empty output values (Lines 89-95).

Right now unknown values (e.g. -o jsn) silently fall back to plaintext success, which can break automation expecting structured output.

Proposed fix
 	switch opts.Output {
 	case "json", "yaml":
 		return cmdutil.NewExporter(opts.Output, opts.IO.Out).Write(validateResult{
 			Valid:   true,
 			Message: "Config is valid",
 		})
+	case "", "table":
+		fmt.Fprintln(opts.IO.Out, "Config is valid")
+		return nil
+	default:
+		return fmt.Errorf("unsupported output format: %s", opts.Output)
 	}
-	fmt.Fprintln(opts.IO.Out, "Config is valid")
-	return nil
🤖 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/config/validate/validate.go` around lines 89 - 95, The switch on
opts.Output in the validate flow currently only handles "json" and "yaml" and
silently falls back for unknown non-empty values; update the logic around
opts.Output (the switch and its default path) to detect unsupported non-empty
outputs and return a non-nil error (e.g., using fmt.Errorf) instead of
proceeding to plaintext success, so callers relying on structured output get a
clear failure; keep using cmdutil.NewExporter(opts.Output,
opts.IO.Out).Write(validateResult{...}) for the supported "json"/"yaml" branches
and ensure validateResult remains used for the successful export.

fmt.Fprintln(opts.IO.Out, "Config is valid")
return nil
}
Expand Down
69 changes: 69 additions & 0 deletions pkg/cmd/config/validate/validate_test.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package validate

import (
"encoding/json"
"net/http"
"os"
"path/filepath"
"testing"

"github.com/spf13/cobra"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

Expand All @@ -14,6 +16,18 @@ import (
"github.com/api7/a7/pkg/iostreams"
)

// newCmdWithOutputFlag returns NewCmdValidate wrapped under a parent cobra.Command
// that registers the global --output persistent flag, matching how the root
// command in pkg/cmd/root/root.go wires things up. Unit tests need this so
// `c.SetArgs("-o", "json")` resolves the inherited flag.
func newCmdWithOutputFlag(t *testing.T, ios *iostreams.IOStreams) *cobra.Command {
t.Helper()
root := &cobra.Command{Use: "a7"}
root.PersistentFlags().StringP("output", "o", "", "Output format: json, yaml (default: table)")
root.AddCommand(NewCmdValidate(factoryWithIO(ios)))
return root
}

type mockConfig struct {
baseURL string
}
Expand Down Expand Up @@ -287,6 +301,61 @@ func TestConfigValidate_MissingFileFlag(t *testing.T) {
assert.Contains(t, err.Error(), "required flag \"file\" not set")
}

// TestConfigValidate_SuccessOutputJSON asserts that `-o json` on the success
// path emits a parseable JSON object rather than the human-readable string.
// Regression for the case where the CLI ignored -o on success and broke
// scripted callers piping into `jq`.
func TestConfigValidate_SuccessOutputJSON(t *testing.T) {
ios, _, stdout, _ := iostreams.Test()

filePath := filepath.Join(t.TempDir(), "config.yaml")
require.NoError(t, os.WriteFile(filePath, []byte("version: \"1\"\nservices: []\n"), 0o644))

root := newCmdWithOutputFlag(t, ios)
root.SetArgs([]string{"validate", "-f", filePath, "-o", "json"})
require.NoError(t, root.Execute())

var result struct {
Valid bool `json:"valid"`
Message string `json:"message"`
}
require.NoError(t, json.Unmarshal(stdout.Bytes(), &result), "stdout must be valid JSON, got: %s", stdout.String())
assert.True(t, result.Valid)
assert.Equal(t, "Config is valid", result.Message)
}

// TestConfigValidate_SuccessOutputYAML asserts the parallel YAML path.
func TestConfigValidate_SuccessOutputYAML(t *testing.T) {
ios, _, stdout, _ := iostreams.Test()

filePath := filepath.Join(t.TempDir(), "config.yaml")
require.NoError(t, os.WriteFile(filePath, []byte("version: \"1\"\nservices: []\n"), 0o644))

root := newCmdWithOutputFlag(t, ios)
root.SetArgs([]string{"validate", "-f", filePath, "-o", "yaml"})
require.NoError(t, root.Execute())

out := stdout.String()
assert.Contains(t, out, "valid: true")
assert.Contains(t, out, "message: Config is valid")
}

// TestConfigValidate_SuccessDefaultOutput keeps the existing
// "Config is valid" behavior for table / unset output, so users running the
// command interactively still see the friendly message.
func TestConfigValidate_SuccessDefaultOutput(t *testing.T) {
ios, _, stdout, _ := iostreams.Test()

filePath := filepath.Join(t.TempDir(), "config.yaml")
require.NoError(t, os.WriteFile(filePath, []byte("version: \"1\"\nservices: []\n"), 0o644))

root := newCmdWithOutputFlag(t, ios)
root.SetArgs([]string{"validate", "-f", filePath})
require.NoError(t, root.Execute())

assert.Contains(t, stdout.String(), "Config is valid")
}

// TestConfigValidate_EmptyUnsupportedSections asserts that declaring an
// unsupported top-level section (upstreams, consumer_groups, service_templates)
// is rejected even when the section is explicitly empty. Presence alone is
Expand Down
Loading