Command to re-notify maintainers of a PR
adamjstewart opened this issue · 13 comments
Right now, @spackbot only requests reviews from maintainers when a PR is first opened. This is on purpose: if a maintainer does not belong to the "maintainers" permissions group, we don't want to continuously ping them via comment after every single commit.
However, sometimes @spackbot doesn't work for unknown reasons (spack/spack#24939). Also, there are many PRs that were opened before @spackbot existed that we would like to be able to automatically notify maintainers (spack/spack#23382 (comment)). Even for more recently opened PRs, people often modify additional packages in later commits.
I see two possible solutions to this problem:
- Add a command like
@spackbot maintainersor@spackbot request reviewto manually force @spackbot to run its maintainer functionality (no preference for command name) - Notify maintainers on follow-up commits (not just when a PR is opened) but in a limited mode where we request review of maintainers with "maintainer" permissions but don't comment for other maintainers
I think we want to support both of these if possible.
However, sometimes @spackbot doesn't work for unknown reasons (spack/spack#24939).
I've seen this too on my local development server, and it's usually the case that the server doesn't see anything (e.g., it's not that the request comes in and then the server doesn't respond appropriately, it simply doesn't see any request!) And a restart always seems to resolve it. It could be many things - either the server is receiving so many requests that it misses a few for some unknown reason, or on the other end GitHub is sending out so many that it misses them. For example, turning on "check_run" event notifications just for the style means that we greatly increase the number of requests that come in.
My thinking is that we should try disabling check runs (meaning spackbot won't say "hey I can fix the style!" and seeing if we get fewer random failures - the event is not worth having if it clogs up the event stream to the extent that the more useful ones don't work.
Add a command like @spackbot maintainers or @spackbot request review to manually force @spackbot to run its maintainer functionality (no preference for command name)
I like this approach because it makes the re-notification at the decision / hands of a person with write.
Notify maintainers on follow-up commits (not just when a PR is opened) but in a limited mode where we request review of maintainers with "maintainer" permissions but don't comment for other maintainers
I'm less excited about this one because for PRs where there are a lot of new commits it's going to be noisy / possibly get very annoying. If we get the functionality we need with 1. then I don't think we need this second one.
I'm less excited about this one because for PRs where there are a lot of new commits it's going to be noisy / possibly get very annoying.
Why would it be noisy? The bot won't comment on the PR after the first commit. And if someone is already a reviewerer, we don't have to re-request review.
If a maintainer is already pinged for a PR, that signs them up for notifications. So it would be extra noise.
But that is already true for new commits added by the PR author. The bot possibly adding more people as reviewers won't generate additional notifications will it? (I'm not sure about email notifications since I don't use them).
It would be two emails. Try assigning me to something and then also AT-ing me and I can verify!
The bot possibly adding more people as reviewers won't generate additional notifications will it?
Yeah, it does -- if you're mentioned, requested for review, and a few other reasons, GitHUb subscribes you to the issue. There are little cc's like mention@noreply.github.com that let you know why/filter but yeah once you're in an issue you're IN the issue.
It wouldn't be the same as assigning and AT-ing, it would be the same as requesting review and re-requesting review. I assume it works similarly as labels where if the label is already present it won't be re-added, but I'm not 100% sure.
Four emails
It's not so bad with threaded, but now that I've seen several clients on MacOs that don't nicely do that, it would be annoying to receive there. And for my email that I'm showing here, it's not so bad if I'm afk and then I see them all at once (and there aren't any more coming in) but if I were to read the thread after 2 were sent (and then get two new ones) it's a slight annoyance that will make me less likely to want to participate later (or more likely to ignore). Either way, I think we need to be really careful and conservative about making the bot ping/assign more than necessary. If there is already a even at the opening of a PR to do this maintainer search, imho we should not keep trying to do it again. And if there is now a command that someone can explicitly run (to say, look again for reviewers for a newly added package) that meets the need for that use case.
Yes, and those 4 emails have nothing to do with @spackbot, they occur naturally when someone comments on an issue/PR that you are subscribed to.
As I mentioned above, @spackbot should not comment on PRs after every commit. This will result in extraneous emails. What I'm trying to suggest is that @spackbot add reviewers to the PR after every commit. If someone is already listed as a reviewer, I don't think GitHub will send out an additional email. Maybe someone can confirm/deny this using #24?
The four emails were a dummy example - three were comments because I'm already subscribed, and the fourth was an assignment (which would be akin to if @spackbot had assigned / requested me to review again). It's an extra email.
I think I'm having trouble understanding you - you aren't suggesting to re-run the assignment email after every commit, but you are suggesting to look for new reviewers after every commit and then only assign new ones. I would guess that the chance that a new package file is added to a PR is small, but the cost of doing the check again is pretty big - so I'm not in favor of this feature. If this does happen, the maintainer (spack maintainer, not package maintainer) can explicitly ask spackbot to look again.
the fourth was an assignment (which would be akin to if @spackbot had assigned / requested me to review again). It's an extra email.
Is that akin to @spackbot requesting you to review again, or is that only akin to @spackbot requesting you to review the first time?
I think I'm having trouble understanding you
Right now, @spackbot does three things when you open a PR:
- Request a review from maintainers with "maintainer" privileges
- Comment on the PR for maintainers without "maintainer" privileges
- Add labels to the PR based on the modified files
On additional commits, we only run 3, not 1 or 2. What I'm suggesting is to add 1 to additional commits.
At least with labels, if a label is already present, GitHub doesn't tell you that it added a label on the second commit because that label is already present. I'm hoping/wondering if the same is true for reviewers (i.e. if a review is already requested, a second email shouldn't occur).
Once #24 is deployed, this should be easy to test. Just run @spackbot maintainers 3 times and see if you get 3 emails (for the 3 comments that were added) or 6 emails (for the 3 comments and being requested to review 3 times).
Is that akin to @spackbot requesting you to review again, or is that only akin to @spackbot requesting you to review the first time?
You'll get an email both times - the content is different. One will say "spackbot is requesting your review" and the next will say "spackbot is re-requesting your review." You'd have to get the current reviewers on the PR and then only assign someone that isn't already a reviewer.
On additional commits, we only run 3, not 1 or 2. What I'm suggesting is to add 1 to additional commits.
Yeah I think I understand, and I am not in favor of that. I think maintainers and labels should be done once when the PR is opened, automated, and any huge changes to the content of the PR should be under the spack maintainers decision to ask spackbot for a specific update. If you love this feature and want to add it I won't argue, but I don't like it so I'm not offering to implement it.
I think we should be implementing features that have a huge gain for making PRs easier to manage, but doing so with a mindset of being conservative to minimize the number of automated things that spackbot tries to do. Firstly to not be annoying, but secondly to be mindful that this is a service that is running and the more actions / checks that are run, the more likely that something will bug out (which we are already seeing and haven't figured out yet) and the more costly it is to run the service. My strategy is generally to maximize the automation that is useful, and make the maintainer empowered to ask for more if needed, but don't do more if it's not.
You'll get an email both times - the content is different. One will say "spackbot is requesting your review" and the next will say "spackbot is re-requesting your review." You'd have to get the current reviewers on the PR and then only assign someone that isn't already a reviewer.
Ah damn, nevermind then.
I still think it's worth doing so that we don't accidentally merge a PR that modifies a package without giving the package's maintainers a chance to review. But we can try to keep track of a PR and see if any new files are added and manually run @spackbot maintainers if that happens. Hopefully no one forgets to do this...