PR reviewing process proposal

[adizere]

Context

This is a preliminary discussion towards fixing #2402.

I offer a perspective here that might be missing some parts. If we manage to agree on those missing parts and how to fix them, we can formalize everything in the CONTRIBUTING.md doc & close #2402.

Table of contents:

1. A naive description of how PR reviewing is done using reviewers only
2. Adding assignees to the picture
3. Takeaways

Main discussion

1. A naive description of how PR reviewing is done, using only reviewers

In the figure below I describe a naive view of the PR reviewing process.

The main problem with this description is that the reviewers (which are tagged automatically) might:

• be away
• have no knowledge of the changed code
• have no context on how to reproduce the original issue
• have other priorities.

When we have stale issue, the main reasons for that are:
a. we forgot to ask someone for explicit feedback, i.e., the PR needs further testing & reviewing, but it wasn't clear who should do it because there are multiple people tagged as reviewers
b. decisions needs to be made before the PR can progress, but neither the PR author nor reviewers feel like they can take those decisions.

Essentially, reviewers are a best-effort activity. It works in 90% of PRs, but not always. Reviewers are only in charge with correctness of the PR, not with taking decisions out of the scope of correctness, nor are they responsible for timely merging, for opening subsequent issues or anything else.

2. Adding assignees to the picture

Assignees are different from reviewers: a project maintainer manually tags the assignee, for instance. So an assigned person is not providing their best-effort, but rather a prioritized activity. The decision to assign someone will consequently shift people priorities, so it must take into account the context of the whole project's roadmap.

I propose that an assignee acts as a person responsible for all things needed to close the PR in a timely manner. This is a big responsibility that is a super-set of the responsibilities of both reviewer and PR author. Whenever a reviewer is unavaiable, the assignee will act as reviewer. If the PR author is away of has other priorities, the assignee will fix problems & apply review suggestions & handle the merging.

The assignee is a "safety net," and it involves a lot of responsibility. The project maintainers will tag assignees when necessary.

Takeaways

How many approvals are necessary to merge a PR?

• Default, 1.
• In special cases, we signal specifically in the PR (one example is Replace positional arguments with flags in CLI #2275)

What is the responsibility of reviewers?

• Correctness of the PR
  • Which include testing (manually if necessary) and reviewing the code.
• They can ask for fixes, including new test cases.
• They can optionally do fixes (instead of the PR author).
• They can optionally merge the PR (instead of the PR author).

What are the responsibilities of the PR author?

• Fix review suggestions and provide rationale for the technical choices they made in their PR.
• Merge the PR once they have at least 1 approving review.
• No need to tag assignees nor reviewers, though they are encouraged to do that.

What are the responsibilities of the assignee?

• Testing, reviewing, "cat herding" reviewers and the PR author.
• Opening subsequent issue to capture follow-up work.
• Take any decisions needed to timely merge the PR, including coordination with other teams, for example.
• Fix problems in the PR (instead of the PR author), if necessary.
• Merging the PR.

[seanchen1991]

Transcribing the takeaways from today's weekly meeting here:

• AZ: Important to tell explicitly the assigned person. Otherwise it gets lost in emails, buried.
  • PR owner should be first “line of offense” to tag reviewer or assigned.
• HS: Consider Slack integration with GitHub. Can get lost, must be personalized per person (DM).
• LJ: In any case, it should be documented. Not obvious it’s ok to DM people or assign them, explicit documentation.
  • AZ: Part of onboarding.
• SEC: Rust project. Bot randomly tags someone. Either “by luck” they are good, or they tag someone themselves.
• AZ proposal: rid of CODEOWNER. Any PR author tries to find someone, otherwise they tag someone. Project maintainers will be responsible.
• SA: It’s important to clarify the “responsibilities” (championing, get it merged, fix & solutions) of whoever is tagged reviewers.
  • Document this process.

[adizere]

Resolved in #2438.

In particular:

https://github.com/informalsystems/ibc-rs/blob/f534cbd506f10fcc379bb161ecbffc88dadd3968/CONTRIBUTING.md?plain=1#L178-L215