Add the mention bot?

[dstufft]

One feedback people have been having so far is that the firehouse of notifications from Github on python/cpython is too much and they're having to unwatch the repository. That is a reasonable thing to do (I, for instance, do not watch the repository). However that can make it harder to get people to review PRs since they'll no longer be notified of news ones and they'll have to go out of their way to look for them.

One solution to this problem is https://github.com/facebook/mention-bot. It will attempt to intelligently notify people on new PRs (by CCing them) who it thinks is a good fit to review the code (effectively, which people were working around this area most recently). This bot also has a feature that might help with the module owner issue mentioned in #29, which it allows people to configure themselves to always get notified for certain PRs that touch certain files. For people who never want to be automatically notified, they can add themselves to the configuration blacklist and it will never select them as a potential reviewer. It has a number of other features (skipping if the person is a contributor, delaying the mention for X amount of time to let a PR get a review "naturally" before notifying people, etc) that attempt to reduce the amount of notifications it gives as well.

[brettcannon]

Since the bot has an open endpoint so we don't have to keep it running then I'm +1 for giving a try. I guess we would require someone be in the Python org in order to be mentioned? And would we want to blacklist any specific files?

[dstufft]

I would say yea you'd want them to be in the Python org.

We probably want to blacklist files that regularly get updated in PRs that are not related to the actual change. So Misc/NEWS, Misc/ACKS, etc. That will prevent someone from getting flagged as a possible reviewer just because they happened to have edited Misc/NEWS also. Not sure if there any other files like that but if there we probably want to blacklist them too.

I'm on the fence about delaying. On one hand I think it could reduce some noise if people are asking each other for reviews themselves in IRC or whatever, but on the other hand it's a nicer experience if someone is able to quickly review instead of having it happen X hours later.

[brettcannon]

We can try it without a delay and then scale one up if we find it too noisy.

[orsenthil]

Yeah, I am open on trying this. We can evaluate how helpful this is after we try.

[dstufft]

In python/cpython#280 a bit of a process foul up happened when a change to a security only branch got merged without the explicit approval of the RM for that branch. An additional feature that would be useful for the mentionbot is to see what branch a change is being merged into, and if it's a security only branch then either assign or request a review for the RM for that branch.

Looking at the docs for mentionbot, it doesn't currently support this, but presumably we could add it.

[warsaw]

You know I did that on purpose, to expose the glaring weaknesses in our process. It wasn't at all me being an idiot.

[dstufft]

Looking at the mentionbot PRs, it might actually already be workable as is. It appears that it pulls the configuration from the target branch. Which is somewhat helpful in that it allows people to control their settings on a per branch basis (on the flip side, settings must be controlled on a per branch basis).

But that means that on each security branch, we can set it up to always notify the relevant release manager (and probably only notify them, if all changes have to go through them).

[berkerpeksag]

alwaysNotifyForPaths, assignToReviewer and createReviewRequest settings of mentionbot would be really useful, but I'm not really fan of the default "here's a list of potential reviewers" comment.

[brettcannon]

I have yet to hear anyone say this is a bad idea, so do you want to draft up a PR for master @dstufft and we can give it a shot? I can add the webhook once the PR is ready.

[dstufft]

Alright @brettcannon , done in python/cpython#352.

• It doesn't delay notification.
• It ignores Misc/ACKS and Misc/NEWS assuming that those files are going to get touched a lot in ways not related to the change that needs reviewed.
  • This will of course need updated with the outcome of Generate Misc/NEWS from individual files #6.
• I left the place where people would add themselves to the file to have the mention bot ignore them and never mention them in a comment. This allows people to opt-out of participating by simply editing .mention-bot.
• If we get a bot that automatically creates backports, we could add it to userBlacklistForPR so it doesn't create additional review comments.
• I required that to be mentioned, someone should be in the python org. Ideally we could narrow that down even further by saying what team they need to be on. This is being tracked by Mention only users who are part of a team facebookarchive/mention-bot#92.

We should probably modify the release procedure for moving a release to a security only release to either remove this file, or modify it so that it says:

{
  "findPotentialReviewers": false,
  "createReviewRequest": true,
  "assignToReviewer": true,  // This is an alternative to createReviewRequest
  "alwaysNotifyForPaths": [
    {
      "name": "the-release-manager-for-this-branch",
      "files": ["**/*"]
    }
  ]
}

This should automatically either assign or create a review request (depending on which option we pick) the release manager for that branch on each PR.

In addition, when cutting a brand new release branch (but not a security release branch) we'd probably want a procedure to just delete this file. New PRs should be coming in on master after that branch is created anyways so the primary thing creating old PRs will be backports which should have already been reviewed.

[brettcannon]

OK, PR is merged and the webhook has been added. Now I guess we wait and see it work. 😄