Skip to content

Add terminal color choosing support#2508

Closed
ulyssessouza wants to merge 1 commit intomoby:masterfrom
ulyssessouza:add-term-colors
Closed

Add terminal color choosing support#2508
ulyssessouza wants to merge 1 commit intomoby:masterfrom
ulyssessouza:add-term-colors

Conversation

@ulyssessouza
Copy link

@ulyssessouza ulyssessouza commented Dec 7, 2021 •
edited
Loading

This adds the capability of choosing the color to print in the terminal by just setting an envvar called BUILDKIT_TERM_COLOR.

Note that this is a "follow-up" of #2368 making it more flexible so that the users can choose it's preferred color depending on their terminal themes and preferences.

It supports the following values:

"default"

"black"
"red"
"green"
"yellow"
"blue"
"magenta"
"cyan"
"white"

"light-black"
"light-red"
"light-green"
"light-yellow"
"light-blue"
"light-magenta"
"light-cyan"
"light-white"

tonistiigi
tonistiigi reviewed Dec 7, 2021
View reviewed changes
util/progress/progressui/colors.go


func getTermColor() aec.ANSI {
envColorString := os.Getenv("BUILDKIT_TERM_COLOR")
Copy link
Member

@tonistiigi tonistiigi Dec 7, 2021

Choose a reason for hiding this comment

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

Is there a convention for this? I know NO_COLOR is a thing but this looks somewhat weird as there isn't just one color. You are changing the color of the completed item but iirc we have 4 possible different colors used by this progressbar.

Copy link
Author

@ulyssessouza ulyssessouza Dec 8, 2021

Choose a reason for hiding this comment

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

AFAIC there is not convention on terminal color naming. I just took the ones from "github.com/morikuni/aec" in lower case.

This PR only affects the "typical buildkit blue" color and not the other 3. It's not a complete theme (I actually like the idea of having it 😺).

@ulyssessouza
Add color choosing support
4ac2fc5 
Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
@tonistiigi
Copy link
Member

tonistiigi commented Dec 10, 2021

@crazy-max wdyt?

I think we could definitely support NO_COLOR and if there is a known convention for passing additional colors via env it would probably be ok. Only one I know about is LS_COLORS but that uses a different format. Do we want to define a similar key=index: format for this? https://geoff.greer.fm/lscolors/ . All this could also be in aec level though.

Currently just having a special env for overriding one specific color seems too hacky to me.

@crazy-max
Copy link
Member

crazy-max commented Dec 10, 2021 •
edited
Loading

Yes agree that we should support NO_COLOR, probably upstream on https://github.com/morikuni/aec like go-ansi has done.

I'm not aware of a convention but we should also support cancel and error states (probably also warning #2498 in the future). Maybe with a key like you said @tonistiigi BUILDKIT_TERM_COLOR=run=blue,cancel=yellow,error=red?

@crazy-max
Copy link
Member

crazy-max commented Dec 17, 2021

As discussed using LS_COLORS lgtm. e.g., LS_COLORS=ru=blue:ca=yellow:er=red:wa=orange

@tonistiigi
Copy link
Member

tonistiigi commented Dec 17, 2021

So not LS_COLORS directly but similar logic. BUILDKIT_COLORS. Or maybe could be AEC_COLORS=31=34: as well.

Keys do not need to be 2 chars and there is no "orange".

@spkane
Copy link

spkane commented Feb 9, 2022

@ulyssessouza Is this PR being actively worked on still, based on the feedback? I would love to see this functionality implemented in buildkit.

@ulyssessouza
Copy link
Author

ulyssessouza commented Feb 10, 2022

@spkane I'll finish that soon... Just trying to make a bit of time here... Wouldn't take much time

@thaJeztah thaJeztah mentioned this pull request May 12, 2022
Open
@spkane spkane mentioned this pull request Jul 8, 2022
Merged
@spkane
Copy link

spkane commented Jul 8, 2022

@ulyssessouza I took a stab at this in another PR (#2954), because I had some time and the desire to try and implement this. I'd appreciate hearing what you think.

@crazy-max
Copy link
Member

crazy-max commented Jul 23, 2022

fixed by #2954

@crazy-max crazy-max closed this Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi tonistiigi left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ulyssessouza @tonistiigi @crazy-max @spkane