fix: use ANSI colors codes instead of RGB #14665

DanielleMaywood · 2024-09-13T08:34:00Z

Instead of using RGB color codes, we instead use ANSI color codes. This means that we respect a user's terminal theme. Not all of the new colors match the old ones but this is a limitation with using 4-bit ANSI colors.

This change also allows users to pass --no-color to the CLI to completely disable theming. It is worth noting that whilst we do define the serpent.Option, we manually check os.Args for the flag as there doesn't appear to be a way to have a global middleware with serpent (whereas spf13/cobra does have this).

This change also removes > from Prompt, and replaces it with as on --no-color there is no difference.

Before:

After:

Before:

After:

Before:

After:

Before:

After:

~~With --no-color~~ Now requires NO_COLOR environment variable

johnstcn

Nice work! Just some nits on my end.
Smoke-tested example prompts with and without colour.

johnstcn · 2024-09-13T14:35:31Z

cli/root.go

@@ -461,6 +461,12 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err
 			Value:       serpent.StringOf(&r.globalConfig),
 			Group:       globalGroup,
 		},
+		{
+			Flag:        cliui.NoColorFlag,


Wonder if it would make sense to also allow setting it as an env (CODER_NO_COLOR)? Not blocking, but it would allow folks for whom our colour scheme doesn't work to simply turn it off forever by setting it in their shell profile.

Yes that should be a good idea. There is a 'standard' around using NO_COLOR for this https://no-color.org. We could do both NO_COLOR and CODER_NO_COLOR?

Sounds good to me!

johnstcn · 2024-09-13T14:38:53Z

cli/cliui/cliui.go

+	red           = Color("1")
+	green         = Color("2")
+	yellow        = Color("3")
+	magenta       = Color("5")
+	white         = Color("7")
+	brightBlue    = Color("12")
+	brightMagenta = Color("13")


nit: indicate these are ANSI color codes

johnstcn · 2024-09-13T14:39:06Z

cli/cliui/cliui.go

+func ifTerm(f pretty.Formatter) pretty.Formatter {
 	if !isTerm() {
 		return pretty.Nop
 	}
-	return fmt
+	return f


johnstcn · 2024-09-13T14:39:50Z

cmd/cliui/main.go

+	}
+
+	root.Children = append(root.Children, &serpent.Command{
+		Use: "colors",


mafredri

I think the global NoColor var could cause a race in our testing since we run things concurrently. E.g. if multiple tests in cli package set --no-color option, we'd run into it.

Other than that, I think this looks great. Might be worth looking into improving serpent so we can do this in a better way. And perhaps get rid of the package-global state in favor of a "color" instance. That's a much more intrusive change though.

mafredri · 2024-09-13T14:41:43Z

cli/root.go

@@ -461,6 +461,12 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err
 			Value:       serpent.StringOf(&r.globalConfig),
 			Group:       globalGroup,
 		},
+		{
+			Flag:        cliui.NoColorFlag,


Suggestion: I think we should add the env as well here.

mafredri · 2024-09-13T14:43:07Z

cli/cliui/cliui.go

+		// has the flag.
+		if slices.Contains(os.Args, fmt.Sprintf("--%s", NoColorFlag)) {
+			color = termenv.Ascii
+		}


Is this needed? I see that the serpent options modify the global var in this package, so checking if NoColor should suffice?

That said, I really wish we could do it differently as modifying globals is a bit icky 😅

Unfortunately not as NoColor is set after our code runs. This is because there is no (apparent) way to have this initialization code ran before every command but after serpent sets up the options.

mafredri · 2024-09-13T14:47:09Z

cli/cliui/cliui.go

-			pretty.FgColor(Red),
-			pretty.BgColor(color.Color("#2c2c2c")),
+			pretty.FgColor(color.Color("#ED567A")),
+			pretty.BgColor(color.Color("#2C2C2C")),


Were there no acceptable ANSI colors to replace these? Why did we go from red to ed567a?

As we set both the foreground and background colour, we have complete control over the contrast so this looks the same regardless of the terminal theme. If we want to use an ANSI code instead we can but there isn't a need to here.

red was defined as #ED567A, and the new red didn't look as good on the background so I kept it as the old one.

johnstcn · 2024-09-13T15:06:18Z

I think the global NoColor var could cause a race in our testing since we run things concurrently.

@mafredri potentially, yes -- but we already check for test.v and skip colours. So I don't think we'd even hit this as long as the check for test.v came first.

DanielleMaywood · 2024-09-13T15:10:14Z

I think the global NoColor var could cause a race in our testing since we run things concurrently. E.g. if multiple tests in cli package set --no-color option, we'd run into it.

I should probably have documented this but we don't actually use the global var, I needed it to set it in cmd/cliui but it appears now removing it from cmd/coder doesn't cause the same issue so I should be able to remove this.

We can't make use of it as serpent sets it too late as we can't hook into serpent before it run commands.

mafredri · 2024-09-13T15:11:20Z

@mafredri potentially, yes -- but we already check for test.v and skip colours. So I don't think we'd even hit this as long as the check for test.v came first.

Updating the var from multiple locations is still a data race, can be seen in CI here: https://github.com/coder/coder/actions/runs/10851466684/job/30115309148?pr=14665#step:5:5121

Edit: I just checked the first instance of data race and linked it, stack trace doesn't really say that it's due to the global var AFAICT 😂

mafredri · 2024-09-13T15:16:28Z

I should probably have documented this but we don't actually use the global var, I needed it to set it in cmd/cliui but it appears now removing it from cmd/coder doesn't cause the same issue so I should be able to remove this.

Alright, then I think using a local var to the serpent command would be better. 👍🏻 (+ comment explaining it's purpose)

DanielleMaywood · 2024-09-13T15:17:31Z

I should probably have documented this but we don't actually use the global var, I needed it to set it in cmd/cliui but it appears now removing it from cmd/coder doesn't cause the same issue so I should be able to remove this.

Turns out serpent behaves differently with serpent.BoolOf versus serpent.Discard. BoolOf allows just writing --no-color but we can't do that with Discard.

DanielleMaywood · 2024-09-13T15:18:39Z

Alright, then I think using a local var to the serpent command would be better. 👍🏻 (+ comment explaining it's purpose)

So define a local var in root.go such as noColorDiscarded?

mafredri · 2024-09-13T15:25:38Z

cli/cliui/cliui.go

+		// serpent as it isn't possible to create a root middleware that
+		// runs for every command. For now we just check if `os.Args`
+		// has the flag.
+		if slices.Contains(os.Args, fmt.Sprintf("--%s", NoColorFlag)) ||


This should also support --no-color=true and --no-color=true (omitting = means true) to be consistent with our CLI. Ideally we'd rely on serpent parsing here but I understand that not easily achieved.

Can do!

--no-color=true and --no-color=true

Do my eyes deceive me or are these both the same?

A case of copy-pasta 😂, sorry. Meant false for the second.

Got you, just pushed a change with --no-color=true handling. It appears we already handle the --no-color=false case!

mafredri · 2024-09-13T15:27:29Z

So define a local var in root.go such as noColorDiscarded?

Yep, inside func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, error) 👍🏻.

ammario · 2024-09-13T17:29:04Z

cli/cliui/cliui.go

 		if flag.Lookup("test.v") != nil {
 			// Use a consistent colorless profile in tests so that results
 			// are deterministic.
 			color = termenv.Ascii
 		}
+
+		// Currently it appears there is no way to use the flag from


Have you tried setting the option on the root command? It should then evaluate on every command.

ammario · 2024-09-13T17:30:51Z

cli/cliui/cliui.go

+		// serpent as it isn't possible to create a root middleware that
+		// runs for every command. For now we just check if `os.Args`
+		// has the flag.
+		if slices.Contains(os.Args, fmt.Sprintf("--%s", NoColorFlag)) ||


This is super janky. Relying on os.Args makes it difficult to test. Also, someone could modify the option at the CLI layer and easily forget to update this. Have you considered checking the CLI option higher up in the stack?

I absolutely agree with you, however serpent doesn't (from what I can tell) have any way to support this as it lacks the ability to set a middleware that runs before any command.

To demonstrate what I mean, the code in this gist has a Middleware setup but it only runs on the root cmd and not it's children. If you know of a way to have this run before a commands handler but after options are setup I'd love to do that instead!

Scratch that: I think using rootCmd.Walk and injecting the middleware to every child should work? Will have a go at doing that.

I've pushed a change that should be less janky (using the method describe in the prior message), let me know what you think!

If you do the Walk approach we have the flag duplicated on every help, which is obnoxious. Can't you use an Option on the root command?

Sorry I think I explained myself poorly, the Walk was to inject middleware onto every command. The Option was on the root command. This change was backed out anyways as mutating the DefaultStyles after func init() was causing Go's race checker to be unhappy.

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.

DanielleMaywood · 2024-09-13T21:37:58Z

I've ripped out all the logic for --no-color as mutating DefaultStyles whilst the program is running causes a race condition that is non-trivial to solve due to using DefaultStyles around 150 times in the codebase. The behaviour can still be enabled by setting the NO_COLOR (https://no-color.org/) environment variable by using termenv's EnvColorProfile instead of ColorProfile.

ammario · 2024-09-16T18:41:54Z

cmd/cliui/main.go

@@ -37,6 +38,43 @@ func main() {
 		},
 	}

+	root.Children = append(root.Children, &serpent.Command{
+		Use: "colors",


This should be hidden

ammario · 2024-09-16T18:43:30Z

cmd/cliui/main.go

+		Use: "colors",
+		Handler: func(inv *serpent.Invocation) error {
+			msg := pretty.Sprint(cliui.DefaultStyles.Code, "This is a code message")
+			_, _ = fmt.Fprintln(inv.Stdout, msg)


You can simplify this with pretty.Fprintf

Done! I had to separate the newline from the Fprintf call though as otherwise the newline would get themed.

ammario

Should prob only merge when one of the core devs approve. I generally don't want to take accountability as a review since my knowledge of the internals has atrophied.

mafredri

Looks great. I think relying on ansi colors where we don't use a background, and hex colors where we do, is indeed the simplest approach.

Thanks for working through all the iterations of this and glad we could land on a simple solution in the end. 👍🏻

mafredri · 2024-09-17T12:33:37Z

cli/cliui/cliui.go

 )

 // Color returns a color for the given string.
 func Color(s string) termenv.Color {
 	colorOnce.Do(func() {
-		color = termenv.NewOutput(os.Stdout).ColorProfile()
+		color = termenv.NewOutput(os.Stdout).EnvColorProfile()


Good solution for disabling colors 👍🏻

DanielleMaywood added 2 commits September 13, 2024 08:22

chore: add command for showing colors

5e308df

fix: use ANSI color codes instead of RGB

86175bf

github-actions bot assigned DanielleMaywood Sep 13, 2024

DanielleMaywood added 4 commits September 13, 2024 12:01

feat: add '--no-color' flag

098d35d

fix: revert colors

3d13060

chore: change colors

5b8fa3b

fix: update golden files

4ce84a4

DanielleMaywood force-pushed the dm-cli-color-theme-change branch from d97280b to 4ce84a4 Compare September 13, 2024 12:54

fix: replace blue with brightBlue

a75dbb2

DanielleMaywood marked this pull request as ready for review September 13, 2024 13:53

DanielleMaywood marked this pull request as draft September 13, 2024 14:01

DanielleMaywood added 2 commits September 13, 2024 14:06

fix: drop '> ' for unfocused prompts

390a7ca

fix: run 'make fmt'

7e6db79

DanielleMaywood marked this pull request as ready for review September 13, 2024 14:24

johnstcn approved these changes Sep 13, 2024

View reviewed changes

chore: allow disabling color with env flags

ce913a5

mafredri reviewed Sep 13, 2024

View reviewed changes

DanielleMaywood added 2 commits September 13, 2024 15:36

fix: apply fixes from feedback

22a2d0b

fix: run 'make gen'

5e42118

ammario reviewed Sep 13, 2024

View reviewed changes

DanielleMaywood added 2 commits September 13, 2024 19:23

fix: refactor janky code

40eb24e

fix: re-add public function

ccf174a

DanielleMaywood added 6 commits September 13, 2024 19:41

fix: re-add init for non-color tests

1fbef1d

fix: move styles to 'init' that can be

44c3726

fix: stop overwriting entire DefaultStyles

d16f625

fix: make code and field obey --no-color

1babe96

fix: apply nit

6d93c56

DanielleMaywood requested a review from ammario September 16, 2024 18:29

ammario requested changes Sep 16, 2024

View reviewed changes

DanielleMaywood added 3 commits September 16, 2024 18:58

fix: simplify code && hide command

506aa28

fix: newline shouldn't be themed

997dc43

fix: appease the linter

24a838d

ammario approved these changes Sep 16, 2024

View reviewed changes

DanielleMaywood requested a review from mafredri September 17, 2024 11:17

mafredri approved these changes Sep 17, 2024

View reviewed changes

DanielleMaywood merged commit 14d3e30 into main Sep 17, 2024
26 checks passed

DanielleMaywood deleted the dm-cli-color-theme-change branch September 17, 2024 13:21

github-actions bot locked and limited conversation to collaborators Sep 17, 2024

fix: use ANSI colors codes instead of RGB #14665

fix: use ANSI colors codes instead of RGB #14665

Uh oh!

Conversation

DanielleMaywood commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnstcn left a comment

Choose a reason for hiding this comment

Uh oh!

johnstcn Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnstcn commented Sep 13, 2024

Uh oh!

DanielleMaywood commented Sep 13, 2024

Uh oh!

mafredri commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mafredri commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielleMaywood commented Sep 13, 2024

Uh oh!

DanielleMaywood commented Sep 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri commented Sep 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielleMaywood Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielleMaywood Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielleMaywood commented Sep 13, 2024

DanielleMaywood commented Sep 13, 2024 •

edited

Loading

johnstcn Sep 13, 2024 •

edited

Loading

mafredri commented Sep 13, 2024 •

edited

Loading

mafredri commented Sep 13, 2024 •

edited

Loading

DanielleMaywood Sep 13, 2024 •

edited

Loading

DanielleMaywood Sep 13, 2024 •

edited

Loading