From ca99420498f0e8d1f432f85c4da57f5265b7f3e8 Mon Sep 17 00:00:00 2001 From: David Riazati <9407960+driazati@users.noreply.github.com> Date: Thu, 24 Feb 2022 12:53:32 -0800 Subject: [PATCH] [ci] Check more events before pinging reviewers (#10208) * [ci] Check more events before pinging reviewers This was missing some events before (reviews without comments, PR updated from a draft -> ready for review) so these were being ignored when finding the latest event. This PR adds them and restructures the code a bit to make it more clear what is happening for each PR. This addresses some of the issues from #9983 * fix tests Co-authored-by: driazati --- tests/python/unittest/test_ci.py | 32 +++++++++----- tests/scripts/ping_reviewers.py | 74 ++++++++++++++++++++++---------- 2 files changed, 74 insertions(+), 32 deletions(-) diff --git a/tests/python/unittest/test_ci.py b/tests/python/unittest/test_ci.py index d3e7c79b88ed2..6ca46bc60cd57 100644 --- a/tests/python/unittest/test_ci.py +++ b/tests/python/unittest/test_ci.py @@ -307,11 +307,20 @@ def run(pr, check): assert check in proc.stdout + def all_time_keys(time): + return { + "updatedAt": time, + "lastEditedAt": time, + "createdAt": time, + "publishedAt": time, + } + run( { "isDraft": True, + "number": 2, }, - "Checking 0 PRs", + "Checking 0 of 1 fetched", ) run( @@ -319,7 +328,7 @@ def run(pr, check): "isDraft": False, "number": 2, }, - "Checking 0 PRs", + "Checking 0 of 1 fetched", ) run( @@ -330,7 +339,7 @@ def run(pr, check): "isDraft": False, "author": {"login": "user"}, "reviews": {"nodes": []}, - "publishedAt": "2022-01-18T17:54:19Z", + **all_time_keys("2022-01-18T17:54:19Z"), "comments": {"nodes": []}, }, "Pinging reviewers ['someone'] on https://github.com/apache/tvm/pull/123", @@ -345,14 +354,14 @@ def run(pr, check): "isDraft": False, "author": {"login": "user2"}, "reviews": {"nodes": []}, - "publishedAt": "2022-01-18T17:54:19Z", + **all_time_keys("2022-01-18T17:54:19Z"), "comments": { "nodes": [ - {"updatedAt": "2022-01-19T17:54:19Z", "bodyText": "abc"}, + {**all_time_keys("2022-01-19T17:54:19Z"), "bodyText": "abc"}, ] }, }, - "Checking 0 PRs", + "Checking 0 of 1 fetched", ) # Old comment, ping @@ -364,10 +373,13 @@ def run(pr, check): "isDraft": False, "author": {"login": "user"}, "reviews": {"nodes": []}, - "publishedAt": "2022-01-18T17:54:19Z", + **all_time_keys("2022-01-18T17:54:19Z"), "comments": { "nodes": [ - {"updatedAt": "2022-01-19T17:54:19Z", "bodyText": "abc"}, + { + **all_time_keys("2022-01-18T17:54:19Z"), + "bodyText": "abc", + }, ] }, }, @@ -383,10 +395,10 @@ def run(pr, check): "isDraft": False, "author": {"login": "user"}, "reviews": {"nodes": []}, - "publishedAt": "2022-01-18T17:54:19Z", + **all_time_keys("2022-01-18T17:54:19Z"), "comments": { "nodes": [ - {"updatedAt": "2022-01-27T17:54:19Z", "bodyText": "abc"}, + {**all_time_keys("2022-01-27T17:54:19Z"), "bodyText": "abc"}, ] }, }, diff --git a/tests/scripts/ping_reviewers.py b/tests/scripts/ping_reviewers.py index 50e67824888c0..5c881594cf301 100755 --- a/tests/scripts/ping_reviewers.py +++ b/tests/scripts/ping_reviewers.py @@ -32,6 +32,7 @@ def prs_query(user: str, repo: str, cursor: str = None): after = "" if cursor is not None: after = f', before:"{cursor}"' + time_keys = "createdAt\nupdatedAt\nlastEditedAt\npublishedAt" return f""" {{ repository(name: "{repo}", owner: "{user}") {{ @@ -43,27 +44,29 @@ def prs_query(user: str, repo: str, cursor: str = None): number url body + {time_keys} isDraft author {{ login }} reviews(last:100) {{ nodes {{ + {time_keys} + bodyText author {{ login }} comments(last:100) {{ nodes {{ - updatedAt + {time_keys} bodyText }} }} }} }} - publishedAt comments(last:100) {{ nodes {{ authorAssociation bodyText - updatedAt + {time_keys} author {{ login }} @@ -92,8 +95,25 @@ def find_reviewers(body: str) -> List[str]: def check_pr(pr, wait_time, now): - published_at = datetime.datetime.strptime(pr["publishedAt"], GIT_DATE_FORMAT) - last_action = published_at + last_action = None + + def update_last(new_time, description): + if isinstance(new_time, str): + new_time = datetime.datetime.strptime(new_time, GIT_DATE_FORMAT) + if new_time is None: + print(f" time not found: {description}") + return + nonlocal last_action + if last_action is None or new_time > last_action[0]: + last_action = (new_time, description) + + def check_obj(obj, name): + update_last(obj["publishedAt"], f"{name} publishedAt: {obj}") + update_last(obj["updatedAt"], f"{name} updatedAt: {obj}") + update_last(obj["lastEditedAt"], f"{name} lastEditedAt: {obj}") + update_last(obj["createdAt"], f"{name} lastEditedAt: {obj}") + + check_obj(pr, "pr") # GitHub counts comments left as part of a review separately than standalone # comments @@ -101,17 +121,16 @@ def check_pr(pr, wait_time, now): review_comments = [] for review in reviews: review_comments += review["comments"]["nodes"] + check_obj(review, "review") # Collate all comments comments = pr["comments"]["nodes"] + review_comments # Find the last date of any comment for comment in comments: - commented_at = datetime.datetime.strptime(comment["updatedAt"], GIT_DATE_FORMAT) - if commented_at > last_action: - last_action = commented_at + check_obj(comment, "comment") - time_since_last_action = now - last_action + time_since_last_action = now - last_action[0] # Find reviewers in the PR's body pr_body_reviewers = find_reviewers(pr["body"]) @@ -128,17 +147,19 @@ def check_pr(pr, wait_time, now): if time_since_last_action > wait_time: print( - " Pinging reviewers", + " Pinging reviewers", reviewers, "on", pr["url"], "since it has been", time_since_last_action, - "since anything happened on that PR", + f"since anything happened on that PR (last action: {last_action[1]})", ) return reviewers else: - print(f"Not pinging PR {pr['number']}") + print( + f" Not pinging PR {pr['number']} since it has been only {time_since_last_action} since the last action: {last_action[1]}" + ) return None @@ -206,18 +227,27 @@ def ping_reviewers(pr, reviewers): prs = r["data"]["repository"]["pullRequests"]["nodes"] # Don't look at draft PRs at all - prs = [pr for pr in prs if not pr["isDraft"]] - - # Don't look at super old PRs - prs = [pr for pr in prs if pr["number"] > cutoff_pr_number] - - # [slow rollout] - prs = [pr for pr in prs if pr["author"]["login"] in author_allowlist] - - print(f"Checking {len(prs)} PRs: {[pr['number'] for pr in prs]}") + prs_to_check = [] + for pr in prs: + if pr["isDraft"]: + print(f"Skipping #{pr['number']} since it's a draft") + elif pr["number"] <= cutoff_pr_number: + print( + f"Skipping #{pr['number']} since it's too old ({pr['number']} <= {cutoff_pr_number})" + ) + elif pr["author"]["login"] not in author_allowlist: + # [slow rollout] + print( + f"Skipping #{pr['number']} since author {pr['author']['login']} is not in allowlist: {author_allowlist}" + ) + else: + print(f"Checking #{pr['number']}, {author_allowlist}") + prs_to_check.append(pr) + + print(f"Summary: Checking {len(prs_to_check)} of {len(prs)} fetched") # Ping reviewers on each PR in the response if necessary - for pr in prs: + for pr in prs_to_check: print("Checking", pr["url"]) reviewers = check_pr(pr, wait_time, now) if reviewers is not None and not args.dry_run: