Integrate r? with the new GitHub request reivew feature

[nrc]

Integrate r? with the new GitHub request reivew feature

[davidalber]

Are you wanting this in lieu of setting an assignee or in addition to it?

[nrc]

Good question. Probably as well as to start with, then maybe we can repurpose assignee if people are comfortable using the reviewer (I think we would need to change Bors as well, and possibly other code).

[davidalber]

I've investigated the API side of setting a reviewer. First, the current way that assignee is being set in Highfive is deprecated (Highfive uses assignee in the edit issue request), so what I am writing probably applies to the way that Highfive should be changed for modifying assignees (if we keep that feature in the long run).

• Reviewers are set through a create review request. Multiple reviewers and teams can be added at once.
• Calling that endpoint does not clear preexisting reviewers. This is the part that is different. The approach Highfive currently uses clears preexisting assignees.
• Reviewers can be removed by using the delete review request. That also takes a list.

The consequence is that, if we want to use reviewers, we need to decide what Highfive does with existing reviewers. For example, when someone does "r? @davidalber", I would expect any reviewers previously assigned by Highfive to be removed. However, it's possible that other reviewers were added manually and perhaps we don't want to remove them. Options:

• Highfive removes all reviewers before it adds a reviewer. I believe this is the same as current semantics.
• Highfive removes reviewers it previously added before adding new reviewers.
• Highfive doesn't remove any reviewers. This doesn't really seem like an option.

Ignoring implementation complexity, I think Option 2 is the best solution. The simplest acceptable approach is Option 1, so maybe we should start there.

[nrc]

Hmm, I'm very keen to avoid adding any state at all to highfive. It is a joy to work with compared to some other bots I've had because of the lack of state. I believe knowing which reviewers highfive added would require either state (unless we can get a history of reviewer changes including who made the change from the API? If that is possible then it seems fine).

Option 1 seems a fine way to start. We might to a hybrid where we remove reviewers highfive is summouned by a user comment, but we leave a reviewer in place if it was set when the PR was submitted (i.e., highfive was summoned on a new PR rather than a comment).

[davidalber]

I'm with you regarding state. It's kind of a big line to cross.

GitHub has an endpoint to retrieve events for an issue. That shows events for reviewers added and by whom. For davidalber/highfive-test-repo#1, for example, see https://api.github.com/repos/davidalber/highfive-test-repo/issues/1/events. Here's the first event from that, which shows a review request (the most interesting stuff is under review_requester and requested_reviewer):

{
   "id":1517745939,
   "url":"https://api.github.com/repos/davidalber/highfive-test-repo/issues/events/1517745939",
   "actor":{
      "login":"davidalber",
      "id":933552,
      "avatar_url":"https://avatars3.githubusercontent.com/u/933552?v=4",
      "gravatar_id":"",
      "url":"https://api.github.com/users/davidalber",
      "html_url":"https://github.com/davidalber",
      "followers_url":"https://api.github.com/users/davidalber/followers",
      "following_url":"https://api.github.com/users/davidalber/following{/other_user}",
      "gists_url":"https://api.github.com/users/davidalber/gists{/gist_id}",
      "starred_url":"https://api.github.com/users/davidalber/starred{/owner}{/repo}",
      "subscriptions_url":"https://api.github.com/users/davidalber/subscriptions",
      "organizations_url":"https://api.github.com/users/davidalber/orgs",
      "repos_url":"https://api.github.com/users/davidalber/repos",
      "events_url":"https://api.github.com/users/davidalber/events{/privacy}",
      "received_events_url":"https://api.github.com/users/davidalber/received_events",
      "type":"User",
      "site_admin":false
   },
   "event":"review_requested",
   "commit_id":null,
   "commit_url":null,
   "created_at":"2018-03-13T04:24:00Z",
   "review_requester":{
      "login":"davidalber",
      "id":933552,
      "avatar_url":"https://avatars3.githubusercontent.com/u/933552?v=4",
      "gravatar_id":"",
      "url":"https://api.github.com/users/davidalber",
      "html_url":"https://github.com/davidalber",
      "followers_url":"https://api.github.com/users/davidalber/followers",
      "following_url":"https://api.github.com/users/davidalber/following{/other_user}",
      "gists_url":"https://api.github.com/users/davidalber/gists{/gist_id}",
      "starred_url":"https://api.github.com/users/davidalber/starred{/owner}{/repo}",
      "subscriptions_url":"https://api.github.com/users/davidalber/subscriptions",
      "organizations_url":"https://api.github.com/users/davidalber/orgs",
      "repos_url":"https://api.github.com/users/davidalber/repos",
      "events_url":"https://api.github.com/users/davidalber/events{/privacy}",
      "received_events_url":"https://api.github.com/users/davidalber/received_events",
      "type":"User",
      "site_admin":false
   },
   "requested_reviewer":{
      "login":"TapscottLab",
      "id":22222483,
      "avatar_url":"https://avatars2.githubusercontent.com/u/22222483?v=4",
      "gravatar_id":"",
      "url":"https://api.github.com/users/TapscottLab",
      "html_url":"https://github.com/TapscottLab",
      "followers_url":"https://api.github.com/users/TapscottLab/followers",
      "following_url":"https://api.github.com/users/TapscottLab/following{/other_user}",
      "gists_url":"https://api.github.com/users/TapscottLab/gists{/gist_id}",
      "starred_url":"https://api.github.com/users/TapscottLab/starred{/owner}{/repo}",
      "subscriptions_url":"https://api.github.com/users/TapscottLab/subscriptions",
      "organizations_url":"https://api.github.com/users/TapscottLab/orgs",
      "repos_url":"https://api.github.com/users/TapscottLab/repos",
      "events_url":"https://api.github.com/users/TapscottLab/events{/privacy}",
      "received_events_url":"https://api.github.com/users/TapscottLab/received_events",
      "type":"User",
      "site_admin":false
   }
}

We'd have to worry about pagination, but that's less trouble to get right than making sure the local copy of added reviewers is consistent with the remote copy.

For now, I'll go with Option 1. We can refine it later if it's desirable.