Ensure --no-color applies to all Supervisor output
#4334
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request! Here is what will happen next:
Thank you for contributing! |
|
cc: @bixu |
| // | ||
| // There is currently no short option to check; just the long | ||
| // name. | ||
| if args.contains(&String::from("--no-color")) { |
There was a problem hiding this comment.
This seems redundant and could be simplified
if args.contains("--no-color") {
There was a problem hiding this comment.
@georgemarshall Sadly, no 😦 : rust-lang/rust#42671
In several places we were directly embedding ANSI color codes in a log message as a way to emphasize certain parts of that message. However, this bypasses the existing controls we have over whether or not to colorize the Supervisor's output; regardless of your settings, these emphasized parts would *always* be colored. As an initial step to bring colored output under control, we are simply removing all these embedded "emphasis colors". A possible long-term solution would be to introduce a minimal templating language of sorts that allows us to mark particular bits of text for emphasis of one kind or another, allowing the final formatting of a message to be handled centrally. Think of an HTML/CSS-style separation of concerns, but for fancy colored log output. Signed-off-by: Christopher Maier <[email protected]>
Signed-off-by: Christopher Maier <[email protected]>
The `core::output` colorization logic hinges on the value of a global atomic variable. However, the Launcher is a separate process from the Supervisor; any value set in the Supervisor is completely distinct from what the Launcher sees. As a result, the Launcher would *always* colorize output, because colorization currently defaults to `true`. Since the Launcher always has access to the arguments used to start the Supervisor, and since there is only a single option we care about here, it's a simple matter to just check if a `--no-color` argument was passed and then take action accordingly. There's no need to bring `clap` in for something like this. Signed-off-by: Christopher Maier <[email protected]>
Just cuts down on repetition Signed-off-by: Christopher Maier <[email protected]>
We were using `UI::default()` everywhere, which doesn't honor the `--no-color` flag. We still have two different mechanisms governing output (our `output` module and our `ui` module), and should try and unify those as much as is practical. In the meantime, however, we can bridge the gap here. Signed-off-by: Christopher Maier <[email protected]>
christophermaier
force-pushed
the
cm/no-color
branch
from
e2e5c11 to
1f24903
Compare
fnichol
approved these changes
Dec 19, 2017
| }; | ||
|
|
||
| let isatty = if env::var(NONINTERACTIVE_ENVVAR) | ||
| // Keep string boolean for backwards-compatibility |
There was a problem hiding this comment.
It's not exactly backwards-compatibility, it's current 😄
| @@ -372,7 +371,7 @@ impl CfgRenderer { | |||
| cfg_dest.display() | |||
| ); | |||
| outputln!(preamble ctx.svc.group, "Updated {} {}", | |||
| Purple.bold().paint(template.as_str()), | |||
There was a problem hiding this comment.
Purple, wow, we were using all the colors!
|
@thesentinels approve |
|
🤘 I am testing your branch against master before merging it. We do this to ensure that the master branch is never failing tests. |
|
|
Travis CI has
|
💖 Travis CI reports this PR passed. It always makes me feel nice when humans approve of one anothers work. I'm merging this PR now. I just want you and the contributor to answer me one question:
|
christophermaier
added
Focus:Supervisor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should ensure that the
--no-coloroption truly does eliminate all ANSI color codes from Supervisor output (note that this does not apply to any ANSI codes that might be present in the output of any supervised services).There are a few key points:
We were embedding color codes directly in log messages to emphasize certain parts. This was done in a way that completely bypassed any
--no-coloroption; the color would always be there.The Launcher is responsible for routing standard output and error from supervised services, and uses our core output library. However, that library is currently driven by a global variable that is not set in the Launcher (because it's a completely separate process). Now, we ensure that the Launcher uses whatever colorization option is specified by the Supervisor
Some places in the Supervisor commands were using the
UI::default()function to set up their outputs, but this does not honor the--no-coloroption. Now, all uses ofUIin the Supervisor commands goes through a common setup function that does honor this option.Starting up the Supervisor with
--no-colornow should truly configure things so there is no colored output. Future work still exists, though, including but not limited to:--no-colorthe default behavior for non-interactive usesFurther details are in the commit messages.
Note that for users to fully experience this, a new Launcher package will need to be built and promoted.
Fixes #997