diff --git a/openqabot/approver.py b/openqabot/approver.py index a49bb6c..cffbea1 100644 --- a/openqabot/approver.py +++ b/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 diff --git a/pylintrc b/pylintrc index ec720fd..a9cd5f4 100644 --- a/pylintrc +++ b/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 diff --git a/tests/test_approve.py b/tests/test_approve.py index 1e2f4d7..951e20e 100644 --- a/tests/test_approve.py +++ b/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 + )