Remove enrich as a dependency #2557
Conversation
ssbarnea
force-pushed
the
fix/rm-enrich
branch
from
fd56ae9
to
a7747a8
Compare
| # Based on Ansible implementation | ||
| def to_bool(value: Any) -> bool: | ||
| """Return a bool for the arg.""" | ||
| if value is None or isinstance(value, bool): | ||
| return bool(value) | ||
| if isinstance(value, str): | ||
| value = value.lower() | ||
| if value in ("yes", "on", "1", "true", 1): | ||
| return True | ||
| return False | ||
|
|
||
|
|
||
| def should_do_markup(stream: TextIO = sys.stdout) -> bool: | ||
| """Decide about use of ANSI colors.""" | ||
| py_colors = None | ||
|
|
||
| # https://xkcd.com/927/ | ||
| for env_var in ["PY_COLORS", "CLICOLOR", "FORCE_COLOR", "ANSIBLE_FORCE_COLOR"]: | ||
| value = os.environ.get(env_var, None) | ||
| if value is not None: | ||
| py_colors = to_bool(value) | ||
| break | ||
|
|
||
| # If deliberately disabled colors | ||
| if os.environ.get("NO_COLOR", None): | ||
| return False | ||
|
|
||
| # User configuration requested colors | ||
| if py_colors is not None: | ||
| return to_bool(py_colors) | ||
|
|
||
| term = os.environ.get("TERM", "") | ||
| if "xterm" in term: | ||
| return True | ||
|
|
||
| if term == "dumb": | ||
| return False | ||
|
|
||
| # Use tty detection logic as last resort because there are numerous | ||
| # factors that can make isatty return a misleading value, including: | ||
| # - stdin.isatty() is the only one returning true, even on a real terminal | ||
| # - stderr returning false if user user uses a error stream coloring solution | ||
| return stream.isatty() | ||
|
|
||
|
|
There was a problem hiding this comment.
The comment on this PR and commit message suggest removal of enrich as a dependency is the only change, so the added lines above caught my eye. I recognized those lines of code as being the same as that involved in a stdout processing bug in PR #3699.
I also noted that there were four places that the enrich package was referenced (two with just the package name and no version specification, but from different sources, one with a minimum version >=1.2.6 specified, and the other with explicit version ==1.2.7.) That means four different versions could potentially be involved in different aspects of testing or runtime installation. Removing enrich reduces the potential it would have this problem, but is that the only package with that potential? I'm not sure if a more DRY way to specify packages in all of those places is possible, but you may want to look into it for more consistency.
What might be more problematic here (but also related to DRYness) is that the lines added here to ansible-lint are the same lines of code that exist in molecule. I wanted to make sure that was known.
The potential impact here is that should changes be made to the now-common lines of code in molecule as part of merging PR #3699, those changes would need to be replicated here in ansible-lint as well, else the runtime behavior be different between one case and the other. Because molecule runs ansible-lint, this could result in a bug that would be very difficult to find and fix as one could assume it was resolved by changes to molecule and not even realize the same code existed in ansible-lint running in a subprocess. Since both contain the comment "Based on Ansible implementation" this might indicate the same lines of code also exist in a third project, increasing the random bug problem.
Might it be more robust (more DRY) to have this code exist in someplace like ansible-core and have both molecule and ansible-lint import the functions, as ansible-core is already a dependency of both?
Lastly, I notice a couple issues with the above code related to the way color-capability is being determined. By using the TERM environment variable to infer based only on 'xterm' being found in the string, this leaves out many other terminal types that similarly are color capable. For example, if byobu is used for terminal multiplexing, or either screen or tmux (which byobu front-ends), the TERM environment variable could be something like screen-256color, or tmux-256color, both of which are as much color-capable as xterm, xterm-256color, etc. Even if the test was to look for color in the TERM setting rather than just xterm, I suspect that won't properly identify all terminal types supporting color. The comment in lines 390-393 reflects the difficulty in detection (though it says "stdin.isatty() is the only one returning true", yet the test being called is stream.isatty() and when stream is not passed to the function that would default to testing stdout instead of stdin: either the comment is wrong, or the test result may be the opposite of what is intended).
Users expect to have control of terminal behavior by explicitly setting the TERM environment variable appropriate to the terminal they are using. If a child process of a command line program the user runs in their terminal changes that behavior incorrectly due to an mistaken inference (as opposed to an explicit setting), that can lead to a very frustrating user experience and be extremely difficult to debug. Wouldn't a more opaque method of getting colored output via configuration settings or command line options that is well-documented in both molecule and ansible-lint (for those who use ansible-lint apart from molecule) be more consistent and produce a better user experience?
There was a problem hiding this comment.
@davedittrich I only kept the part of enrich that I needed in ansible-lint. Now that I look at the code, I could simplify it but I do not think it is buggy. The most important part is that it does recognize and respect these env vars related to coloring.
The term part is almost never used, basically for reasons that you mentioned. Not only that there are too many terminals, but they can also easily lie about what they are. I never recommended anyone to alter TERM to enable or disable ANSI coloring.
I think that we will want to move this code to https://github.com/ansible/ansible-compat so we can reuse it from there. I am the author of both libraries (enrich and ansible-compat), but enrich was only created as a temporary wrapper for some rich limitations and I want to slowly get rid of it (also from molecule).
If you want to join forced on this, maybe try to reach me on matrix, or google chat.
There was a problem hiding this comment.
PS. Sorry for not going deeper into this, now is probably the busiest time of the year due to AnsibleFest 2022, but I will more than happy to work on this right after it (Oct 22)
No description provided.