Skip to content

Commit

Permalink
[ci] Check more events before pinging reviewers (apache#10208)
Browse files Browse the repository at this point in the history 
* [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 apache#9983

* fix tests

Co-authored-by: driazati <[email protected]>
  • Loading branch information
@driazati
2 people authored and pfk-beta committed Apr 11, 2022
1 parent f454950 commit ca99420
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 32 deletions.
32 changes: 22 additions & 10 deletions tests/python/unittest/test_ci.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -307,19 +307,28 @@ 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(
{
"isDraft": False,
"number": 2,
},
"Checking 0 PRs",
"Checking 0 of 1 fetched",
)

run(
Expand All @@ -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",
Expand All @@ -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
Expand All @@ -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",
},
]
},
},
Expand All @@ -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"},
]
},
},
Expand Down
74 changes: 52 additions & 22 deletions tests/scripts/ping_reviewers.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}") {{
Expand All @@ -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
}}
Expand Down Expand Up @@ -92,26 +95,42 @@ 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
reviews = pr["reviews"]["nodes"]
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"])
Expand All @@ -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

Expand Down Expand Up @@ -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:
Expand Down

0 comments on commit ca99420

Please sign in to comment.