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

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

Open
brlin-tw opened this issue Sep 25, 2024 · 2 comments
Labels

Comments

@brlin-tw
Copy link

brlin-tw commented Sep 25, 2024

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:

  • The output device is a terminal device.
  • The user explicitly overrides the behavior via the --force-color command option or the FORCE_COLOR environment variable.
Issue Type
  • Bug Report
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
  • ansible installation method: pip(via pipx)
  • ansible-lint installation method: pip(via pipx)
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
@brlin-tw brlin-tw added bug new Triage required labels Sep 25, 2024
@ssbarnea ssbarnea removed the new Triage required label Oct 9, 2024
@ssbarnea
Copy link
Member

ssbarnea commented Oct 9, 2024

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
Copy link
Author

brlin-tw commented Oct 9, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

No branches or pull requests

2 participants