GitHub PR process

[webbnh]

In GChat, Peter posted a link to a rant about the GitHub PR process whose ills are supposedly addressed by using Gerrit. I stopped reading it when the topic switched from GitHub to Gerrit, because we, as a team, have been unable to commit (er...) to enduring the pain required to switch to and use Gerrit. I do not intend to denigrate Gerrit in any way, but GitHub is the devil that we have, and I think that, with attendance to a few small details, we can make it be a perfectly usable tool. Toward that end, I present the following suggestions for discussion.

1. Once a PR is put up for review, every subsequent update to it should be made in a new commit. GitHub offers reasonable support for reviewing individual commits or commit-ranges, which eases the pain of re-reviews considerably iff the changes are made in separate commits. We should desperately avoid amending existing commits during a review.
2. When a PR receives approval, then the branch can be squashed as appropriate into a final form and recirculated for final approval. I believe that, if the reviewers previously marked all the modules as "viewed", then GitHub will show the post-squash PR as "no change" since the previous review, which makes granting final approval all but trivial.

Also, GitHub tracks the "conversations" around review comments. (It doesn't always present them well, but it's getting better.) These are all accessible (albeit, sometimes with a little work) via the main PR page (under the "Conversation" tab), even ones for commits which are no longer accessible because they have been amended/rebased/superseded. (There is now also a list of conversations, with the resolved ones filtered out by default, accessible via a drop-down in the "Files changed" tab, although trying to access them directly from there can be challenging.)

3. We need to come to some consensus as a team as to how these conversations get marked as resolved. (Please discuss this in Discussion GitHub: When to mark a PR "conversation" as "resolved"? #2113.)
4. We should require that they all be resolved before merging.

Discuss!

[riya-17]

One thing that I observed while playing around with conversations is that it only works with loaded files. The files which are not rendered due to being large and have an unresolved conversation present it, the 'conversations' won't lead us to that. but yeah each conversation also has files mentioned with it, So, that is cool.

[webbnh]

Yes, clicking on a conversation in the list apparently only takes you to that conversation if it is loaded in the current files view. So, if the conversation is attached to a large diff that wasn't loaded or if it is attached to a line which is not present because it is in a commit which was not selected or which was amended, then clicking on it does nothing. However, being able to see the list serves as confirmation that the conversations should be findable in the display in the "Conversations" tab.

[portante]

I am still of the opinion that we should move to Gerrit based on the way the team "likes" to do code reviews. I still don't think GitHub's method of tracking code reviews makes sense for the behaviors the team wants.

[webbnh]

I am still of the opinion that we should move to Gerrit based on the way the team "likes" to do code reviews. I still don't think GitHub's method of tracking code reviews makes sense for the behaviors the team wants.

That's fine, but it's not germane to this discussion. (This discussion is about how to make GitHub work and workable for the team.)

If you want to discuss switching to Gerrit, perhaps it would be good to open a new Discussion for that. (Even without considering the merits of the switch, we need to lay out the requirements and mechanism for the switch -- they are non-trivial, and they require a substantial commitment from the team just to get to the point where we could do our first review -- so if you want to discuss that, please don't tube this discussion with those topics.)