Refactor output colorization and improve heartbeat and listener commands

CONTRIBUTING.md

@@ -33,3 +33,50 @@ And do not terminate with a dot.
 ## Do not capitalize pflag description string
 
 And do not terminate with a dot.
+
+# Commands
+
+## Name a command that shows things after what it renders
+
+`list`, aliased `ls`, when the answer is a row per thing: a flat table whose
+columns are selectable, that `-o json`, `-o tab` and `-o flat` reshape, and
+that a script reads. Most of the tree is this.
+
+`status` when the answer is a composed view of one subject, whose layout
+carries meaning that rows cannot: the resource tree of `om <object> instance
+status`, the node-column board of `om ccfg status`. Reformatting one of those
+as a table would lose information.
+
+The test is mechanical: a command whose renderer is a `tab=` column spec is a
+listing, whatever its subject. `om daemon hb status`, `om daemon relay status`
+and `om node relay status` were listings wearing the other name, and are
+`list` now.
+
+## Keep the name a renamed command answered to
+
+Give the new name to the command, and build the old one from it with `Use`
+overridden, the aliases cleared and `Hidden: true`. It stays out of the help
+and out of the completion, and the scripts that type it keep working.
+
+# Rendering
+
+## Do not use color.Set
+
+`color.Set` is the imperative form of the fatih/color api: it writes the escape
+sequence to the package output, which is the process stdout, as a side effect,
+and returns the color to render with. A function composing a string therefore
+leaves a bare, never reset sequence on the terminal ahead of whatever it
+returns, and the first line rendered afterwards inherits it. That is how a
+section holding a comment came out italic in `om <obj> config show`.
+
+Use `color.New`. It builds the same color and writes nothing, and its `Fprint`,
+`Sprint` and `SprintFunc` open and close the sequence around the text they
+render.
+
+## Paint where the output is
+
+An escape sequence belongs to the code writing to the terminal, not to the type
+carrying the value. A daemon type is published, read back by api clients and by
+the tui, which paints cells of its own: it hands the state over, and the
+renderer draws the icon standing for it.
+

core/commoncmd/config.go

@@ -128,13 +128,13 @@ func ColorizeINI(b []byte) []byte {
 
 		// Section header
 		if sectionRE.MatchString(line) {
-			color.Set(color.FgHiYellow, color.Bold).Fprintln(out, line)
+			color.New(color.FgHiYellow, color.Bold).Fprintln(out, line)
 			continue
 		}
 
 		// Comment
 		if commentRE.MatchString(line) {
-			color.Set(color.FgHiBlack, color.Italic).Fprintln(out, line)
+			color.New(color.FgHiBlack, color.Italic).Fprintln(out, line)
 			continue
 		}
 
@@ -150,13 +150,13 @@ func ColorizeINI(b []byte) []byte {
 
 				// Colorize key
 				key, scope, scopeFound := strings.Cut(key, "@")
-				color.Set(color.FgCyan).Fprint(out, key)
+				color.New(color.FgCyan).Fprint(out, key)
 				if scopeFound {
-					color.Set(color.FgHiMagenta).Fprint(out, "@"+scope)
+					color.New(color.FgHiMagenta).Fprint(out, "@"+scope)
 				}
 
 				// Colorize delimiter
-				color.Set(color.FgHiBlack).Fprint(out, delim)
+				color.New(color.FgHiBlack).Fprint(out, delim)
 
 				// Highlight references in the value
 				referenceMatches := referenceRE.FindAllStringIndex(value, -1)
@@ -168,7 +168,7 @@ func ColorizeINI(b []byte) []byte {
 
 						// Write reference part in green + bold
 						referenceText := value[match[0]:match[1]]
-						color.Set(color.FgGreen, color.Bold).Fprint(out, referenceText)
+						color.New(color.FgGreen, color.Bold).Fprint(out, referenceText)
 						lastPos = match[1]
 					}
 					// Write remaining part after last reference
@@ -179,7 +179,7 @@ func ColorizeINI(b []byte) []byte {
 				}
 
 				if inlineComment != "" {
-					color.Set(color.FgHiBlack, color.Italic).Fprint(out, inlineComment)
+					color.New(color.FgHiBlack, color.Italic).Fprint(out, inlineComment)
 				}
 
 				out.WriteString("\n")
@@ -204,7 +204,7 @@ func ColorizeINI(b []byte) []byte {
 
 				// Write reference part in green + bold
 				referenceText := line[match[0]:match[1]]
-				color.Set(color.FgGreen, color.Bold).Fprint(out, referenceText)
+				color.New(color.FgGreen, color.Bold).Fprint(out, referenceText)
 				lastPos = match[1]
 			}
 			// Write remaining part after last reference

core/commoncmd/config_test.go

@@ -0,0 +1,56 @@
+package commoncmd
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/fatih/color"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// colorized renders src with the colors on, and returns what ColorizeINI
+// returned and what it wrote past its return, which is meant to be nothing.
+func colorized(t *testing.T, src string) (rendered string, leaked string) {
+	t.Helper()
+	t.Setenv("NO_COLOR", "")
+	savedOutput, savedNoColor := color.Output, color.NoColor
+	buf := bytes.NewBuffer(nil)
+	color.Output, color.NoColor = buf, false
+	t.Cleanup(func() { color.Output, color.NoColor = savedOutput, savedNoColor })
+	b := ColorizeINI([]byte(src))
+	return string(b), buf.String()
+}
+
+// TestColorizeINIWritesNothingOfItsOwn pins that the rendering is the
+// returned bytes and nothing else.
+//
+// It used to set each color with color.Set, which writes the escape sequence
+// to the process output as a side effect and returns the color to render
+// with. Every colored element therefore left a bare, never reset sequence
+// ahead of the whole rendering.
+func TestColorizeINIWritesNothingOfItsOwn(t *testing.T) {
+	_, leaked := colorized(t, "[fs#1]\n# why this flag\ntype = flag  # inline\nnodes = {clusternodes}\n")
+	assert.Empty(t, leaked, "the rendering must be the returned bytes, not a side effect")
+}
+
+// TestColorizeINISectionHeaderCarriesNoCommentAttribute pins the symptom the
+// leak showed: the italic of a comment was still on when the first line was
+// drawn, so the section header of a commented section was rendered italic.
+func TestColorizeINISectionHeaderCarriesNoCommentAttribute(t *testing.T) {
+	const italic = "\x1b[3m"
+	for _, src := range []string{
+		"[fs#1]\n# why this flag\ntype = flag\n",
+		"# why this section\n[fs#1]\ntype = flag\n",
+		"[DEFAULT]\nnodes = *\n\n# why this section\n[fs#1]\ntype = flag\n",
+	} {
+		rendered, leaked := colorized(t, src)
+		require.Empty(t, leaked)
+
+		// The first thing drawn opens with its own attributes, so nothing a
+		// later line sets can reach it.
+		first, _, _ := bytes.Cut([]byte(rendered), []byte("\n"))
+		assert.Truef(t, bytes.HasPrefix(first, []byte("\x1b[")), "the first line must open its own sequence, got %q", first)
+		assert.NotContainsf(t, string(first), italic, "the first line of %q must not be italic", src)
+	}
+}

core/commoncmd/daemon_hb_status.go → core/commoncmd/daemon_hb_list.go

@@ -2,10 +2,10 @@ package commoncmd
 
 import (
 	"encoding/json"
-	"fmt"
 	"sort"
 	"strings"
 
+	"github.com/fatih/color"
 	"github.com/spf13/cobra"
 
 	"github.com/opensvc/om3/v3/core/client"
@@ -17,7 +17,7 @@ import (
 )
 
 type (
-	CmdDaemonHeartbeatStatus struct {
+	CmdDaemonHeartbeatList struct {
 		Color        string
 		Output       string
 		NodeSelector string
@@ -26,13 +26,57 @@ type (
 	}
 )
 
-func NewCmdDaemonHeartbeatStatus(defaultNodeSelectorFilter string) *cobra.Command {
-	options := CmdDaemonHeartbeatStatus{
+// heartbeatStreamRow is a row of the heartbeat listing.
+//
+// The state and the beating flag travel from the node as values, and the
+// icons standing for them are drawn here, where the output is. The daemon
+// type carrying the values holds no escape sequence: it is published, and
+// the tui reads the same values to paint cells of its own.
+type heartbeatStreamRow struct {
+	daemonsubsystem.HeartbeatStreamPeerStatusTableEntry
+}
+
+type heartbeatStreamRows []heartbeatStreamRow
+
+// Unstructured returns the row the tab renderer reads: what the entry
+// carries, plus the two icon columns the default output names.
+func (t heartbeatStreamRow) Unstructured() map[string]any {
+	m := t.HeartbeatStreamPeerStatusTableEntry.Unstructured()
+	m["state_icon"] = heartbeatStateIcon(t.State)
+	m["beating_icon"] = heartbeatBeatingIcon(t.IsSingleNode || t.IsBeating)
+	return m
+}
+
+// heartbeatStateIcon draws the run state of a stream.
+func heartbeatStateIcon(state string) string {
+	switch state {
+	case "running":
+		return color.New(color.FgGreen).Sprint("O")
+	case "stopped", "failed":
+		return color.New(color.FgRed).Sprint("X")
+	case "warning":
+		return color.New(color.FgYellow).Sprint("!")
+	default:
+		return color.New(color.FgHiBlack).Sprint("?")
+	}
+}
+
+// heartbeatBeatingIcon draws whether a stream is beating.
+func heartbeatBeatingIcon(beating bool) string {
+	if beating {
+		return color.New(color.FgGreen).Sprint("O")
+	}
+	return color.New(color.FgRed).Sprint("X")
+}
+
+func NewCmdDaemonHeartbeatList(defaultNodeSelectorFilter string) *cobra.Command {
+	options := CmdDaemonHeartbeatList{
 		NodeSelector: defaultNodeSelectorFilter,
 	}
 	cmd := &cobra.Command{
-		Use:   "status",
-		Short: fmt.Sprintf("daemon heartbeat status"),
+		Use:     "list",
+		Short:   "list the heartbeat streams and their state",
+		Aliases: []string{"ls"},
 		RunE: func(cmd *cobra.Command, args []string) error {
 			return options.Run()
 		},
@@ -46,7 +90,7 @@ func NewCmdDaemonHeartbeatStatus(defaultNodeSelectorFilter string) *cobra.Comman
 	return cmd
 }
 
-func (t *CmdDaemonHeartbeatStatus) Run() error {
+func (t *CmdDaemonHeartbeatList) Run() error {
 	cli, err := client.New()
 	if err != nil {
 		return err
@@ -79,7 +123,7 @@ func (t *CmdDaemonHeartbeatStatus) Run() error {
 
 	isSingleNode := len(data.Cluster.Node) == 1
 
-	table := make(daemonsubsystem.HeartbeatStreamPeerStatusTable, 0)
+	table := make(heartbeatStreamRows, 0)
 	for nodename, nodeData := range data.Cluster.Node {
 		if nodeMap != nil {
 			if _, ok := nodeMap[nodename]; !ok {
@@ -103,7 +147,7 @@ func (t *CmdDaemonHeartbeatStatus) Run() error {
 					}
 				}
 			}
-			table = append(table, e)
+			table = append(table, heartbeatStreamRow{e})
 		}
 	}
 
@@ -128,3 +172,14 @@ func (t *CmdDaemonHeartbeatStatus) Run() error {
 
 	return nil
 }
+
+// NewCmdDaemonHeartbeatStatus is the name the list command answered to
+// before the listings were named after what they render: a row per stream.
+// It is kept for the readers whose fingers and scripts type it.
+func NewCmdDaemonHeartbeatStatus(defaultNodeSelectorFilter string) *cobra.Command {
+	cmd := NewCmdDaemonHeartbeatList(defaultNodeSelectorFilter)
+	cmd.Use = "status"
+	cmd.Aliases = nil
+	cmd.Hidden = true
+	return cmd
+}

core/commoncmd/daemon_hb_list_test.go

@@ -0,0 +1,86 @@
+package commoncmd
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/fatih/color"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/opensvc/om3/v3/daemon/daemonsubsystem"
+)
+
+func heartbeatRow(t *testing.T, state string, isBeating, isSingleNode bool) map[string]any {
+	t.Helper()
+	t.Setenv("NO_COLOR", "")
+	savedNoColor := color.NoColor
+	color.NoColor = false
+	t.Cleanup(func() { color.NoColor = savedNoColor })
+
+	entry := daemonsubsystem.HeartbeatStreamPeerStatusTableEntry{
+		HeartbeatStreamPeerStatus: daemonsubsystem.HeartbeatStreamPeerStatus{
+			IsBeating: isBeating,
+		},
+		IsSingleNode: isSingleNode,
+	}
+	entry.Status.State = state
+	return heartbeatStreamRow{entry}.Unstructured()
+}
+
+// TestHeartbeatStreamRowDrawsTheIcons pins that the listing composes the
+// two icon columns its default output names, from the values the daemon
+// type carries.
+func TestHeartbeatStreamRowDrawsTheIcons(t *testing.T) {
+	for _, tc := range []struct {
+		state        string
+		isBeating    bool
+		isSingleNode bool
+		stateIcon    string
+		beatingIcon  string
+	}{
+		{"running", true, false, "O", "O"},
+		{"stopped", false, false, "X", "X"},
+		{"failed", false, false, "X", "X"},
+		{"warning", true, false, "!", "O"},
+		{"", false, false, "?", "X"},
+
+		// A single node has no peer to beat with, and is not stale for it.
+		{"running", false, true, "O", "O"},
+	} {
+		m := heartbeatRow(t, tc.state, tc.isBeating, tc.isSingleNode)
+		stateIcon, ok := m["state_icon"].(string)
+		require.Truef(t, ok, "state %q has no state_icon", tc.state)
+		beatingIcon, ok := m["beating_icon"].(string)
+		require.Truef(t, ok, "state %q has no beating_icon", tc.state)
+
+		assert.Equalf(t, tc.stateIcon, stripANSI(stateIcon), "state icon of %q", tc.state)
+		assert.Equalf(t, tc.beatingIcon, stripANSI(beatingIcon), "beating icon of %q", tc.state)
+		assert.Containsf(t, stateIcon, "\x1b[", "the state icon of %q must be colored", tc.state)
+		assert.Containsf(t, beatingIcon, "\x1b[", "the beating icon of %q must be colored", tc.state)
+	}
+}
+
+// TestHeartbeatStreamRowKeepsWhatTheEntryCarries pins that the row adds to
+// the entry rather than replacing it: the other columns of the default
+// output are still resolved.
+func TestHeartbeatStreamRowKeepsWhatTheEntryCarries(t *testing.T) {
+	m := heartbeatRow(t, "running", true, false)
+	for _, key := range []string{"id", "node", "peer", "type", "desc", "changed_at", "state", "is_beating"} {
+		assert.Containsf(t, m, key, "%s is named by the default output", key)
+	}
+}
+
+func stripANSI(s string) string {
+	for {
+		i := strings.Index(s, "\x1b[")
+		if i < 0 {
+			return s
+		}
+		j := strings.IndexByte(s[i:], 'm')
+		if j < 0 {
+			return s
+		}
+		s = s[:i] + s[i+j+1:]
+	}
+}