|
| 1 | +# Pull request process |
| 2 | + |
| 3 | +The pull request (PR) process reflects the team culture on collaborating within |
| 4 | +the open source project. At LoopBack, we strive to foster a supportive, |
| 5 | +collaborative, and constructive environment for everybody to enjoy working |
| 6 | +together with the same passion to create great values for our community. Both |
| 7 | +community and code matter for us. |
| 8 | + |
| 9 | +Our maintainers are very distributed around the world with different time zones |
| 10 | +and work schedules. Some of us are paid full-time IBM employees while others are |
| 11 | +voluntary community contributors. Most of our changes are officially accepted |
| 12 | +into the code base via pull requests. |
| 13 | + |
| 14 | +To facilitate collaborations, this document establishes a protocol on reviewing, |
| 15 | +approving, and landing a pull request for [loopback-next](https://github.com/strongloop/loopback-next). |
| 16 | +It has the following objectives: |
| 17 | + |
| 18 | +1. Defines a process that PR author and reviewers can follow |
| 19 | +2. Set the expectations among our maintainers who are distributed |
| 20 | +3. Strike a good balance between the time we can land a PR and the quality of |
| 21 | + the proposed change |
| 22 | + |
| 23 | +For general knowledge about github PR process, see: |
| 24 | + |
| 25 | +https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/proposing-changes-to-your-work-with-pull-requests |
| 26 | + |
| 27 | +We also have a tutorial to cover how to submit a PR to `loopback-next`: |
| 28 | + |
| 29 | +https://loopback.io/doc/en/lb4/submitting_a_pr.html |
| 30 | + |
| 31 | +## The PR process |
| 32 | + |
| 33 | +### Submit a PR |
| 34 | + |
| 35 | +Provide some context |
| 36 | + |
| 37 | +- Pointing to an existing issue |
| 38 | +- Summarizing the problem/solution |
| 39 | +- Linking to document changes/diagrams |
| 40 | + |
| 41 | +Check the items from the PR template |
| 42 | + |
| 43 | +Set the right labels |
| 44 | + |
| 45 | +Consider to break complex PRs into a few smaller ones |
| 46 | + |
| 47 | +### Assign a primary owner and nominate additional reviewers |
| 48 | + |
| 49 | +When a PR is submitted, github automatically suggest some of reviewers based on |
| 50 | +(CODEOWNERS)[https://github.com/strongloop/loopback-next/blob/master/CODEOWNERS] |
| 51 | +and those who have worked on the code recently. |
| 52 | + |
| 53 | +### Review a PR |
| 54 | + |
| 55 | +The reviewers should make it explicit for different categories of feedbacks, such as: |
| 56 | + |
| 57 | +- Must have/fix (the author must address such comments) |
| 58 | +- Nice to have/fix (the author should try to address such comments but they don't |
| 59 | + block the PR from being landed) |
| 60 | +- Nitpick (up to the author to decide) |
| 61 | + |
| 62 | +For `Must have/fix` Request changes |
| 63 | + |
| 64 | +Ping peers if the process is stalled. |
| 65 | + |
| 66 | +- https://google.github.io/eng-practices/review/reviewer/ |
| 67 | +- https://blog.pragmaticengineer.com/good-code-reviews-better-code-reviews/ |
| 68 | +- https://medium.com/palantir/code-review-best-practices-19e02780015f |
| 69 | + |
| 70 | +Iterative commits |
| 71 | + |
| 72 | +To address review comments, it's recommended to keep pushing new commits as the |
| 73 | +PR will automatically pick up new changes pushed to the same source branch. |
| 74 | + |
| 75 | +Such commits should be squashed and reorganized to maintain a clean history. |
| 76 | + |
| 77 | +### Approve a PR |
| 78 | + |
| 79 | +### Veto a PR |
| 80 | + |
| 81 | +### Land a PR |
| 82 | + |
| 83 | +Minimum requirements: |
| 84 | + |
| 85 | +- CI is green (with exceptions that the failure is caused by known issues) |
| 86 | +- Approved by the owner |
| 87 | +- Approved by mandatory reviewers |
| 88 | + |
| 89 | +If the author of a PR is a maintainer, he/she will be responsible for landing |
| 90 | +the PR upon approvals. Otherwise, the owner of the PR should land it on behalf |
| 91 | +of the community contributor. |
| 92 | + |
| 93 | +Other maintainers can land the PR for the author too to avoid further delays |
| 94 | +in case of preparing a release or fixing build breaks. |
| 95 | + |
| 96 | +Depending on the nature of a PR, we'll decide the minimum duration that we keep |
| 97 | +it open even if the minimum criteria have been met so that other maintainers will |
| 98 | +have a chance to review the changes. |
| 99 | + |
| 100 | +- Trivial changes, such as fixing typos, code formatting issues, eslint violations |
| 101 | + |
| 102 | +### Follow up a PR |
| 103 | + |
| 104 | +- Additional reviews/comments |
| 105 | +- Create follow-up stories |
| 106 | +- Submit follow-up PRs |
| 107 | +- Revert a PR |
0 commit comments