Expand and clarify PR process

[cjohnhanson]

We should update the merge process docs:

• [x] "Merge requests" and any other relevant language should be updated to use GitHub terms (i.e., "Pull Requests")
• [ ] We should distinguish between Reviewers whose reviews are optional but requested and Assignees whose reviews are required for the PR to merge.
• [ ] Requested reviewers who begin a review should add themselves as an Assignee to indicate to the author that their review is in progress and the branch shouldn't be merged until the review has completed and requested changes have been addressed.

CC: @DouweM

[DouweM]

This is in part to address the fact that I was a requested reviewer on https://github.com/meltano/meltano/pull/6112 and in the middle of my review with a bunch of staged comments when I saw it getting merged out from under me because someone else had already approved. GitHub really should have a "User is in the process of reviewing, are you sure you want to merge without seeing their comments?" warning.

[aaronsteers]

@cjohnhanson - I'm agreed on the first point for sure. However, I would prefer to keep assigners as the authors, just for tracking simplicity. What if we adapt this to

• A team mention in reviewers is an invitation/fyi, but not a requirement.
• A direct mention as reviewer is a (effectively) an assignment. If you cannot review, please remove your name from reviewers and add a comment.
• If you do want to review, add your name explicitly.

Something like this? ☝️

Also - we are still wrestling a bit with codeowners. I want to have redundancy in codeowners - without spamming everyone with requests. But the way codeowners works in GitHub, we must auto-add everyone who is on the list of codeowners.

We're actually considering removing.

Additional background:

• I am considering a conflicting policy of only one assignee allowed per item at a time, at least for engineering work.

[aaronsteers]

I think each work item (issues and PRs) should have a single owner and assignee.

But to the point of reviewers:

• I think we should definitely add ourselves if we want to be noted as a reviewer.
• Immediately drop a comment on the issue if you want the reviewer to hold for your feedback.

[aaronsteers]

Another topic here is related to what necessary quorom is needed of approvers, and how do authors correctly identify who needs to review and who they can invite to optionally review.

There's always a risk that (unless coordinated otherwise) if the quorum of approvals is met, it is possible the PR will merge while an optional reviewer is still in the process of reviewing.

[DouweM]

@aaronsteers It's fine that I wasn't a required reviewer on https://github.com/meltano/meltano/pull/6112, but once I got there and started reviewing I thought "I'm glad I got here before this got merged", and then it got merged anyway because there was no way for Cody or anyone else to know that I had pending comments. It's a problem in the GitHub product more than anything, but what would you recommend I'd have done in this case to make sure my review was submitted and taken into account before the merge button was hit?

Assigning myself when I start reviewing is the approach Cody and I quickly came up with, but I can see how that's not sufficient.

[aaronsteers]

@DouweM - Agreed we're wrestling with some less than ideal behaviors on the GitHub UX side.

I ran into this on another PR a couple days ago and I dealt with this by immediately sent a 'Request Changes' (which was admittedly a misnomer) with just a "hey, please wait while I catch up and review the rest of this". I didn't actually have changes to request but I did want the PR to hold until I had more time to review it and think about it.

Two things I think are especially painful on the GitHub UX side:

1. Once you have a review in progress, it's difficult or impossible to just add a comment.
2. As you called out, @DouweM, there's nothing from the author's side that tells them "FYI, Douwe has 14 comments drafted and queued up to send - probably you should wait until those are submitted". 😮

[DouweM]

@aaronsteers I wonder what GitHub's take on this is. One of their DevRel people (Martin Woodward) is in our Slack and has said we can reach out with any questions, so we should! Wanna draft a quick explanation/question for him that I can send, or you can yourself? Doesn't need to be today, of course.

[cjohnhanson]

@aaronsteers @DouweM honestly a low-tech solution here would be to just have some standards around an emoji react on the PR description. I.e 👀 = I'm currently reviewing, don't merge.

[aaronsteers]

We had a very similar thing happen in Gitlab and my proposal was almost the same! (But then we didn't have the Propose Changes UI at all.)

I don't think 👀 is strong enough because I use this all the time for "I saw it" or "ACK".

We could use 😕 or we could just get in the habit of submitting a comment or a blocking review with the Request Changes UI that says "I need more time." or "I want to talk more about this. Don't merge yet."

I'm all for low tech solutions and building a shared communication language tuned for async challenges.

[aaronsteers]

From the GitHub side, I want them to notify authors if a reviewer had a pending review already started. If there isn't a feature request on this we should request it.

[DouweM]

@aaronsteers I shared the feature request with Martin Woodward, he said:

That is stunningly good feedback. A bit like the ‘Martin is typing…’ in Slack

Needs to be some timeout stuff in case people start and don’t finish etc, but yeah

At the moment, the best was is to go in quick with a ‘I have thoughts’ type comment and make as request changes and then go back and do a proper review, but that sucks. I’ll ping the PR team now and chat with them

So this may actually get solved 😄