Ignore drive-by reviews in mergebot #1175
Conversation
| "projectColumn": "Needs Maintainer Action", | ||
| "labels": [ | ||
| "Critical package", | ||
| "Other Approved", |
There was a problem hiding this comment.
I'll note that this basically kills off "Other approved" for PRs that "other" reviews cannot apply to. Maybe that's bad.
| "variables": { | ||
| "input": { | ||
| "subjectId": "MDExOlB1bGxSZXF1ZXN0Mzg4MzgyMzYy", | ||
| "body": "@RReverser Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?\n<!--typescript_bot_stale-ping-f5a3ad-37d784d-->" |
There was a problem hiding this comment.
This is an example of a downside; if randos do come and review, anything they say will be ignored and they won't be pinged.
In this case, I doubt RReverser is a rando 😄
| "projectColumn": "Needs Author Action", | ||
| "labels": [ | ||
| "The CI failed", | ||
| "Owner Approved", |
There was a problem hiding this comment.
This might be a bug; odd to have an owner approval get removed...
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
With the amount of TS team oversight still spent on DT, I'd say that the current state is correct, because we still have the ability to override driveby comments with an approval or merge.
But typically any comment at all means that the PR isn't settled and shouldn't merge. I guess the exceptions are "good, glad we came to an agreement!" kinds of things, but that's less common than "wait wait I disagree" ones. I think.
2 participants
I noticed that anyone can go to any PR and request changes, which makes the bot say "One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!" and applies the "Revision needed" tag.
I feel like this shouldn't be possible; we shouldn't care about reviews from people who can't actually approve the PR.