Guidelines for PR Merging

[liamfallon]

Should we have some guidelines for PR merging. Here's a few (some lifted from ONAP):

1. Code reviews should be preformed within 36 business hours* after the PR has been raised
2. Self merging of code is not allowed (Enforced by Prow ;-))
3. Each PR should have at least one review from a reviewer from a company other than the submitter's company
4. Anyone can provide a code review on any PR
5. A PR with outstanding comments cannot be merged
6. A PR must have reviews from x% of reviewers in the OWNERS file before it can be merged

*I realise this is ambiguous, but the intent is to ensure everyone has one working day to review a PR no matter what time zone they are in.

[vjayaramrh]

@liamfallon Good initiation of the discussion related to PR merging.

• REgarding bullet 1, Can you expand on what you believe should be done related to PR review within 36 hours.? I want to confirm that you are not expecting it to be merged within 36 hours, right?
• Regarding bullet 2, Totally, agree with not allowing self-merging of code.
• Regarding bullet 4, Totally agree that anyone should provide a code review comments, that's how it was in OpenStack. In my opinion, it maybe a good idea to make it a requirement to have atleast 2 reviewers that do not have merge rights to conduct PR review, before it gets merged. THis would spread the knowledge across the community and encourage wider participation.
• Regarding bullet 5, Can prow be configured to disallow PR merge if there are outstanding comments?

[liamfallon]

On the 36 (working) hours, this would be the minimum amount of time a PR should be available for review. This is to give everyone the opportunity to have a say on the review. if a PR is up for 36 hours and a person hasn't commented, then it is OK to assume that the person has no issue with the PR being merged.

I guess the point with this one is to avoid the situation where a PR is raised and merged by contributors in one time zone before contributors in another time zone have an opportunity to see it.

Also there are cases where "emergency" PRs must be merged (eg for incorrect configuration) but the spirit of this one is intended for reviews that change/add/delete functionality rather than bug fixes/config changes.

[liamfallon]

Closing this discussion as it has petered out.