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

Output color #1700

Open
rmoriz opened this Issue Apr 23, 2017 · 18 comments

Comments

Projects
None yet
8 participants
@rmoriz
Copy link

rmoriz commented Apr 23, 2017

Why are successfull tests in Travis displayed in red?

job 640 5 - rabbitmq_chef-cookbook - travis ci

example link: https://travis-ci.org/rabbitmq/chef-cookbook/jobs/225025268#L559

@adamleff

This comment has been minimized.

Copy link
Contributor

adamleff commented Apr 26, 2017

That's a great question :) Definitely a bug we need to look into, even if it's just occurring on Travis.

@adamleff adamleff added the Type: Bug label Apr 26, 2017

@madAndroid

This comment has been minimized.

Copy link

madAndroid commented May 2, 2017

We're also experiencing the same in Jenkins:

validate-ami__2_console__jenkins_

@mmb

This comment has been minimized.

Copy link

mmb commented Nov 27, 2017

I see the same issue with Concourse CI.

I think this is due to the use of colors which require a 256 color terminal. Could inspec be changed to use only basic colors?

@ghost

This comment has been minimized.

Copy link

ghost commented Apr 24, 2018

The same thing happens in TeamCity

@frank-carnovale

This comment has been minimized.

Copy link

frank-carnovale commented Jun 27, 2018

And in Jenkins
correction.. in Jenkins, a fix was to upgrade the 'AnsiColor' Plugin to 0.5.2.

@mmb

This comment has been minimized.

Copy link

mmb commented Jun 28, 2018

I'll send a PR to change it to regular green if it will be accepted.

@wyardley

This comment has been minimized.

Copy link

wyardley commented Nov 10, 2018

I have this problem in CircleCI as well (their default TERM setting is 'dumb', but did try forcing it to xterm). If I run the same job in Circle via ssh, my term gets set to xterm-xfree86 and it displays just fine.

@mmb did you ever submit that PR? I think the regular green actually is easier on the eyes anyway.

@wyardley

This comment has been minimized.

Copy link

wyardley commented Nov 10, 2018

COLORS = {
'critical' => "\033[0;1;31m",
'major' => "\033[0;1;31m",
'minor' => "\033[0;36m",
'failed' => "\033[0;1;31m",
'passed' => "\033[0;1;32m",
'skipped' => "\033[0;37m",
'reset' => "\033[0m",
}.freeze
# Most currently available Windows terminals have poor support
# for UTF-8 characters so use these boring indicators
INDICATORS = {
'critical' => ' [CRIT] ',
'major' => ' [MAJR] ',
'minor' => ' [MINR] ',
'failed' => ' [FAIL] ',
'skipped' => ' [SKIP] ',
'passed' => ' [PASS] ',
'unknown' => ' [UNKN] ',
'empty' => ' ',
'small' => ' ',
}.freeze
else
# Extended colors for everyone else
COLORS = {
'critical' => "\033[38;5;9m",
'major' => "\033[38;5;208m",
'minor' => "\033[0;36m",
'failed' => "\033[38;5;9m",
'passed' => "\033[38;5;41m",
'skipped' => "\033[38;5;247m",
'reset' => "\033[0m",
}.freeze
# Groovy UTF-8 characters for everyone else...
# ...even though they probably only work on Mac
INDICATORS = {
'critical' => ' × ',
'major' => '',
'minor' => '',
'failed' => ' × ',
'skipped' => '',
'passed' => '',
'unknown' => ' ? ',
'empty' => ' ',
'small' => ' ',
}.freeze

Seems like this is the section. What do folks think makes the most sense? key on TERM=dumb as well as the platform selector? Or maybe switch to 'color' or something similar, rather than using escape codes directly?

@clintoncwolfe

This comment has been minimized.

Copy link
Contributor

clintoncwolfe commented Nov 14, 2018

Note: InSpec v1 is generally not receiving noncritical updates at this point.

On InSpec v3 / master, the color codes are located in https://github.com/inspec/inspec/blob/master/lib/inspec/reporters/cli.rb .

@wyardley

This comment has been minimized.

Copy link

wyardley commented Nov 14, 2018

@clintoncwolfe Phew, thanks for pointing that out [that was actually the one I was editing in my branch]..

A naive and pretty narrow fix would be to replace that case selector with something like
if RUBY_PLATFORM =~ /windows|mswin|msys|mingw|cygwin/ or ENV['TERM'] == 'dumb'
but I feel like the cleanest fix would be to look for explicit TERM settings likely to have 256 colors and / or the setting of $COLORTERM, and default to standard colors otherwise?

@dev-rowbot

This comment has been minimized.

Copy link

dev-rowbot commented Jan 10, 2019

@wyardley @clintoncwolfe I am looking in to a similar issue - we use Rundeck for our automation and it attempts to render the ANSI colours in the log output.

The problem is that it can handle basic ANSI colours but the extended colours do not look great - red looks pink for example - and it cannot display the Groovy UTF-8 characters (to quote the source code) which end up just looking bad in the output.

I thought of adding an additional reporter, something like cli-safe which just uses the basic colours and boring indicators.
This would mean duplicating the cli reporter, removing some code, and then duplicating the cli tests. This does not sound like a great solution to me because it would also mean always having to update cli and cli-safe when making any changes or fixes.

Your suggestion of adding a check for ENV['TERM'] == 'dumb' would work for Rundeck but I'm not sure checking $COLORTERM would help since I can see it is not set when Rundeck executes.

I am keen to fix the issue but wanted to know your thoughts on the best way forward?

  • Add additional checks for ENV['TERM'] and possibly ENV['COLORTERM']
  • Add a new cli reporter with basic functionality
  • Add an additional command line flag that will be used by the cli reporter (is this even possible?)
@wyardley

This comment has been minimized.

Copy link

wyardley commented Jan 10, 2019

$COLORTERM would not be set, which is the point, but it doesn’t seem to be universally implemented among terminals that do use extended colors.

Having another flag (—basic-color? Or —extended-color with basic the default) might make the most sense? I think the current approach is maybe a little too magical for not much benefit, but I don’t know if there would be buy-in to simply getting rid of the fancier colors entirely.

@dev-rowbot

This comment has been minimized.

Copy link

dev-rowbot commented Jan 11, 2019

@wyardley sorry I misunderstood how $COLORTERM was being used. I've come across some info here which echo's what you're saying about there being no universal implementation - https://gist.github.com/XVilka/8346728

I think the flag idea may be a good solution. I would suggest --basic-color with the current behaviour of attempting to use extended colours being the default to avoid breaking the behaviour unexpectedly.

@clintoncwolfe

This comment has been minimized.

Copy link
Contributor

clintoncwolfe commented Jan 11, 2019

We currently have --color and -no-color, a pair of boolean options. We could make --color take argument, default :fancy (or something). We could then accept --color basic, and if --no-color is seen, set color to 'none'. Thoughts?

@wyardley

This comment has been minimized.

Copy link

wyardley commented Jan 11, 2019

@clintoncwolfe that sounds great to me!
I guess the other question would be whether dumbing down the UTF-8 symbols (as was done in the current test reporter) in certain cases should be tied to that option, or a separate option?

@mmb

This comment has been minimized.

Copy link

mmb commented Jan 12, 2019

Most CI systems have no problem showing the UTF-8 so there may be fewer users who want to disable them.

@wyardley

This comment has been minimized.

Copy link

wyardley commented Jan 12, 2019

@mmb yes, that’s true. But the current test output formatter makes both changes on Windows (hard-coded), so thought maybe they caused an issue there?

@dev-rowbot

This comment has been minimized.

Copy link

dev-rowbot commented Jan 14, 2019

@clintoncwolfe I support the idea of having --color basic.

I assume then that the behaviour would then be

  • --color basic - basic colours and basic indicators
  • --no-color basic - no colour with basic indicators
  • --color and --no-color - same behaviour as before
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.