From bcbe415288142fe93e1f9dc6ad937226894d4db0 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Tue, 6 Oct 2026 11:50:53 +1100 Subject: [PATCH] fix(spx): plain output when stdout is not a terminal, NO_COLOR or TERM=dumb (#1170) --- cmd/spinifex/cmd/export_test.go | 7 ++ cmd/spinifex/cmd/root.go | 27 +++++ cmd/spinifex/cmd/root_output_test.go | 149 +++++++++++++++++++++++++++ go.mod | 2 +- spinifex/utils/download_test.go | 107 +++++++++++++++++++ spinifex/utils/utils.go | 21 ++++ 6 files changed, 312 insertions(+), 1 deletion(-) create mode 100644 cmd/spinifex/cmd/root_output_test.go create mode 100644 spinifex/utils/download_test.go diff --git a/cmd/spinifex/cmd/export_test.go b/cmd/spinifex/cmd/export_test.go index 9a22769ba..55bdeb317 100644 --- a/cmd/spinifex/cmd/export_test.go +++ b/cmd/spinifex/cmd/export_test.go @@ -29,6 +29,13 @@ func SetFirewallApplyHelper(path string) func() { // InitCLILogger exposes initCLILogger for testing. var InitCLILogger = initCLILogger +// OutputStylingEnabled, IsTerminal and SetOutputStyling expose the spx colour guard. +var ( + OutputStylingEnabled = outputStylingEnabled + IsTerminal = isTerminal + SetOutputStyling = setOutputStyling +) + // CLILogLevel exposes the CLI logger's level var for testing. var CLILogLevel = cliLogLevel diff --git a/cmd/spinifex/cmd/root.go b/cmd/spinifex/cmd/root.go index 50cf832a8..100eaf553 100644 --- a/cmd/spinifex/cmd/root.go +++ b/cmd/spinifex/cmd/root.go @@ -2,12 +2,15 @@ package cmd import ( "fmt" + "io" "log/slog" "os" "github.com/mulgadc/spinifex/spinifex/config" + "github.com/pterm/pterm" "github.com/spf13/cobra" "github.com/spf13/viper" + "golang.org/x/term" ) var ( @@ -50,6 +53,8 @@ func Execute() { cliLogLevel.Set(slog.LevelError) slog.SetDefault(slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: cliLogLevel}))) + setOutputStyling(outputStylingEnabled(os.Getenv("NO_COLOR"), os.Getenv("TERM"), isTerminal(os.Stdout))) + err := rootCmd.Execute() if err != nil { fmt.Fprintln(os.Stderr, err) @@ -57,6 +62,28 @@ func Execute() { } } +// outputStylingEnabled reports whether spx may emit ANSI colour and text styling: +// only on a terminal that is not TERM=dumb, with NO_COLOR unset or empty. +func outputStylingEnabled(noColor, termName string, stdoutIsTerminal bool) bool { + return noColor == "" && termName != "dumb" && stdoutIsTerminal +} + +// isTerminal reports whether w is a file attached to a terminal. +func isTerminal(w io.Writer) bool { + f, ok := w.(*os.File) + return ok && term.IsTerminal(int(f.Fd())) +} + +// setOutputStyling switches pterm's colour and bold/underline styling for every +// printer at once. pterm forces colour on at init regardless of the terminal. +func setOutputStyling(enabled bool) { + if enabled { + pterm.EnableStyling() + } else { + pterm.DisableStyling() + } +} + // initCLILogger retunes the CLI logger from SPX_LOG_LEVEL, then --verbose so an // explicit flag wins over the environment. There is no root --debug flag: four // service subcommands already define one with a service-specific meaning, and a diff --git a/cmd/spinifex/cmd/root_output_test.go b/cmd/spinifex/cmd/root_output_test.go new file mode 100644 index 000000000..161d700c1 --- /dev/null +++ b/cmd/spinifex/cmd/root_output_test.go @@ -0,0 +1,149 @@ +package cmd_test + +import ( + "bytes" + "io" + "os" + "strings" + "testing" + + "github.com/mulgadc/spinifex/cmd/spinifex/cmd" + "github.com/pterm/pterm" +) + +func TestOutputStylingEnabled(t *testing.T) { + tests := []struct { + name string + noColor string + term string + terminal bool + want bool + }{ + {name: "terminal keeps colour", term: "xterm-256color", terminal: true, want: true}, + {name: "unset TERM on a terminal keeps colour", terminal: true, want: true}, + {name: "TERM=dumb drops colour on a terminal", term: "dumb", terminal: true, want: false}, + {name: "pipe drops colour", terminal: false, want: false}, + {name: "NO_COLOR drops colour on a terminal", noColor: "1", terminal: true, want: false}, + {name: "any non-empty NO_COLOR counts, even 0", noColor: "0", terminal: true, want: false}, + {name: "NO_COLOR on a pipe", noColor: "1", terminal: false, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := cmd.OutputStylingEnabled(tt.noColor, tt.term, tt.terminal); got != tt.want { + t.Fatalf("OutputStylingEnabled(%q, %q, %v) = %v, want %v", tt.noColor, tt.term, tt.terminal, got, tt.want) + } + }) + } +} + +func TestIsTerminalFalseForNonTTYWriters(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + defer r.Close() + defer w.Close() + + f, err := os.CreateTemp(t.TempDir(), "out") + if err != nil { + t.Fatal(err) + } + defer f.Close() + + for name, wr := range map[string]io.Writer{"pipe": w, "regular file": f, "buffer": &bytes.Buffer{}} { + if cmd.IsTerminal(wr) { + t.Errorf("IsTerminal(%s) = true, want false", name) + } + } +} + +// renderNodesTable renders a table shaped like `spx get nodes` through the same +// pterm.DefaultTable the CLI uses, after applying the styling decision. +func renderNodesTable(t *testing.T, noColor, termName string, terminal bool) string { + t.Helper() + t.Cleanup(func() { cmd.SetOutputStyling(true) }) + cmd.SetOutputStyling(cmd.OutputStylingEnabled(noColor, termName, terminal)) + + var buf bytes.Buffer + data := pterm.TableData{ + {"NAME", "STATUS", "IP"}, + {"node1", "Ready", "10.0.0.1"}, + {"node-long-name", "NotReady", "10.0.0.22"}, + } + if err := pterm.DefaultTable.WithHasHeader().WithLeftAlignment().WithData(data).WithWriter(&buf).Render(); err != nil { + t.Fatalf("render: %v", err) + } + return buf.String() +} + +func TestTableOutputPlainWhenNotTerminal(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + defer r.Close() + defer w.Close() + + out := renderNodesTable(t, "", "xterm", cmd.IsTerminal(w)) + assertPlainAlignedTable(t, out) +} + +func TestTableOutputPlainWithNoColorOnTerminal(t *testing.T) { + out := renderNodesTable(t, "1", "xterm", true) + assertPlainAlignedTable(t, out) +} + +func TestTableOutputPlainOnDumbTerminal(t *testing.T) { + out := renderNodesTable(t, "", "dumb", true) + assertPlainAlignedTable(t, out) +} + +func TestTableOutputStyledOnTerminal(t *testing.T) { + out := renderNodesTable(t, "", "xterm", true) + if !strings.Contains(out, "\x1b[") { + t.Fatalf("table on a terminal carries no ANSI styling, so the plain-output tests prove nothing:\n%q", out) + } +} + +func assertPlainAlignedTable(t *testing.T, out string) { + t.Helper() + if strings.ContainsRune(out, '\x1b') { + t.Fatalf("output contains an escape byte:\n%q", out) + } + + lines := strings.Split(strings.TrimRight(out, "\n"), "\n") + var rows []string + for _, l := range lines { + if strings.Contains(l, "node") || strings.HasPrefix(l, "NAME") { + rows = append(rows, l) + } + } + if len(rows) != 3 { + t.Fatalf("want header and two node rows, got %d:\n%s", len(rows), out) + } + + // Columns line up only if the width calculation saw the same bytes it printed. + statusCol := strings.Index(rows[0], "STATUS") + ipCol := strings.Index(rows[0], "IP") + for _, row := range rows[1:] { + fields := strings.Fields(row) + var status string + for _, f := range fields { + if strings.Contains(f, "Ready") { + status = f + } + } + if got := strings.Index(row, status); got != statusCol { + t.Errorf("STATUS column at %d in %q, header at %d", got, row, statusCol) + } + if got := strings.Index(row, "10.0.0."); got != ipCol { + t.Errorf("IP column at %d in %q, header at %d", got, row, ipCol) + } + } + + // grep -w Ready must match: the byte before the word is not a word character. + ready := strings.Index(rows[1], "Ready") + if ready <= 0 || rows[1][ready-1] != ' ' { + t.Errorf("Ready is not preceded by a space in %q", rows[1]) + } +} diff --git a/go.mod b/go.mod index 2ecf82e0a..29f263073 100644 --- a/go.mod +++ b/go.mod @@ -48,6 +48,7 @@ require ( golang.org/x/crypto v0.57.0 golang.org/x/sync v0.23.0 golang.org/x/sys v0.48.0 + golang.org/x/term v0.46.0 golang.org/x/time v0.16.0 golang.org/x/tools v0.50.0 gopkg.in/ini.v1 v1.67.3 @@ -197,7 +198,6 @@ require ( golang.org/x/mod v0.41.0 // indirect golang.org/x/net v0.59.0 // indirect golang.org/x/telemetry v0.0.0-20260908163034-4bcc4b2ee518 // indirect - golang.org/x/term v0.46.0 // indirect golang.org/x/text v0.42.0 // indirect golang.org/x/vuln v1.6.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260825221802-da73d73af1c5 // indirect diff --git a/spinifex/utils/download_test.go b/spinifex/utils/download_test.go new file mode 100644 index 000000000..91cbbb594 --- /dev/null +++ b/spinifex/utils/download_test.go @@ -0,0 +1,107 @@ +package utils + +import ( + "bytes" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/pterm/pterm" + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" +) + +// captureFD1 redirects file descriptor 1 for the duration of fn, so it also +// catches writers that captured os.Stdout at init (pterm's cursor and colour). +func captureFD1(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + require.NoError(t, err) + saved, err := unix.Dup(1) + require.NoError(t, err) + + done := make(chan []byte) + go func() { + b, _ := io.ReadAll(r) + done <- b + }() + + func() { + defer func() { + require.NoError(t, unix.Dup2(saved, 1)) + _ = unix.Close(saved) + _ = w.Close() + }() + require.NoError(t, unix.Dup2(int(w.Fd()), 1)) + fn() + }() + + out := <-done + _ = r.Close() + return string(out) +} + +func TestDownloadFileWithProgressPlainWhenStylingDisabled(t *testing.T) { + wasRaw := pterm.RawOutput + t.Cleanup(func() { + if wasRaw { + pterm.DisableStyling() + } else { + pterm.EnableStyling() + } + }) + pterm.DisableStyling() + + const chunk = 32 * 1024 + const chunks = 64 + payload := bytes.Repeat([]byte("spinifex"), chunk*chunks/8) + + tests := []struct { + name string + contentLength bool + wantStart string + }{ + {name: "content length", contentLength: true, wantStart: "Downloading img (2.0 MiB) ..."}, + {name: "chunked without content length", contentLength: false, wantStart: "Downloading img (size unknown) ..."}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if tt.contentLength { + w.Header().Set("Content-Length", strconv.Itoa(len(payload))) + } + flusher, ok := w.(http.Flusher) + if !ok { + t.Error("response writer cannot flush") + return + } + for off := 0; off < len(payload); off += chunk { + _, _ = w.Write(payload[off : off+chunk]) + flusher.Flush() + } + })) + defer srv.Close() + + dest := filepath.Join(t.TempDir(), "img.raw") + var dlErr error + out := captureFD1(t, func() { + dlErr = DownloadFileWithProgress(srv.URL, "img", dest, 0) + }) + require.NoError(t, dlErr) + + got, err := os.ReadFile(dest) + require.NoError(t, err) + require.True(t, bytes.Equal(payload, got), "downloaded file differs from served payload") + + require.NotContains(t, out, "\x1b", "plain output carries an escape sequence") + require.NotContains(t, out, "\r", "plain output redraws a line") + lines := strings.Split(strings.TrimRight(out, "\n"), "\n") + require.Equal(t, []string{tt.wantStart, "Downloaded img (2.0 MiB)"}, lines) + }) + } +} diff --git a/spinifex/utils/utils.go b/spinifex/utils/utils.go index 4b4ccabef..ed612d390 100644 --- a/spinifex/utils/utils.go +++ b/spinifex/utils/utils.go @@ -309,6 +309,12 @@ func DownloadFileWithProgress(url string, name string, filename string, timeout cl := resp.ContentLength + // The bar and spinner write cursor escapes and per-write lines even with + // styling disabled, so plain output gets one start and one done line. + if pterm.RawOutput { + return downloadPlain(f, resp.Body, name, cl) + } + if cl > 0 { total := safecast.Int64ToUint64(cl) bar, update := NewByteProgressBar(fmt.Sprintf("Downloading %s", name), total) @@ -353,6 +359,21 @@ func DownloadFileWithProgress(url string, name string, filename string, timeout return nil } +func downloadPlain(dst io.Writer, src io.Reader, name string, contentLength int64) error { + size := "size unknown" + if contentLength > 0 { + size = HumanBytes(safecast.Int64ToUint64(contentLength)) + } + pterm.Printfln("Downloading %s (%s) ...", name, size) + + written, err := io.Copy(dst, src) + if err != nil { + return fmt.Errorf("copy error: %w", err) + } + pterm.Printfln("Downloaded %s (%s)", name, HumanBytes(safecast.Int64ToUint64(written))) + return nil +} + // progressWriter turns byte counts into a callback for UI updates. type progressWriter func(n int)