Don't nag on-call to assign a maintainer to a PR that's awaiting someone else's review #39208
Conversation
…eone else's review Signed-off-by: Raven Black <ravenblack@dropbox.com>
| @@ -185,17 +185,23 @@ async def tracked_prs(self): | |||
| if age > self.slo_max + datetime.timedelta(hours=36): | |||
| stalled_prs.append(message) | |||
|
|
|||
| if self.is_contrib(pull): | |||
| # If the PR is a contrib PR, we don't want to assign it to maintainer. | |||
There was a problem hiding this comment.
why not? maintainers still need to land these and are usually the right people
There was a problem hiding this comment.
I don't know, that's already the behavior, I just moved the condition up to here so it doesn't do a bunch of work for no reason in this case.
tools/repo/notify.py
Outdated
| @@ -227,6 +233,12 @@ def is_contrib(self, pr): | |||
| return True | |||
| return False | |||
|
|
|||
| async def is_reviewed(self, pr): | |||
| async for review in self.repo.getiter(f"pulls/{pr['number']}/reviews"): | |||
| if review["state"] == "APPROVED" or "/lgtm" in review["body"]: | |||
There was a problem hiding this comment.
im not too sure about this - it doesnt check who is doing the reviewing at all or why they lgtm - eg it may have only been for api/deps/coverage/etc
how about a slightly more explicit approach - lets add a review:needs-maintainer label
we would then need to add a label setter in repokitteh or gh actions to allow people to set it - but we could eg check they are in reviewers.yaml or a relevant group
There was a problem hiding this comment.
I don't think we'd need to check for permission - this change is only making the nag happen less when it's not necessary. Allowing random strangers to pass by and change the state to needs-maintainer would in the worst case just give us the behavior we already have before this change.
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
Well, seems like we want something better, but my impetus to do it isn't proportionate to the effort for a better version, so I guess we'll just not. |
No description provided.