Add pop of color to terminal output with a color formatter#857
Conversation
bobcatfish
requested review from
dgageot,
dlorenc,
r2d4 and
viglesiasce
as code owners
pkg/skaffold/deploy/helm.go
Outdated
| } | ||
| if err := h.helm(out, "get", releaseName); err != nil { | ||
| fmt.Fprintf(out, "Helm release %s not installed. Installing...\n", releaseName) | ||
| color.Fprintf(out, color.Red, "Helm release %s not installed. Installing...\n", releaseName) |
There was a problem hiding this comment.
I made this red just for fun!
r2d4
left a comment
r2d4
left a comment
There was a problem hiding this comment.
LGTM. We can merge the existing "colorpicker" codes with the new ones in either this PR or a new PR at some point.
pkg/skaffold/color/colors.go
Outdated
|
|
||
| var ( | ||
| // LightRed can format text to be displayed to the terminal in light red, using ANSI escape codes. | ||
| LightRed = Color(91) |
There was a problem hiding this comment.
Would it make sense to merge this with https://github.com/GoogleContainerTools/skaffold/pull/857/files#diff-bbfb2ceedb52229e4fa6b1f5e4cc8bcfR27?
There was a problem hiding this comment.
The other file looks like it's duplicating this but actually it is only specifying the order of the colors to use when coloring pod output
pkg/skaffold/color/colors.go
Outdated
| @@ -0,0 +1,69 @@ | |||
| /* | |||
There was a problem hiding this comment.
One kind of odd nit:
Maybe package pkg/skaffold/util/color?
There was a problem hiding this comment.
I'm guessing you want to do this b/c the color functionality isn't really a "core part" of skaffold, it's like an extra utility, and I see what you're saying, but I find that calling packages util or lib or anything like that b/c it makes it very hard to reason about what should and should not be inside them. I'd be more inclined to take the contents of pkg/skaffold/util/ and move them into their own packages.
See "bad package names" at https://blog.golang.org/package-names
There was a problem hiding this comment.
But also it's not great that the Color type is color.Color :(
dgageot
left a comment
dgageot
left a comment
There was a problem hiding this comment.
small nitpick. Other than that LGTM!
cmd/skaffold/app/cmd/fix.go
Outdated
| } | ||
| if cfg.GetVersion() == config.LatestVersion { | ||
| fmt.Fprintln(out, "config is already latest version") | ||
| color.Fprintln(out, color.Default, "config is already latest version") |
There was a problem hiding this comment.
What about color.Default.Fprintln(out, "config is already latest version")? If you like it, it can be done in another PR.
There was a problem hiding this comment.
ooooo that's a cool idea
|
daviiiiiiiiiiiid |
This implementation avoids needing to instantiate a formatter object (as seen in GoogleContainerTools#850) by using a global function pointer to override the IsTerminal logic. This means any code at any time could output in whatever color it wants to. (Thanks @r2d4 for suggesting this implementation in the first place!) Skaffold draws a distinction between log output, which goes to stdout, and output from Skaffold to the user, which goes to stdout and is interleaved with output from the tools Skaffold users (e.g. docker for building). This commit changes the output from Skaffold to the user to be Blue! The escape codes to make the text blue (or other colors, in the case of the kube pod-specific logging) will only be output if stdout is a terminal. If output is being redirected to a file for example, the escape codes will not be included. This change makes use of the exciting colors.go logic but splits it into the picker (specifically for mapping images to colors consistently) from the logic to add escape codes. Other alternatives: * GoogleContainerTools#856 - Wrap io.Writer * GoogleContainerTools#850 - Instantiate a formatter object * Separate formatting logic from writing logic (I tried this out and it didn't seem like it improved anything as it didn't align well with how `fmt` is implemented)
Before this change, if you redirected output to a file, you'd get the ANSI color escape codes as well surrounding the name of the pod. Now they will only be output if the output is a terminal.
dgageot@ suggested that instead of passing the color into the formatting
methods, we could make these methods be on the color struct itself,
which makes for a pretty slick interface imo.
One thing that still isn't clear is how to output one line in multiple
colors in a terminal safe way - Sprint/Sprintf would have to bebe aware
of the target output stream to make this happen, or we'd need to
introduce some kind of formatting syntax (e.g. '{blue}blue
text{reset:blue}') but I think that can wait until we really need it,
the workaround in log.go seems find for now.
bobcatfish
force-pushed
the
color_output_globalish
branch
from
f16cefe to
b7ee990
Compare
|
Made the change you suggested @dgageot ! |
|
LGTM! |
3 participants
This implementation avoids needing to instantiate a formatter object
(as seen in #850) by using a global function pointer to override the
IsTerminal logic. This means any code at any time could output in
whatever color it wants to. (Thanks @r2d4 for suggesting this
implementation in the first place!)
Skaffold draws a distinction between log output, which goes to stdout,
and output from Skaffold to the user, which goes to stdout and is
interleaved with output from the tools Skaffold users (e.g. docker for
building). This commit changes the output from Skaffold to the user to
be Blue!
The escape codes to make the text blue (or other colors, in the case of
the kube pod-specific logging) will only be output if stdout is a
terminal. If output is being redirected to a file for example, the
escape codes will not be included.
This change makes use of the exciting colors.go logic but splits it
into the picker (specifically for mapping images to colors consistently)
from the logic to add escape codes.
Other alternatives:
didn't seem like it improved anything as it didn't align well with
how
fmtis implemented)