Dev

.github/workflows/build.yml

@@ -48,9 +48,18 @@ jobs:
           path: docs/flags.md
 
       - name: Test
-        # Discover's first sign-in opens a real window, which chromedp will not
-        # do without a display. Xvfb comes with the runner image.
-        run: xvfb-run -a go test -race -cover -timeout 5m ./...
+        # Xvfb where the image has it, since Discover's first sign-in opens a
+        # real window and chromedp will not without a display. Not required:
+        # the browser test skips itself when there is no DISPLAY.
+        #
+        # -p 1 because two packages reach config.DebugRoot(), one fixed path
+        # shared by every test binary.
+        run: |
+          if command -v xvfb-run >/dev/null 2>&1; then
+            xvfb-run -a go test -race -cover -p 1 -timeout 5m ./...
+          else
+            go test -race -cover -p 1 -timeout 5m ./...
+          fi
 
       - name: Build Linux
         run: |

.github/workflows/release.yml

@@ -24,9 +24,17 @@ jobs:
           go-version-file: go.mod
 
       - name: Test
-        # Discover's first sign-in opens a real window, which chromedp will not
-        # do without a display. Xvfb comes with the runner image.
-        run: xvfb-run -a go test -timeout 5m ./...
+        # -short, which is what broke the v0.5.0 release: the browser
+        # integration test drove chromedp on the runner, Chrome never came up,
+        # and 45s later the tag had no binaries. A release gate must not depend
+        # on whether a browser starts on a VM -- the full suite, that test
+        # included, runs in build.yml on every push to master, so the commit
+        # being tagged has already been through it.
+        #
+        # -p 1 because two packages reach config.DebugRoot(), one fixed path
+        # shared by every test binary; run in parallel they clear each other's
+        # store and the release fails at random.
+        run: go test -short -p 1 -timeout 5m ./...
 
       - name: Build Linux
         run: |

.gitignore

@@ -4,3 +4,9 @@
 *.txt
 *.json
 *.md
+# The release job builds into dist/, and a local reproduction of it leaves
+# 38MB of binaries here that git add -A would happily commit.
+dist/
+# go test -c and -cpuprofile leave these behind.
+*.test
+*.out

cmd/iontk/batch.go

@@ -16,22 +16,15 @@ import (
 	"tyrtop.com/iontk/internal/tui"
 )
 
-// selection is what a batch run acts on. A --search resolves its own targets,
-// each carrying the client and device login of the tenant it came from,
-// because one search spans the whole working set; everything else hands over
-// element IDs and lets one client serve them all.
 type selection struct {
 	elements []string
 	targets  []sase.Target
 	clients  []*api.Client
-	// stdin says the IDs arrived down a pipe, which changes what the progress
-	// display may do with the terminal.
-	stdin bool
+	stdin    bool
 }
 
 func (s selection) crossTenant() bool { return len(s.targets) > 0 }
 
-// flagSelection is the ordinary --element / -f target list.
 func flagSelection() (selection, error) {
 	els, err := gatherElements(flagElement, flagElementsPath)
 	if err != nil {
@@ -48,8 +41,6 @@ func runBatch(ctx context.Context, sel selection) error {
 	var f app.Flags
 	var err error
 	if sel.crossTenant() {
-		// Nothing left to resolve: every target already holds the credential
-		// it will sign in with.
 		f = app.BaseFlags()
 	} else if f, err = resolveFlags(os.Stderr); err != nil {
 		return err
@@ -91,20 +82,13 @@ func runBatch(ctx context.Context, sel selection) error {
 		return err
 	}
 
-	// So a run that outlives its credential can stop, rather than spend an
-	// element timeout each on everything left.
 	ctx, stopRun := context.WithCancel(ctx)
 	defer stopRun()
 
-	// The api package no longer prints refresh failures itself, so a batch run
-	// has to say them out loud or a token dying mid-run looks like the devices
-	// going quiet.
 	for _, c := range clients {
 		go watchCredential(c.TokenErrors(), c.RefreshStopped, stopRun, os.Stderr)
 	}
 
-	// The file is only created once the run is ready to produce something, so a
-	// failure before this point leaves no empty report behind.
 	out, where, err := output.Open(flagOut, dest, format, time.Now())
 	if err != nil {
 		return err
@@ -114,18 +98,13 @@ func runBatch(ctx context.Context, sel selection) error {
 		fmt.Fprintln(os.Stderr, "writing", format, "results to", where)
 	}
 	sinkOpts := []output.Option{output.WithOrientation(orient)}
-	// A table only has to fit when it is being read on the way past. A file
-	// holds the only copy of what it is given, so nothing is trimmed into one.
 	if where == "stdout" {
 		if cols, _, serr := term.GetSize(int(os.Stdout.Fd())); serr == nil && cols > 0 {
 			sinkOpts = append(sinkOpts, output.WithWidth(cols))
 		}
 	}
 	sink := output.NewSink(out, format, sinkOpts...)
 
-	// Fixture mode swaps the client only here: the lookup and the session come
-	// from the playback device, and everything either side of them is the real
-	// run, including ioncli's autologin and pager handling.
 	if app.Fixture != nil {
 		playback := app.Fixture.SessionClient(f.PagerOff, f.PagerPrompts)
 		for i := range targets {
@@ -159,8 +138,6 @@ func runBatch(ctx context.Context, sel selection) error {
 	return nil
 }
 
-// batchOutput resolves the format, row orientation and destination folder,
-// with flags winning over the saved settings.
 func batchOutput() (format, orient, dir string, err error) {
 	format, orient, dir = output.FormatJSON, output.OrientION, ""
 	if st := app.OpenStore(); st != nil {
@@ -200,8 +177,6 @@ func drainPlain(events <-chan sase.Event, total int, sink output.Sink) (int, int
 	return total, failed, nil
 }
 
-// dead separates a spent credential from a run that merely finished; both
-// close the channel.
 func watchCredential(ch <-chan error, dead func() bool, stop func(), w io.Writer) {
 	for err := range ch {
 		fmt.Fprintln(w, err)

cmd/iontk/batch_test.go

@@ -72,8 +72,6 @@ func TestWatchCredentialStopsTheRunWhenTheCredentialIsSpent(t *testing.T) {
 	}
 }
 
-// The CLI and the TUI read the same settings, so a flag has to be the only
-// thing that moves one of them off what the other would have done.
 func TestBatchOutputPrefersFlagsOverSettings(t *testing.T) {
 	app.Debug = true
 	t.Cleanup(func() {
@@ -109,8 +107,6 @@ func TestBatchOutputPrefersFlagsOverSettings(t *testing.T) {
 		t.Errorf("batchOutput() = %q/%q, want the flags to win", format, orient)
 	}
 
-	// A typo is an error before the run starts, not a silent fall back to the
-	// default -- a fleet written out the wrong way round is a re-run.
 	flagOrient = "sideways"
 	if _, _, _, err := batchOutput(); err == nil {
 		t.Error("batchOutput() error = nil, want an unknown orientation to fail")

cmd/iontk/creds.go

@@ -5,17 +5,22 @@ import (
 	"io"
 
 	"tyrtop.com/iontk/internal/app"
+	"tyrtop.com/iontk/internal/ui"
 )
 
-// Batch and shell both go through here, so they can't end up using different
-// tenants.
 func resolveFlags(w io.Writer) (app.Flags, error) {
-	creds, err := app.ResolveCredentials(app.OpenStore(), flagTenant)
+	st := app.OpenStore()
+	app.ApplyDisplaySettings(st, nil)
+
+	creds, err := app.ResolveCredentials(st, flagTenant)
 	if err != nil {
 		return app.Flags{}, err
 	}
 	for _, line := range creds.Lines() {
 		fmt.Fprintln(w, line)
 	}
+	if err := ui.ThemesErr(); err != "" {
+		fmt.Fprintln(w, "themes: "+err)
+	}
 	return creds.Flags, nil
 }

cmd/iontk/fixtures.go

@@ -9,8 +9,6 @@ import (
 	"tyrtop.com/iontk/internal/fixture"
 )
 
-// loadFixtures runs before every command, so --fixtures works on the root and
-// on the subcommands alike.
 func loadFixtures(w io.Writer) error {
 	if flagFixtures == "" {
 		return nil
@@ -21,13 +19,9 @@ func loadFixtures(w io.Writer) error {
 	}
 	app.Fixture = set
 
-	// Said out loud, because a fixture run that looked like a real one would be
-	// the worst possible confusion in a tool that writes to appliances.
 	fmt.Fprintf(w, "fixtures: %s — no network, no saved state\n", flagFixtures)
 	for _, t := range set.Tenants {
 		fmt.Fprintf(w, "fixtures: %s (tsg %s), %d elements\n", t.Name, t.TSGID, len(t.Elements))
-		// IDs are generated from the names unless the file gives them, and
-		// --element needs one, so -v is how you find out what they are.
 		if !app.Defaults.Verbose {
 			continue
 		}
@@ -36,8 +30,6 @@ func loadFixtures(w io.Writer) error {
 		}
 	}
 
-	// The environment would otherwise win over the fixture's own logins and
-	// token, and every session would answer "Login incorrect".
 	for _, k := range []string{"ION_USER", "ION_PASS", "SCM_TOKEN", "SCM_CLIENT_ID", "SCM_CLIENT_SECRET", "SCM_TSG_ID"} {
 		if os.Getenv(k) == "" {
 			continue

cmd/iontk/flagdoc.go

@@ -12,9 +12,6 @@ import (
 	"github.com/spf13/pflag"
 )
 
-// Flags are documented in docs/flags.md, which this generates. A hand-written
-// reference went stale within a release; three flags were added and one
-// deprecated in a single sitting.
 var genFlagDocsCmd = &cobra.Command{
 	Use:    "gen-flag-docs [file]",
 	Short:  "Write the flag reference to a file, or to stdout",
@@ -25,11 +22,7 @@ var genFlagDocsCmd = &cobra.Command{
 		if err := writeFlagDocs(&b, rootCmd); err != nil {
 			return err
 		}
-		// Writing the file here rather than redirecting stdout keeps the
-		// go:generate line free of a shell, so it runs on Windows too.
 		if len(args) == 1 {
-			// docs/ is gitignored, so a fresh checkout has no directory to
-			// write into.
 			if dir := filepath.Dir(args[0]); dir != "." {
 				if err := os.MkdirAll(dir, 0o755); err != nil {
 					return fmt.Errorf("create %s: %w", dir, err)
@@ -45,9 +38,6 @@ var genFlagDocsCmd = &cobra.Command{
 	},
 }
 
-// flagGroups is the running order. A flag missing from here fails generation
-// rather than landing in a catch-all: being forced to classify a new flag is
-// the only thing that keeps the grouping meaningful.
 var flagGroups = []struct {
 	name  string
 	intro string
@@ -78,6 +68,9 @@ var flagGroups = []struct {
 	{"Rich data", "The TUI's third element mode reads the portal instead of opening a session. Its panes are defined in YAML: `--views` points at a file instead of `views.yaml` in the config directory, which is also how a `--fixtures` run gets views of its own.", []string{
 		"views",
 	}},
+	{"Appearance", "Colour schemes are YAML too: `--themes` points at a file whose schemes are added to the built-in ones, or replace one of the same name, instead of `themes.yaml` in the config directory. The same escape hatch as `--views`, for the same reason.", []string{
+		"themes",
+	}},
 }
 
 func writeFlagDocs(w io.Writer, root *cobra.Command) error {
@@ -154,8 +147,6 @@ func writeFlagTable(b *strings.Builder, flags []*pflag.Flag) {
 	}
 }
 
-// groupFlags sorts every registered flag into its group, and reports any that
-// no group claims.
 func groupFlags(root *cobra.Command) (map[string][]*pflag.Flag, error) {
 	want := map[string]string{}
 	for _, g := range flagGroups {
@@ -184,8 +175,6 @@ func groupFlags(root *cobra.Command) (map[string][]*pflag.Flag, error) {
 			strings.Join(ungrouped, ", "))
 	}
 
-	// Within a group, the order flagGroups lists them in, not alphabetical:
-	// related timeouts read better together than apart.
 	for _, g := range flagGroups {
 		rank := map[string]int{}
 		for i, name := range g.flags {

cmd/iontk/flagdoc_test.go

@@ -11,8 +11,6 @@ import (
 )
 
 func TestEveryFlagIsGrouped(t *testing.T) {
-	// The guard that makes the generated doc worth having: a new flag cannot
-	// quietly land in a catch-all section nobody reads.
 	if _, err := groupFlags(rootCmd); err != nil {
 		t.Errorf("groupFlags() error = %v", err)
 	}
@@ -26,7 +24,6 @@ func TestAnUngroupedFlagFailsGeneration(t *testing.T) {
 	if err == nil {
 		t.Fatal("groupFlags() error = nil, want an ungrouped flag to fail")
 	}
-	// The message has to name the flag and where to put it, or it is a riddle.
 	for _, want := range []string{"brand-new", "flagdoc.go"} {
 		if !strings.Contains(err.Error(), want) {
 			t.Errorf("error = %q, want it to mention %q", err, want)
@@ -40,7 +37,6 @@ func TestGroupedFlagsKeepTheirListedOrder(t *testing.T) {
 		t.Fatalf("groupFlags() error = %v", err)
 	}
 
-	// Related timeouts read better together than alphabetically.
 	var want []string
 	for _, g := range flagGroups {
 		if g.name == "Connection" {
@@ -72,8 +68,6 @@ func TestDeprecatedFlagsAreSeparated(t *testing.T) {
 			t.Errorf("--%s is in Deprecated with no deprecation message", f.Name)
 		}
 	}
-	// A deprecated flag must not also appear in a live group, or it reads as
-	// current.
 	for _, g := range flagGroups {
 		for _, f := range grouped[g.name] {
 			if f.Deprecated != "" {
@@ -95,7 +89,6 @@ func TestTheDocMentionsEveryFlagAndEnvVar(t *testing.T) {
 			t.Errorf("--%s is missing from the generated doc", f.Name)
 		}
 	})
-	// These are read in BaseFlags() and documented nowhere else.
 	for _, env := range []string{
 		"SCM_CLIENT_ID", "SCM_CLIENT_SECRET", "SCM_TSG_ID", "SCM_TOKEN",
 		"ION_USER", "ION_PASS",
@@ -107,7 +100,6 @@ func TestTheDocMentionsEveryFlagAndEnvVar(t *testing.T) {
 }
 
 func TestGeneratingIntoAMissingDirectory(t *testing.T) {
-	// docs/ is gitignored, so CI generates into a directory that is not there.
 	out := filepath.Join(t.TempDir(), "docs", "flags.md")
 
 	if err := genFlagDocsCmd.RunE(genFlagDocsCmd, []string{out}); err != nil {
@@ -124,7 +116,6 @@ func TestGeneratingIntoAMissingDirectory(t *testing.T) {
 }
 
 func TestTheGeneratorIsHidden(t *testing.T) {
-	// It would otherwise show up in --help beside run and shell.
 	if !genFlagDocsCmd.Hidden {
 		t.Error("gen-flag-docs is not hidden")
 	}

cmd/iontk/inspect.go

@@ -5,10 +5,10 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
-	"time"
 
 	"github.com/spf13/cobra"
 	"tyrtop.com/iontk/internal/app"
+	"tyrtop.com/iontk/internal/clock"
 	"tyrtop.com/iontk/internal/sase/api"
 )
 
@@ -63,18 +63,13 @@ func runInspect(ctx context.Context, eid string) error {
 	}
 
 	snap := client.ElementSnapshot(ctx, el)
-	// The record is the one body the snapshot does not fetch for itself, and
-	// the one the widened element fields are read off.
 	snap.Raw[api.ElementURL(eid)] = body
 
 	enc := json.NewEncoder(os.Stdout)
 	enc.SetIndent("", "  ")
 	if flagInspectRaw {
 		return enc.Encode(snap.Raw)
 	}
-	// Every part that failed is reported on stderr as well as in the JSON: a
-	// snapshot of an element whose status endpoint refused is still worth
-	// printing, but not worth mistaking for a complete one.
 	if err := snap.Err(); err != nil {
 		fmt.Fprintln(os.Stderr, err)
 	}
@@ -102,9 +97,6 @@ type statusReport struct {
 	Channels            []channelReport `json:"channels"`
 }
 
-// The utc integers are printed as both the number and a time, because the
-// unit is the part of these shapes least proven against hardware: a wrong
-// scale shows up as a date in the wrong century.
 type channelReport struct {
 	Name           string `json:"name"`
 	Connected      bool   `json:"connected"`
@@ -164,9 +156,5 @@ func inspectOut(snap api.Snapshot) inspectReport {
 }
 
 func utcString(v int64) string {
-	t := api.UTCTime(v)
-	if t.IsZero() {
-		return ""
-	}
-	return t.Format(time.RFC3339)
+	return clock.RFC3339(api.UTCTime(v))
 }