OpenSearchDashboards Code Review Process Proposal

[kristenTian]

Having a few ideas around code review process proposal would like to discuss with the team.

Review on daily basis

As the owner of the OSD repository, it should be our daily base duty to review PR submitted to the repo. An SLA should be set to improve the community’s trust. Instead of delegate this to Oncall (Single person).

Have project maintainers

As a general practice, there is a point of contact(POC) reviewer for each service package, we suggest having project maintainers (also mentioned in this proposal). As the repository and its use cases grow, it is not practical to assume all maintainers have the same context around everything. Therefore, maintainers should be assigned to each area based on their familiarities. While volunteering and learn and be curious are always recommended, having dedicated maintainers for each area can improve efficiency.

Follow through the PR

For each PR, the reviewer should try to follow through with the entire review process until either an approval or rejection is made. This helps to save the team's time overall by avoiding the need for others to redo the ramp-up process as the context accumulates.

[ananzh]

Agree. Would like to add up to @kristenTian's proposal the followings:

• On-call is responsible for assigning PR review to maintainers. Maintainer should provide a deadline of finishing the review.
• On-call is responsible for follow-up. Check long lasting PRs and take over the hand-off follow-up PRs.
• Propose to make regular (weekly) PR review swarm for maintainers. Swarm should focus on:
  • PRs with no clear maintainer specialty, like monaco one.
  • PRs lack of active responses: Contributors don't reply and PRs need to be decided to either close or take over.
  • PRs that are important to share with team. (knowledge share)
  • PRs that have review questions.

[kristenTian]

Had an discussion with @joshuarrrr, having the following add ups:

For "Review on daily basis", Proposing a 2-day business day SLA on each engagement of the PR.

For "Have project maintainers",

• If PR is project contribution, the POCs should be rather clear to find. Besides project's peer approval will be required;
• For other type of PRs, we can leverage the PR swarm meeting to gradually build the domain and expertise POCs mapping, and come up with a mechanism to move forward when there are no POCs.

[AMoo-Miki]

While I understand the immediate benefits of making those more familiar with an area, the PoC for PRs, I cannot convince myself that this will promote expansion of individual expertise. While we have not officially done so, we have kinda been doing what is being proposed here and I don't think we have expanded our expertise much. Take me as an example: after years on the project, I still deffer to others when it comes to visualizations despite always throwing myself into situations which will help me learn new things. With rotating on-calls, one would be "encouraged" to learn more, quicker!

Also is a 2-work-days SLA for first engagement on PRs realistic, considering the number of hours maintainers will have remaining for actual coding?

[ashwin-pc]

I agree with Miki here. While this proposal is in the right direction, it is a little too idealistic. From the PR swarm that we had 2 days ago, here is the proposed process:

A few other callouts from the swarm:

1. If the remaining changes are only nits and the contributor does not respond (within 2 weeks), we make the changes ourselves and just merge the PR.
2. If we mark an external contributors PR as draft because they arent responding, we add a needs help label so that others can pick it up.
3. Anyone can be a second reviewer, but you cant be a rubber stamp, although you can start at a higher trust level

[zhongnansu]

@ashwin-pc On the left side of the diagram. If PRs are from OSD team, we assign the PR to the author. Could you elaborate on this part, that how an author from OSD team should drive his/her PR to get response/approval in time?

[ananzh]

I am seeing this graph from another repo https://github.com/DefinitelyTyped/DefinitelyTyped, which is helpful and could be used as a reference

[ashwin-pc]

@ashwin-pc On the left side of the diagram. If PRs are from OSD team, we assign the PR to the author. Could you elaborate on this part, that how an author from OSD team should drive his/her PR to get response/approval in time?

They can reach out directly to the OSD maintainers who they think are best suited to review the PR. That being said, you are right, this model calls for a maintainer to always be assigned to a PR, which might not always be the case. We can expand that section to mean "If the PR originates from the OSD team, that person is the primary asignee for the PR, but should assign another maintainer to review the PR". That way, the responsibility to get the PR reviewed is on the maintainer, but the responsibility to see the PR to completion is on the PR owner.