Improve the `ansible-not-available` hint detection

[psss]

The error messages about the missing ansible-playbook command differ, sometimes they are present in the exception message, sometimes in the standard error output. Let's make the detection more robust.

Pull Request Checklist

• [x] implement the feature

[psss]

Example error messages for reference:

Error: exec: "ansible-playbook": executable file not found in $PATH
File 'ansible-playbook' not found.

[happz]

Good idea, I didn't realize there's this kind of opportunity, definitely makes sense to unify. But, I would go further: how about moving the pattern and detection into the Hint class?

• the pattern is now repeated three times, compiled every time. Harder to make a typo, but still possible.
• the pattern is detached from the hint. That might be good, might be bad, it got me thinking it might be good to add "CLI output patterns" to hints, because Ansible won't be the last.
• we already know what hint to print out, ansible-not-available, so we can also asked that hint whether the given output qualifies.

Would it make sense to attach these "CLI patterns" to hints? We already know which hint we want to print, if output qualifies, so we can start there, and it would prevent the repetition of the pattern & make the use simpler. I tried to draft some changes, untested, and most of them are just to make space for the list and keeping printing unified:

diff --git a/tmt/steps/provision/local.py b/tmt/steps/provision/local.py
index eb34de02..51853cca 100644
--- a/tmt/steps/provision/local.py
+++ b/tmt/steps/provision/local.py
@@ -81,10 +81,13 @@ class GuestLocal(tmt.Guest):
             )
             # fmt: on
         except tmt.utils.RunError as exc:
-            if exc.stderr and 'ansible-playbook: command not found' in exc.stderr:
-                from tmt.utils.hints import print_hints
+            from tmt.utils.hints import get_hint
+
+            hint = get_hint('ansible-not-available', ignore_missing=False)
+
+            if hint.search_cli_patterns(exc.stderr, exc.stdout, exc.message):
+                hint.print(self._logger)

-                print_hints('ansible-not-available', logger=self._logger)
             raise exc

     def execute(
diff --git a/tmt/utils/hints.py b/tmt/utils/hints.py
index c1c2cd0e..30c73c04 100644
--- a/tmt/utils/hints.py
+++ b/tmt/utils/hints.py
@@ -27,6 +27,8 @@ on the first line, followed by more details in the rest of the text.
 import functools
 import textwrap
 from collections.abc import Iterator
+from typing import Optional, overload, Literal, Pattern
+import re

 import tmt.container
 import tmt.log
@@ -39,6 +41,7 @@ from tmt.log import Logger
 class Hint:
     id: str
     text: str
+    cli_output_patterns: list[Pattern[str]] = tmt.container.simple_field(default_factory=list[Pattern[str]])

     @functools.cached_property
     def summary(self) -> str:
@@ -54,10 +57,17 @@ class Hint:
         # add ref as well.
         return self.summary

-    def __init__(self, hint_id: str, text: str) -> None:
+    def __init__(
+        self,
+        hint_id: str,
+        text: str,
+        cli_output_patterns: Optional[list[str]] = None
+    ) -> None:
         self.id = hint_id
         self.text = textwrap.dedent(text).strip()

+        self.cli_output_patterns = [re.compile(pattern) for pattern in (cli_output_patterns or [])]
+
     def _render(self, s: str, logger: Logger) -> str:
         if tmt.utils.rest.REST_RENDERING_ALLOWED:
             return tmt.utils.rest.render_rst(s, logger)
@@ -73,10 +83,18 @@ class Hint:
     def render(self, logger: Logger) -> str:
         return self._render(self.text, logger)

+    def print(self, logger: Logger) -> None:
+        logger.info('hint', self.render(logger), color='blue')
+
+    def search_cli_patterns(self, *outputs: Optional[str]) -> bool:
+        ...
+

 HINTS: dict[str, Hint] = {
-    _hint_id: Hint(_hint_id, _hint)
-    for _hint_id, _hint in {
+    _hint_id: Hint(_hint_id, _hint_info)
+        if isinstance(_hint_info, str)
+        else Hint(_hint_id, *_hint_info)
+    for _hint_id, _hint_info in {
         'provision': """
             You can use the ``local`` method to execute tests directly on your localhost.

@@ -87,7 +105,8 @@ HINTS: dict[str, Hint] = {

             See ``tmt run report --help`` for all available report options.
             """,
-        'ansible-not-available': """
+        'ansible-not-available': (
+            """
             Make sure ``ansible-playbook`` is installed, it is required for preparing guests using
             Ansible playbooks.

@@ -98,6 +117,8 @@ HINTS: dict[str, Hint] = {
               package.
             * Users who installed tmt from PyPI should install ``tmt[ansible]`` extra.
             """,
+            [r'ansible-playbook.*not found']
+        ),
         # TODO: once `minute` plugin provides its own hints, we can drop
         # this hint and move it to the plugin.
         'provision/minute': """
@@ -163,6 +184,22 @@ def get_hints(*ids: str, ignore_missing: bool = False) -> list[Hint]:
     return list(_render_mandatory_hints())


+@overload
+def get_hint(id_: str, ignore_missing: Literal[True]) -> Optional[Hint]:
+    pass
+
+
+@overload
+def get_hint(id_: str, ignore_missing: Literal[False]) -> Hint:
+    pass
+
+
+def get_hint(id_: str, ignore_missing: bool = False) -> Optional[Hint]:
+    hints = get_hints(id_, ignore_missing=ignore_missing)
+
+    return hints[0] if hints else None
+
+
 def print_hints(*ids: str, ignore_missing: bool = False, logger: tmt.log.Logger) -> None:
     """
     Display given hints by printing them as info-level messages.
@@ -176,4 +213,4 @@ def print_hints(*ids: str, ignore_missing: bool = False, logger: tmt.log.Logger)
     hints = get_hints(*ids, ignore_missing=ignore_missing)

     for hint in hints:
-        logger.info('hint', hint.render(logger), color='blue')
+        hint.print(logger)

[happz]

LGTM.

Seeing import re I've wondered how the hints.py would look if they could also accept glob patterns, as for cases like ansible-playbook*not found, regex seem like an overkill, but I also wanted to learn how it's done in modern Python as it can be useful elsewhere.

In here it's probably unnecessary complication and extra time-for-review, so just fwiw, and example - 85780c7

That said, if we don't expect any regex-worthy patters, I'd think about using just fnmatch instead.

    return any(
        fnmatch.fnmatch(output, pattern)
        for output in outputs
        if output is not None
        for pattern in patterns
    )

Probably not a big complication to support glob patterns eventually. I, for one, prefer one "language" rather than two; I don't have to switch between two modes of operation when it comes to "match a string against a pattern" kind of tasks, and it makes things easier to reason about for me. That said, if glob patterns become interesting, I would only recommend separating them from regular expressions in the code, preferably on the type level, to prevent any developer from using regular expressions with a glob pattern search and vice versa. With NewType, it should be possible to prevent check_for_pattern_message(['foo.*bar'], [...], use_glob=True) - maybe the dot is a typo, or I should have left use_glob be, and maybe it's completely correct, but it's definitely suspicious. So, bare strings might be risky, NewType & define new types to hold "glob pattern" should help with that concern.

[psss]

Would it make sense to attach these "CLI patterns" to hints? We already know which hint we want to print, if output qualifies, so we can start there, and it would prevent the repetition of the pattern & make the use simpler. I tried to draft some changes, untested, and most of them are just to make space for the list and keeping printing unified:

Nice idea, updated in 68785f6e.

[psss]

Seeing import re I've wondered how the hints.py would look if they could also accept glob patterns, as for cases like ansible-playbook*not found, regex seem like an overkill, but I also wanted to learn how it's done in modern Python as it can be useful elsewhere.

I don't have anything against simple glob matching using fnmatch. On the other hand, we're consistently using regular expressions across the whole code, including when interacting with the user e.g. tmt plan ls and friends, so I'd suggest to stick with re unless there's a tangible benefit in introducing another approach.

[martinhoyer]

Probably not a big complication to support glob patterns eventually. I, for one, prefer one "language" rather than two; I don't have to switch between two modes of operation when it comes to "match a string against a pattern" kind of tasks, and it makes things easier to reason about for me. That said, if glob patterns become interesting, I would only recommend separating them from regular expressions in the code, preferably on the type level, to prevent any developer from using regular expressions with a glob pattern search and vice versa. With NewType, it should be possible to prevent check_for_pattern_message(['foo.*bar'], [...], use_glob=True) - maybe the dot is a typo, or I should have left use_glob be, and maybe it's completely correct, but it's definitely suspicious. So, bare strings might be risky, NewType & define new types to hold "glob pattern" should help with that concern.

+1 make sense to me, thanks!

[psss]

Irrelevant network hiccup issue, everything else green, merging.