-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools: adding reminders for API shephards #17081
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,13 @@ | |
'asraa': 'UKZKCFRTP', | ||
} | ||
|
||
# Only notify API reviewers who aren't maintainers. | ||
# Maintainers are already notified of pending PRs. | ||
API_REVIEWERS = { | ||
'markdroth': 'UMN8K55A6', | ||
'adisuissa': 'UT17EMMTP', | ||
} | ||
|
||
|
||
def get_slo_hours(): | ||
# on Monday, allow for 24h + 48h | ||
|
@@ -44,6 +51,13 @@ def is_waiting(labels): | |
return False | ||
|
||
|
||
def is_api(labels): | ||
for label in labels: | ||
if label.name == 'api': | ||
return True | ||
return False | ||
|
||
|
||
# Generate a pr message, bolding the time if it's out-SLO | ||
def pr_message(pr_age, pr_url, pr_title, delta_days, delta_hours): | ||
if pr_age < datetime.timedelta(hours=get_slo_hours()): | ||
|
@@ -55,11 +69,11 @@ def pr_message(pr_age, pr_url, pr_title, delta_days, delta_hours): | |
|
||
|
||
# Adds reminder lines to the appropriate maintainer to review the assigned PRs | ||
def add_reminders(assignees, maintainers_and_prs, message): | ||
def add_reminders(assignees, maintainers_and_prs, message, maintainers_map): | ||
has_maintainer_assignee = False | ||
for assignee_info in assignees: | ||
assignee = assignee_info.login | ||
if assignee not in MAINTAINERS: | ||
if assignee not in maintainers_map: | ||
continue | ||
has_maintainer_assignee = True | ||
if assignee not in maintainers_and_prs.keys(): | ||
|
@@ -69,6 +83,18 @@ def add_reminders(assignees, maintainers_and_prs, message): | |
return has_maintainer_assignee | ||
|
||
|
||
def needs_api_review(labels, repo, pr_info): | ||
if not (is_api(labels)): | ||
return False | ||
headers, data = repo._requester.requestJsonAndCheck( | ||
"GET", | ||
("https://api.github.com/repos/envoyproxy/envoy/statuses/" + pr_info.head.sha), | ||
) | ||
if (data and data[0]["state"] == 'pending'): | ||
return True | ||
return False | ||
|
||
|
||
def track_prs(): | ||
git = github.Github() | ||
repo = git.get_repo('envoyproxy/envoy') | ||
|
@@ -79,13 +105,18 @@ def track_prs(): | |
maintainers_and_prs = {} | ||
# A placeholder for unassigned PRs, to be sent to #maintainers eventually | ||
maintainers_and_prs['unassigned'] = "" | ||
api_review_and_prs = {} | ||
# Out-SLO PRs to be sent to #envoy-maintainer-oncall | ||
stalled_prs = "" | ||
|
||
# Snag all PRs, including drafts | ||
for pr_info in repo.get_pulls("open", "updated", "desc"): | ||
labels = pr_info.labels | ||
assignees = pr_info.assignees | ||
# If the PR is waiting, continue. | ||
if is_waiting(pr_info.labels): | ||
if is_waiting(labels): | ||
continue | ||
if pr_info.draft: | ||
continue | ||
|
||
# Update the time based on the time zone delta from github's | ||
|
@@ -98,37 +129,38 @@ def track_prs(): | |
# SLO, nudge in bold if not. | ||
message = pr_message(delta, pr_info.html_url, pr_info.title, delta_days, delta_hours) | ||
|
||
if (needs_api_review(labels, repo, pr_info)): | ||
add_reminders(pr_info.assignees, api_review_and_prs, message, API_REVIEWERS) | ||
|
||
# If the PR has been out-SLO for over a day, inform on-call | ||
if delta > datetime.timedelta(hours=get_slo_hours() + 36): | ||
stalled_prs = stalled_prs + message | ||
|
||
# Add a reminder to each maintainer-assigner on the PR. | ||
has_maintainer_assignee = add_reminders(pr_info.assignees, maintainers_and_prs, message) | ||
has_maintainer_assignee = add_reminders( | ||
pr_info.assignees, maintainers_and_prs, message, MAINTAINERS) | ||
|
||
# If there was no maintainer, track it as unassigned. | ||
if not has_maintainer_assignee: | ||
# don't bother assigning maintainer WIPs. | ||
if pr_info.draft and pr_info.user.login in maintainers_and_prs.keys(): | ||
continue | ||
maintainers_and_prs['unassigned'] = maintainers_and_prs['unassigned'] + message | ||
|
||
# Return the dict of {maintainers : PR notifications}, and stalled PRs | ||
return maintainers_and_prs, stalled_prs | ||
return maintainers_and_prs, api_review_and_prs, stalled_prs | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might want to update the comment about this with the return val, but non-blocking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call - will do in a follow-up, as it's late to do today and I'd like to get the blocking bits checked in before tomorrow's run :-) |
||
|
||
|
||
def post_to_maintainers(client, maintainers_and_messages): | ||
# Post updates to individual maintainers | ||
for key in maintainers_and_messages: | ||
message = maintainers_and_messages[key] | ||
def post_to_assignee(client, assignees_and_messages, assignees_map): | ||
# Post updates to individual assignees | ||
for key in assignees_and_messages: | ||
message = assignees_and_messages[key] | ||
|
||
# Only send messages if we have the maintainer UID | ||
if key not in MAINTAINERS: | ||
if key not in assignees_map: | ||
continue | ||
uid = MAINTAINERS[key] | ||
uid = assignees_map[key] | ||
|
||
# Ship messages off to slack. | ||
try: | ||
print(maintainers_and_messages[key]) | ||
print(assignees_and_messages[key]) | ||
response = client.conversations_open(users=uid, text="hello") | ||
channel_id = response["channel"]["id"] | ||
response = client.chat_postMessage(channel=channel_id, text=message) | ||
|
@@ -151,15 +183,16 @@ def post_to_oncall(client, unassigned_prs, out_slo_prs): | |
|
||
|
||
if __name__ == '__main__': | ||
maintainers_and_messages, shephards_and_messages, stalled_prs = track_prs() | ||
|
||
SLACK_BOT_TOKEN = os.getenv('SLACK_BOT_TOKEN') | ||
if not SLACK_BOT_TOKEN: | ||
print( | ||
'Missing SLACK_BOT_TOKEN: please export token from https://api.slack.com/apps/A023NPQQ33K/oauth?' | ||
) | ||
sys.exit(1) | ||
|
||
maintainers_and_messages, stalled_prs = track_prs() | ||
|
||
client = WebClient(token=SLACK_BOT_TOKEN) | ||
post_to_maintainers(client, maintainers_and_messages) | ||
post_to_oncall(client, maintainers_and_messages['unassigned'], stalled_prs) | ||
post_to_assignee(client, shephards_and_messages, API_REVIEWERS) | ||
post_to_assignee(client, maintainers_and_messages, MAINTAINERS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it ever the case that an api PRs are unassigned? I'm pretty sure repokitteh handles that by random assignment anyway though.. so i think that's ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it should always be assigned but we could arguably find it not having an API reviewer if someone removed themselves for vacation. We could have a list for unassigned_api - I'll consider it as a feature if it ever happens or @envoyproxy/api-shepherds tell me it's worth it :-)