Skip to content

Implement terminal color using wrapper around io.Writer#856

Closed
bobcatfish wants to merge 1 commit intoGoogleContainerTools:masterfrom
bobcatfish:color_output_writer
Closed

Implement terminal color using wrapper around io.Writer#856
bobcatfish wants to merge 1 commit intoGoogleContainerTools:masterfrom
bobcatfish:color_output_writer

Conversation

@bobcatfish
Copy link
Copy Markdown
Contributor

(I don't want to actually propose this as a solution but I want to make a PR to demonstrate what this proposal would look like)

In #850 I proposed an implementation of terminal color which involves
instantiating a formatter object whenever you want to log to the
terminal in color. This PR instead creates a wrapper around io.Writer
instead, so that theoretically everything output to the terminal will
magically be the right color without callers needing to be aware.

That was how it was in the first iteration HOWEVER when I ran skaffold dev everything was blue, including output from docker, etc. It became
clear that we needed to often use the output writer which didn't apply
formatting, so in the end we need to change pretty much every function
that took io.Writer to take color.Writer instead, so we can control
whether the functions they call output in color or not.

The end result is that when you look at some code, if it still takes
io.Writer as a param, it's very hard to know if that will be in color or
not and I feel this implementation is more confusing and less
maintainable than #850.

(This was a possible fix for #835.)

In GoogleContainerTools#850 I proposed an implementation of terminal color which involves
instantiating a formatter object whenever you want to log to the
terminal in color. This PR instead creates a wrapper around io.Writer
instead, so that theoretically everything output to the terminal will
magically be the right color without callers needing to be aware.

That was how it was in the first iteration HOWEVER when I ran `skaffold
dev` _everything_ was blue, including output from docker, etc. It became
clear that we needed to often use the output writer which didn't apply
formatting, so in the end we need to change pretty much every function
that took io.Writer to take color.Writer instead, so we can control
whether the functions they call output in color or not.

The end result is that when you look at some code, if it still takes
io.Writer as a param, it's very hard to know if that will be in color or
not and I feel this implementation is more confusing and less
maintainable than GoogleContainerTools#850.

(This was a possible fix for GoogleContainerTools#835.)
@bobcatfish
Copy link
Copy Markdown
Contributor Author

bobcatfish commented Jul 27, 2018

Initially I didn't like this idea (when discussed with @r2d4) but after looking at similar libs (such as tabwriter and the docker cli) it seemed reasonable. But when I actually tried it, it spread everywhere in the codebase and was very hard to reason about.

bobcatfish added a commit to bobcatfish/skaffold that referenced this pull request Jul 27, 2018
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)
bobcatfish added a commit to bobcatfish/skaffold that referenced this pull request Jul 27, 2018
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)
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.

1 participant