Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhancement: Add --color flag to cerbos compile #754

Merged
merged 1 commit into from Mar 22, 2022
Merged

Conversation

haines
Copy link
Member

@haines haines commented Mar 18, 2022

Description

Fixes #752 (the color level is now set for pterm as well as the color library, so --no-color turns off all coloring in the tree output)
Fixes #753

This PR introduces a complimentary --color flag in addition to the existing --no-color.

With the default of --color=auto, the output color level is detected from the environment by checking if stdout is a TTY and looking for various environment variables (see https://pkg.go.dev/github.com/jwalton/go-supportscolor for details).

Otherwise, the output color level is set to:

  • none for --no-color, --color=false, or --color=never;
  • basic for --color, --color=true, or --color=always;
  • 256-color for --color=256; and
  • truecolor for --color=16m, --color=full, or --color=truecolor

This is basically a translation of go-supportscolor's "sniff flags" functionality to make it compatible with Kong (which barfs on unknown flags, so we need to explicitly define them).

By default, the output color level is detected from the environment by checking
if stdout is a TTY and looking for various environment variables.

See https://pkg.go.dev/github.com/jwalton/go-supportscolor for details.

Signed-off-by: Andrew Haines <haines@cerbos.dev>
@haines haines marked this pull request as ready for review March 18, 2022 17:00
Copy link
Contributor

@charithe charithe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I am wondering whether we need the --color flag with all these options though. Wouldn't it be simpler to just have --no-color and if it's not defined, try to detect the colours supported by the TTY? In my mind, getting the colours absolutely right is not necessary for this use case. Do you think it's important to let users tune it?

@haines
Copy link
Member Author

haines commented Mar 22, 2022

Do you think it's important to let users tune it?

I think it's important to at least be able to force color to be on even if stdout is not a tty. I think the colors enhance the legibility of the output, and I regularly find myself wanting tools to support this so that I can have color turned on for CI pipelines or within Docker builds.

Is it important to be able to precisely specify whether you have basic, 256, or true color support? Perhaps less so. I initially wanted to just do

Color *bool `negatable:""`

so that you'd have --color to force color, --no-color to force no color, and neither to detect it based on stdout being a TTY. But Kong doesn't support pointer fields yet (alecthomas/kong#280).

Ultimately I was swayed by the fact that we already were detecting the precise color support for the colored JSON output, so it seemed reasonable to have an escape hatch for when that detection failed. This CLI flag style is widely supported in the Node ecosystem (it originally comes from supports-color, which is used by chalk).

Copy link
Contributor

@charithe charithe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@haines haines merged commit bd06a88 into cerbos:main Mar 22, 2022
@haines haines deleted the color branch March 22, 2022 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable color by default when stdout is not a TTY --output=tree does not respect --no-color
2 participants