ansible / ansible-lint

ansible-lint checks playbooks for practices and behavior that could potentially be improved and can fix some of the most common ones for you
https://ansible.readthedocs.io/projects/lint/
GNU General Public License v3.0
3.46k stars 656 forks source link

Ansible-lint shouldn't output colorized output when the output device is not a terminal #4347

Open brlin-tw opened 2 weeks ago

brlin-tw commented 2 weeks ago
Summary

Currently, Ansible-lint will output ASCII escape sequences that construct colorized output to the stderr even when it is not a terminal that can actually interpret them:

$ ansible-lint /dev/null 2>out
$ xxd out | head --lines=1
00000000: 0a1b 5b33 326d 5061 7373 6564 1b5b 306d  ..[32mPassed.[0m

This causes unreadable output in applications that don't expect such output such as Git Cola:

Screenshot that depicts the issue(unreadable ASCII escape sequences appear in the error output dialog or the Git Cola application)

Please only output colorized messages when:

Issue Type
OS / ENVIRONMENT
ansible-lint 24.9.2 using ansible-core:2.17.4 ansible-compat:24.9.1 ruamel-yaml:0.18.6 ruamel-yaml-clib:0.2.8
STEPS TO REPRODUCE

Run the following commands in a text terminal on a Unix-like operating system:

ansible-lint /dev/null 2>out
xxd out | head --lines=1
Desired Behavior

ASCII escape sequences do not appear in the output, as in:

$ ansible-lint --nocolor /dev/null 2>out
$ xxd out | head --lines=1
00000000: 0a50 6173 7365 643a 2030 2066 6169 6c75  .Passed: 0 failu
Actual Behavior

ASCII escape sequences do appear in the output.

$ ansible-lint /dev/null 2>out
$ xxd out | head --lines=1
00000000: 0a1b 5b33 326d 5061 7373 6564 1b5b 306d  ..[32mPassed.[0m
ssbarnea commented 4 days ago

Please provide more context on reproducing this because we already have a complex logic for determining if the output will be colored or not.

Take a look at should_do_markup method and put some breakpoints there to spot why is not behaving the way you expect. You might discover that it has good reasons to do it.

brlin-tw commented 4 days ago

@ssbarnea

Please provide more context on reproducing this

The commands in the STEPS TO REPRODUCE section of the original report should be sufficient to reproduce the issue. I've updated the original report to be more specific.

Take a look at should_do_markup method and put some breakpoints there to spot why is not behaving the way you expect.

One problem I noticed is that the should_do_markup method seems to be checking the standard output device to determine whether the output should be colorized, while most of Ansible-lint's output seems to be outputted to stderr.

Also stream.isatty() is only consulted as a final fallback, IMHO the reasoning behind doing so doesn't really make sense as:

  • stdin.isatty() is the only one returning true, even on a real terminal

Shouldn't this be a bug in the implementation of stdin.isatty()?

  • stderr returning false if user user uses a error stream coloring solution

If the user has a specific use case that requires colorized output even when the output device isn't a terminal, they should specify --force-color(or its equivalent) instead of relying on this fallback behavior.