From 9b64c4b50192cc0218e8743e2d08ad9424c8fab8 Mon Sep 17 00:00:00 2001 From: Ahmon Dancy Date: Thu, 19 Jan 2023 10:10:16 -0800 Subject: [PATCH 1/3] Added some comments --- lib.py | 4 ++-- patchforreview_remover.py | 25 +++++++++++++++++++++++-- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/lib.py b/lib.py index 7b37174..a9ecb6b 100755 --- a/lib.py +++ b/lib.py @@ -11,7 +11,7 @@ class Client(object): def __init__(self, url, username, key): self.url = url - self.username = username + self.username = username # Unused self.column_cache = {} self.phid_cache = {} self.session = { @@ -251,4 +251,4 @@ class Client(object): } } return self.post('maniphest.search', params)[ - 'data'] \ No newline at end of file + 'data'] diff --git a/patchforreview_remover.py b/patchforreview_remover.py index 9bcf0ba..7d7bb62 100755 --- a/patchforreview_remover.py +++ b/patchforreview_remover.py @@ -12,17 +12,29 @@ class Checker(): self.client = client def check(self, t_id): + """ + Returns true if the Patch-For-Review project should be removed from the Phabricator + task identified 't_id'. + """ phid = self.client.lookupPhid(t_id) return self.phid_check(phid) - def phid_check(self, phid): + def phid_check(self, phid) -> bool: + """ + Returns true if the Patch-For-Review project should be removed from the Phabricator + task identified by 'phid'. + """ gerrit_bot_actions = [] + + # Note that transactions are returned in reverse chronological order (most recent first). for transaction in self.client.getTransactions(phid): if re.findall(re.escape('https://github.com/') + r'.+?/pull', str(transaction)): return False if transaction['authorPHID'] == self.gerrit_bot_phid: gerrit_bot_actions.append(transaction) else: + # If someone other than GerritBot adds the Patch-For-Review project, don't + # auto-remove it. if transaction['type'] == 'projects': check = self.project_patch_for_review_phid in str( transaction['fields']) @@ -44,9 +56,17 @@ class Checker(): r'Change \d+ \*\*(?:merged|abandoned)\*\* by ', raw_comment) + # Append True or False depending on whether the action was to + # open a patch (True) or merge a patch (False) gerrit_patch_status[gerrit_patch_id].append(not(bool(merged))) for patch in gerrit_patch_status: + # The normal sequence of GerritBot transactions for a Gerrit change is "Change + # \d+ had a related patch set uploaded" (indicated by True in + # gerrit_patch_status) eventually followed by "Change \d+ (merged|abandoned) + # by whoever" (indicated by False in gerrit_patch_status). The transactions + # are returned in reverse order so the opened/merged pattern will appear as + # the reverse of [True, False], which is [False, True]. if gerrit_patch_status[patch] != [False, True]: return False return True @@ -54,9 +74,10 @@ class Checker(): client = Client.newFromCreds() +gerrit_bot_phid = 'PHID-USER-idceizaw6elwiwm5xshb' project_patch_for_review_phid = 'PHID-PROJ-onnxucoedheq3jevknyr' checker = Checker( - 'PHID-USER-idceizaw6elwiwm5xshb', + gerrit_bot_phid, project_patch_for_review_phid, client) gen = client.getTasksWithProject(project_patch_for_review_phid) From 10c6aace455d38bd35c44fdbae1750c14c537154 Mon Sep 17 00:00:00 2001 From: Ahmon Dancy Date: Thu, 19 Jan 2023 10:57:12 -0800 Subject: [PATCH 2/3] Fix gerrit_patch_id regexp Change the gerrit_patch_id regexp from r'https://gerrit(?:-test|)\.wikimedia\.org/r/.*(\d+)(?:$|\]\])' to r'https://gerrit(?:-test|)\.wikimedia\.org/r/(\d+)(?:$|\]\])' The .* in the old regexp was collecting most of the digits expected to be matched by the (\d+). For example if the input was https://gerrit.wikimedia.org/r/875932, the collected digits would just be "2". --- patchforreview_remover.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchforreview_remover.py b/patchforreview_remover.py index 7d7bb62..52f5d28 100755 --- a/patchforreview_remover.py +++ b/patchforreview_remover.py @@ -51,7 +51,7 @@ class Checker(): return False raw_comment = case['comments'][0]['content']['raw'] gerrit_patch_id = re.findall( - r'https://gerrit(?:-test|)\.wikimedia\.org/r/.*(\d+)(?:$|\]\])', raw_comment)[0] + r'https://gerrit(?:-test|)\.wikimedia\.org/r/(\d+)(?:$|\]\])', raw_comment)[0] merged = re.findall( r'Change \d+ \*\*(?:merged|abandoned)\*\* by ', raw_comment) From ae3f603e5035db2bda6fbe9be571675fe7916238 Mon Sep 17 00:00:00 2001 From: Ahmon Dancy Date: Thu, 19 Jan 2023 12:10:41 -0800 Subject: [PATCH 3/3] patchforreview_remover.py: Handle bot comments for Gitlab status updates Handle change status report for both Gerrit and Gitlab. Bug: T325297 --- patchforreview_remover.py | 81 +++++++++++++++++++++++++++------------ 1 file changed, 56 insertions(+), 25 deletions(-) diff --git a/patchforreview_remover.py b/patchforreview_remover.py index 52f5d28..184f1fe 100755 --- a/patchforreview_remover.py +++ b/patchforreview_remover.py @@ -19,6 +19,37 @@ class Checker(): phid = self.client.lookupPhid(t_id) return self.phid_check(phid) + def get_change_url(self, raw_comment): + m = re.search(r'https://gerrit(?:-test|)\.wikimedia\.org/r/\d+', raw_comment) + if m: + return m[0] + m = re.search(r'https://gitlab\.wikimedia\.org/repos/.*/-/merge_requests/\d+', raw_comment) + if m: + return m[0] + return None + + def get_operation_type(self, raw_comment, url): + """ + If the operation type can be determined from raw_comment, return + it. It will be the string "open" (first patchset created or merge + request created) or "close" (merge or abandon) + """ + # Gitlab style + if re.search(r"opened " + re.escape(url), raw_comment): + return "open" + + if re.search(r"(merged|closed) " + re.escape(url), raw_comment): + return "close" + + # Gerrit style + if re.search(r"Change \d+ had a related patch set uploaded", raw_comment): + return "open" + + if re.search(r'Change \d+ \*\*(?:merged|abandoned)\*\* by ', raw_comment): + return "close" + + return None + def phid_check(self, phid) -> bool: """ Returns true if the Patch-For-Review project should be removed from the Phabricator @@ -50,15 +81,14 @@ class Checker(): if len(case['comments']) != 1: return False raw_comment = case['comments'][0]['content']['raw'] - gerrit_patch_id = re.findall( - r'https://gerrit(?:-test|)\.wikimedia\.org/r/(\d+)(?:$|\]\])', raw_comment)[0] - merged = re.findall( - r'Change \d+ \*\*(?:merged|abandoned)\*\* by ', - raw_comment) - # Append True or False depending on whether the action was to - # open a patch (True) or merge a patch (False) - gerrit_patch_status[gerrit_patch_id].append(not(bool(merged))) + change_url = self.get_change_url(raw_comment) + if change_url: + op = self.get_operation_type(raw_comment, change_url) + + # Append True or False depending on whether the action was to + # open/reopen a change (True) or merge a change (False) + gerrit_patch_status[change_url].append(op in ["open", "reopen"]) for patch in gerrit_patch_status: # The normal sequence of GerritBot transactions for a Gerrit change is "Change @@ -67,25 +97,26 @@ class Checker(): # by whoever" (indicated by False in gerrit_patch_status). The transactions # are returned in reverse order so the opened/merged pattern will appear as # the reverse of [True, False], which is [False, True]. + # FIXME: This logic can't handle a open/close/reopen/merge situation. if gerrit_patch_status[patch] != [False, True]: return False return True +if __name__ == "__main__": + client = Client.newFromCreds() -client = Client.newFromCreds() - -gerrit_bot_phid = 'PHID-USER-idceizaw6elwiwm5xshb' -project_patch_for_review_phid = 'PHID-PROJ-onnxucoedheq3jevknyr' -checker = Checker( - gerrit_bot_phid, - project_patch_for_review_phid, - client) -gen = client.getTasksWithProject(project_patch_for_review_phid) -for phid in gen: - if checker.phid_check(phid): - print(client.taskDetails(phid)['id']) - try: - client.removeProjectByPhid(project_patch_for_review_phid, phid) - except BaseException: - continue - time.sleep(10) + gerrit_bot_phid = 'PHID-USER-idceizaw6elwiwm5xshb' + project_patch_for_review_phid = 'PHID-PROJ-onnxucoedheq3jevknyr' + checker = Checker( + gerrit_bot_phid, + project_patch_for_review_phid, + client) + gen = client.getTasksWithProject(project_patch_for_review_phid) + for phid in gen: + if checker.phid_check(phid): + print(client.taskDetails(phid)['id']) + try: + client.removeProjectByPhid(project_patch_for_review_phid, phid) + except BaseException: + continue + time.sleep(10)