fix(spx): plain output when stdout is not a terminal, NO_COLOR or TERM=dumb (#1170)

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
 

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

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])
+	}
+}

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

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)
+		})
+	}
+}

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)