fix: rip out '--no-color' due to race condition

We still support `NO_COLOR` env variable through termenv's `EnvColorProfile`. The reason for the race condition is that `DefaultStyles` is a global that we shouldn't mutate after `init` is called, but we have to mutate it after `init` has ran to have serpent collect the cli flags and env vars for us.
coder · DanielleMaywood · Sep 17, 2024 · Sep 13, 2024 · Sep 13, 2024 · Sep 13, 2024
commit ff3c39294d72475b0f0ed025ecce43c745faef1f
diff --git a/cli/cliui/cliui.go b/cli/cliui/cliui.go
@@ -3,6 +3,7 @@ package cliui
 import (
 	"flag"
 	"os"
+	"sync"
 	"time"
 
 	"github.com/muesli/termenv"
@@ -11,8 +12,6 @@ import (
 	"github.com/coder/pretty"
 )
 
-const NoColorFlag = "no-color"
-
 var Canceled = xerrors.New("canceled")
 
 // DefaultStyles compose visual elements of the UI.
@@ -32,10 +31,32 @@ type Styles struct {
 	Wrap pretty.Style
 }
 
-var color termenv.Profile
+var (
+	color     termenv.Profile
+	colorOnce sync.Once
+)
+
+var (
+	red           = Color("1")
+	green         = Color("2")
+	yellow        = Color("3")
+	magenta       = Color("5")
+	white         = Color("7")
+	brightBlue    = Color("12")
+	brightMagenta = Color("13")
+)
 
 // Color returns a color for the given string.
 func Color(s string) termenv.Color {
+	colorOnce.Do(func() {
+		color = termenv.NewOutput(os.Stdout).EnvColorProfile()
+
+		if flag.Lookup("test.v") != nil {
+			// Use a consistent colorless profile in tests so that results
+			// are deterministic.
+			color = termenv.Ascii
+		}
+	})
 	return color.Color(s)
 }
 
@@ -91,87 +112,58 @@ func Field(s string) string {
 	return pretty.Sprint(DefaultStyles.Field, s)
 }
 
-func ifTerm(f pretty.Formatter) pretty.Formatter {
+func ifTerm(fmt pretty.Formatter) pretty.Formatter {
 	if !isTerm() {
 		return pretty.Nop
 	}
-	return f
-}
-
-type InitOptions struct {
-	NoColor bool
+	return fmt
 }
 
 func init() {
-	color = termenv.NewOutput(os.Stdout).ColorProfile()
-
-	if flag.Lookup("test.v") != nil {
-		// Use a consistent colorless profile in tests so that results
-		// are deterministic.
-		color = termenv.Ascii
-	}
-
-	DefaultStyles.Code = pretty.Style{
-		ifTerm(pretty.XPad(1, 1)),
-	}
-	DefaultStyles.Field = pretty.Style{
-		pretty.XPad(1, 1),
-	}
-	DefaultStyles.Wrap = pretty.Style{
-		pretty.LineWrap(80),
-	}
-}
-
-func Init(opts InitOptions) {
-	if opts.NoColor {
-		color = termenv.Ascii
-	}
-
-	red := color.Color("1")
-	green := color.Color("2")
-	yellow := color.Color("3")
-	magenta := color.Color("5")
-	white := color.Color("7")
-	brightBlue := color.Color("12")
-	brightMagenta := color.Color("13")
-
 	// We do not adapt the color based on whether the terminal is light or dark.
 	// Doing so would require a round-trip between the program and the terminal
 	// due to the OSC query and response.
-	DefaultStyles.Code = append(DefaultStyles.Code,
-		pretty.FgColor(color.Color("#ED567A")),
-		pretty.BgColor(color.Color("#2C2C2C")),
-	)
-	DefaultStyles.DateTimeStamp = pretty.Style{
-		pretty.FgColor(brightBlue),
-	}
-	DefaultStyles.Error = pretty.Style{
-		pretty.FgColor(red),
-	}
-	DefaultStyles.Field = append(DefaultStyles.Field,
-		pretty.FgColor(color.Color("#FFFFFF")),
-		pretty.BgColor(color.Color("#2B2A2A")),
-	)
-	DefaultStyles.Fuchsia = pretty.Style{
-		pretty.FgColor(brightMagenta),
-	}
-	DefaultStyles.FocusedPrompt = pretty.Style{
-		pretty.FgColor(white),
-		pretty.Wrap("> ", ""),
-		pretty.FgColor(brightBlue),
-	}
-	DefaultStyles.Keyword = pretty.Style{
-		pretty.FgColor(green),
-	}
-	DefaultStyles.Placeholder = pretty.Style{
-		pretty.FgColor(magenta),
-	}
-	DefaultStyles.Prompt = pretty.Style{
-		pretty.FgColor(white),
-		pretty.Wrap("  ", ""),
-	}
-	DefaultStyles.Warn = pretty.Style{
-		pretty.FgColor(yellow),
+	DefaultStyles = Styles{
+		Code: pretty.Style{
+			ifTerm(pretty.XPad(1, 1)),
+			pretty.FgColor(Color("#ED567A")),
+			pretty.BgColor(Color("#2C2C2C")),
+		},
+		DateTimeStamp: pretty.Style{
+			pretty.FgColor(brightBlue),
+		},
+		Error: pretty.Style{
+			pretty.FgColor(red),
+		},
+		Field: pretty.Style{
+			pretty.XPad(1, 1),
+			pretty.FgColor(Color("#FFFFFF")),
+			pretty.BgColor(Color("#2B2A2A")),
+		},
+		Fuchsia: pretty.Style{
+			pretty.FgColor(brightMagenta),
+		},
+		FocusedPrompt: pretty.Style{
+			pretty.FgColor(white),
+			pretty.Wrap("> ", ""),
+			pretty.FgColor(brightBlue),
+		},
+		Keyword: pretty.Style{
+			pretty.FgColor(green),
+		},
+		Placeholder: pretty.Style{
+			pretty.FgColor(magenta),
+		},
+		Prompt: pretty.Style{
+			pretty.FgColor(white),
+			pretty.Wrap("  ", ""),
+		},
+		Warn: pretty.Style{
+			pretty.FgColor(yellow),
+		},
+		Wrap: pretty.Style{
+			pretty.LineWrap(80),
+		},
 	}
 }
 

diff --git a/cli/root.go b/cli/root.go
@@ -331,22 +331,6 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err
 		r.clientURL = new(url.URL)
 	}
 
-	// Add a wrapper to every command to ensure we've loaded our CLI theme prior
-	// to running any handler.
-	var noColor bool
-	cmd.Walk(func(c *serpent.Command) {
-		middleware := func(next serpent.HandlerFunc) serpent.HandlerFunc {
-			cliui.Init(cliui.InitOptions{NoColor: noColor})
-			return next
-		}
-
-		if c.Middleware != nil {
-			c.Middleware = serpent.Chain(c.Middleware, middleware)
-		} else {
-			c.Middleware = middleware
-		}
-	})
-
 	globalGroup := &serpent.Group{
 		Name:        "Global",
 		Description: `Global options are applied to all commands. They can be set using environment variables or flags.`,
@@ -477,14 +461,6 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err
 			Value:       serpent.StringOf(&r.globalConfig),
 			Group:       globalGroup,
 		},
-		{
-			Flag:        cliui.NoColorFlag,
-			Env:         "CODER_NO_COLOR",
-			Default:     "false",
-			Description: "Disable use of color in CLI output.",
-			Group:       globalGroup,
-			Value:       serpent.BoolOf(&noColor),
-		},
 		{
 			Flag: "version",
 			// This was requested by a customer to assist with their migration.

diff --git a/cli/testdata/coder_--help.golden b/cli/testdata/coder_--help.golden
@@ -89,9 +89,6 @@ variables or flags.
           requests. The command must output each header as `key=value` on its
           own line.
 
-      --no-color bool, $CODER_NO_COLOR (default: false)
-          Disable use of color in CLI output.
-
       --no-feature-warning bool, $CODER_NO_FEATURE_WARNING
           Suppress warnings about unlicensed features.
 

diff --git a/cmd/cliui/main.go b/cmd/cliui/main.go
@@ -388,28 +388,6 @@ func main() {
 		},
 	})
 
-	var noColor bool
-	root.Options = []serpent.Option{
-		{
-			Default: "false",
-			Flag:    cliui.NoColorFlag,
-			Value:   serpent.BoolOf(&noColor),
-		},
-	}
-
-	root.Walk(func(cmd *serpent.Command) {
-		middleware := func(next serpent.HandlerFunc) serpent.HandlerFunc {
-			cliui.Init(cliui.InitOptions{NoColor: noColor})
-			return next
-		}
-
-		if cmd.Middleware != nil {
-			cmd.Middleware = serpent.Chain(cmd.Middleware, middleware)
-		} else {
-			cmd.Middleware = middleware
-		}
-	})
-
 	err := root.Invoke(os.Args[1:]...).WithOS().Run()
 	if err != nil {
 		_, _ = fmt.Println(err.Error())

diff --git a/docs/reference/cli/README.md b/docs/reference/cli/README.md
diff --git a/enterprise/cli/testdata/coder_--help.golden b/enterprise/cli/testdata/coder_--help.golden
@@ -49,9 +49,6 @@ variables or flags.
           requests. The command must output each header as `key=value` on its
           own line.
 
-      --no-color bool, $CODER_NO_COLOR (default: false)
-          Disable use of color in CLI output.
-
       --no-feature-warning bool, $CODER_NO_FEATURE_WARNING
           Suppress warnings about unlicensed features.