Merge pull request #183 from r-richardson/improve_tag_parser_167836

Allow for more tolerant comment parsing

openqabot/approver.py

@@ -7,6 +7,7 @@
 from urllib.error import HTTPError
 from datetime import timedelta, datetime
 import re
+import string
 
 import osc.conf
 import osc.core
@@ -58,6 +59,14 @@ def _handle_http_error(e: HTTPError, inc: IncReq) -> bool:
     return False
 
 
+def sanitize_comment_text(
+    text,
+):
+    text = "".join(x for x in text if x in string.printable)
+    text = text.replace("\r", " ").replace("\n", " ")
+    return text.strip()
+
+
 class Approver:
     def __init__(self, args: Namespace, single_incident=None) -> None:
         self.dry = args.dry
@@ -127,10 +136,13 @@ def _approvable(self, inc: IncReq) -> bool:
 
     @lru_cache(maxsize=512)
     def is_job_marked_acceptable_for_incident(self, job_id: int, inc: int) -> bool:
-        regex = re.compile(r"\@review\:acceptable_for\:incident_%s\:(.+)" % inc)
+        regex = re.compile(
+            r"@review:acceptable_for:incident_%s:(.+?)(?:$|\s)" % inc, re.DOTALL
+        )
         try:
             for comment in self.client.get_job_comments(job_id):
-                if regex.match(comment["text"]):
+                sanitized_text = sanitize_comment_text(comment["text"])
+                if regex.search(sanitized_text):
                     return True
         except RequestError:
             pass

pylintrc

@@ -21,6 +21,7 @@ disable=
   R0913, # too-many-arguments
   R0914, # too-many-locals
   R0915, # too-many-statements
+  R0917, # too-many-positional-arguments
   R1718, # consider-using-set-comprehension
   R1734, # use-list-literal
   W0622, # redefined-builtin

tests/test_approve.py

@@ -588,3 +588,46 @@ def test_approval_still_blocked_if_openqa_older_job_dont_include_incident(
     assert approver() == 0
     messages = [x[-1] for x in caplog.record_tuples]
     assert "* SUSE:Maintenance:2:200" not in messages
+
+
+@responses.activate
+@pytest.mark.parametrize("fake_qem", [("NoResultsError isn't raised")], indirect=True)
+@pytest.mark.parametrize(
+    "comment_text",
+    [
+        "\r\r@review:acceptable_for:incident_2:foo\r",  # Carriage returns
+        "Some irrelevant text\n@review:acceptable_for:incident_2:foo",  # Text before @review
+        "@review:acceptable_for:incident_2:foo\nSome extra text",  # Trailing characters
+        "\x0c@review:acceptable_for:incident_2:foo",  # Non-printable character
+    ],
+)
+def test_approval_unblocked_with_various_comment_formats(
+    fake_qem,
+    comment_text,
+    caplog,
+    fake_openqa_older_jobs_api,
+):
+    caplog.set_level(logging.DEBUG, logger="bot.approver")
+
+    responses.add(
+        responses.GET,
+        f"{QEM_DASHBOARD}api/jobs/update/20005",
+        json=[
+            {"job_id": 100000, "status": "passed"},
+            {"job_id": 100002, "status": "failed"},
+            {"job_id": 100003, "status": "passed"},
+        ],
+    )
+    add_two_passed_response()
+    responses.add(
+        responses.GET,
+        url="http://instance.qa/api/v1/jobs/100002/comments",
+        json=[{"text": comment_text}],
+    )
+    assert approver() == 0
+    messages = [x[-1] for x in caplog.record_tuples]
+    assert "* SUSE:Maintenance:2:200" in messages
+    assert (
+        "Ignoring failed job http://instance.qa/t100002 for incident 2 due to openQA comment"
+        in messages
+    )