diff --git a/pkg/cli/ui/errorhandler/executor.go b/pkg/cli/ui/errorhandler/executor.go index 008c57eb3e..820f5638b9 100644 --- a/pkg/cli/ui/errorhandler/executor.go +++ b/pkg/cli/ui/errorhandler/executor.go @@ -2,6 +2,8 @@ package errorhandler import ( "bytes" + "errors" + "io" "strings" "github.com/spf13/cobra" @@ -37,9 +39,20 @@ func (e *Executor) Execute(cmd *cobra.Command) error { err := cmd.Execute() if err == nil { + flushWarnings(originalErrWriter, &errBuf) + return nil } + // A custom exit-code result (e.g. DriftExitError from `cluster diff --exit-code`) + // is a valid, non-failing outcome: main.go surfaces it as a process exit code and + // prints no error, so any warnings the run captured would otherwise be lost. Flush + // them like the success path. Genuine failures instead surface their stderr via the + // normalized message that main.go prints, so they are not flushed here. + if isExitCodeResult(err) { + flushExitCodeWarnings(originalErrWriter, &errBuf, err) + } + message := normalize(errBuf.String()) return &CommandError{ @@ -48,6 +61,47 @@ func (e *Executor) Execute(cmd *cobra.Command) error { } } +// flushWarnings forwards any stderr a non-failing run captured to the real writer, so +// warnings and notices reach the user. The capture exists only to normalize a failing +// command's stderr into the returned error; an empty buffer is a no-op, so commands +// that write nothing are unaffected. +func flushWarnings(w io.Writer, buf *bytes.Buffer) { + if buf.Len() > 0 { + _, _ = w.Write(buf.Bytes()) + } +} + +// flushExitCodeWarnings forwards the stderr an exit-code result captured, minus the +// automatic "Error: " line Cobra appends when a command leaves SilenceErrors unset +// (e.g. `cluster diff`, which sets only SilenceUsage). main.go surfaces an exit-code +// result as a process exit code and prints no error of its own, so replaying Cobra's +// error line here would be spurious noise; the command's real warnings still reach the +// user. Cobra writes the error last (SilenceUsage suppresses the trailing usage) as +// `fmt.Fprintln(w, "Error:", err.Error())`, i.e. exactly "Error: \n". +func flushExitCodeWarnings(writer io.Writer, buf *bytes.Buffer, err error) { + out := buf.Bytes() + + if err != nil { + cobraError := "Error: " + err.Error() + out = bytes.TrimSuffix(out, []byte(cobraError+"\n")) + out = bytes.TrimSuffix(out, []byte(cobraError)) + } + + if len(out) > 0 { + _, _ = writer.Write(out) + } +} + +// isExitCodeResult reports whether err carries a custom KSail exit code — a valid, +// non-failing outcome (e.g. drift detected) rather than a command failure. It mirrors +// the structural interface main.go uses to translate such errors into process exit +// codes, without coupling this package to specific command types. +func isExitCodeResult(err error) bool { + var exitCoder interface{ KSailExitCode() int } + + return errors.As(err, &exitCoder) +} + // CommandError type. // CommandError represents a Cobra execution failure augmented with normalized stderr output. diff --git a/pkg/cli/ui/errorhandler/executor_test.go b/pkg/cli/ui/errorhandler/executor_test.go index e170168475..5bd2a7fca0 100644 --- a/pkg/cli/ui/errorhandler/executor_test.go +++ b/pkg/cli/ui/errorhandler/executor_test.go @@ -1,8 +1,11 @@ package errorhandler_test import ( + "bytes" "errors" + "fmt" "os" + "strings" "testing" snapshottest "github.com/devantler-tech/ksail/v7/internal/testutil/snapshottest" @@ -40,6 +43,126 @@ func TestExecutorExecuteSuccess(t *testing.T) { } } +// TestExecutorExecuteFlushesWarningsOnSuccess guards against silently swallowing +// warnings: the executor captures stderr to normalize a failing command's error, +// but a command that SUCCEEDS while writing a warning to stderr must still have +// that warning reach the real stderr — a regression guard for warnings being +// discarded on the success path. +func TestExecutorExecuteFlushesWarningsOnSuccess(t *testing.T) { + t.Parallel() + + var stderr bytes.Buffer + + cmd := &cobra.Command{ + Use: "test", + RunE: func(c *cobra.Command, _ []string) error { + _, _ = fmt.Fprintln(c.ErrOrStderr(), "heads up: incomplete values") + + return nil + }, + } + cmd.SetErr(&stderr) + + executor := errorhandler.NewExecutor() + + err := executor.Execute(cmd) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if got := stderr.String(); !strings.Contains(got, "heads up: incomplete values") { + t.Fatalf("expected the success-path warning to reach stderr, got %q", got) + } +} + +// exitCodeError is a test error carrying a custom KSail exit code, mirroring +// DriftExitError: a valid, non-failing outcome rather than a command failure. +type exitCodeError struct{ code int } + +func (e *exitCodeError) Error() string { return fmt.Sprintf("exit code %d", e.code) } + +func (e *exitCodeError) KSailExitCode() int { return e.code } + +// TestExecutorExecuteFlushesWarningsOnExitCodeResult guards the custom-exit-code path +// (e.g. `cluster diff --exit-code` detecting drift): main.go surfaces such results as a +// process exit code and prints no error, so a warning the run wrote to stderr must still +// reach the real stderr rather than being swallowed with the captured buffer. It also +// asserts the custom exit code stays detectable via errors.As so main.go keeps propagating +// it. +func TestExecutorExecuteFlushesWarningsOnExitCodeResult(t *testing.T) { + t.Parallel() + + var stderr bytes.Buffer + + cmd := &cobra.Command{ + Use: "test", + SilenceErrors: true, + SilenceUsage: true, + RunE: func(c *cobra.Command, _ []string) error { + _, _ = fmt.Fprintln(c.ErrOrStderr(), "heads up: drift detected") + + return &exitCodeError{code: 2} + }, + } + cmd.SetErr(&stderr) + + executor := errorhandler.NewExecutor() + + err := executor.Execute(cmd) + if err == nil { + t.Fatal("expected the exit-code error to propagate, got nil") + } + + var coder interface{ KSailExitCode() int } + if !errors.As(err, &coder) { + t.Fatalf("expected the custom exit code to remain detectable via errors.As, got %T", err) + } + + if got := stderr.String(); !strings.Contains(got, "heads up: drift detected") { + t.Fatalf("expected the exit-code-path warning to reach stderr, got %q", got) + } +} + +// TestExecutorExecuteDropsCobraErrorOnExitCodeResult guards that the exit-code path does +// NOT replay Cobra's automatic "Error: " line. A command that leaves SilenceErrors +// unset (like `cluster diff`, which sets only SilenceUsage) has Cobra append that line to +// the captured stderr; main.go prints no error for an exit-code result, so surfacing +// Cobra's line would be spurious noise. The command's own warning must still reach stderr. +func TestExecutorExecuteDropsCobraErrorOnExitCodeResult(t *testing.T) { + t.Parallel() + + var stderr bytes.Buffer + + cmd := &cobra.Command{ + Use: "test", + // Mirror `cluster diff`: SilenceUsage set, SilenceErrors UNSET, so Cobra + // appends its own "Error: " line to the captured stderr. + SilenceUsage: true, + RunE: func(c *cobra.Command, _ []string) error { + _, _ = fmt.Fprintln(c.ErrOrStderr(), "heads up: drift detected") + + return &exitCodeError{code: 2} + }, + } + cmd.SetErr(&stderr) + + executor := errorhandler.NewExecutor() + + err := executor.Execute(cmd) + if err == nil { + t.Fatal("expected the exit-code error to propagate, got nil") + } + + got := stderr.String() + if !strings.Contains(got, "heads up: drift detected") { + t.Fatalf("expected the exit-code-path warning to reach stderr, got %q", got) + } + + if strings.Contains(got, "Error: exit code 2") { + t.Fatalf("Cobra's auto error line must not be replayed for exit-code results, got %q", got) + } +} + func TestExecutorExecuteNilCommand(t *testing.T) { t.Parallel()