Skip to content
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 107 additions & 0 deletions PR-PROCESS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
# Pull request process

The pull request (PR) process reflects the team culture on collaborating within
the open source project. At LoopBack, we strive to foster a supportive,
collaborative, and constructive environment for everybody to enjoy working
together with the same passion to create great values for our community. Both
community and code matter for us.

Our maintainers are very distributed around the world with different time zones
and work schedules. Some of us are paid full-time IBM employees while others are
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's anyone being paid fulltime to manage LoopBack anymore.

Suggested change
and work schedules. Some of us are paid full-time IBM employees while others are
and work schedules. Most of us are

voluntary community contributors. Most of our changes are officially accepted
into the code base via pull requests.

To facilitate collaborations, this document establishes a protocol on reviewing,
approving, and landing a pull request for [loopback-next](https://github.com/strongloop/loopback-next).
It has the following objectives:

1. Defines a process that PR author and reviewers can follow
2. Set the expectations among our maintainers who are distributed
3. Strike a good balance between the time we can land a PR and the quality of
the proposed change

For general knowledge about github PR process, see:

https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/proposing-changes-to-your-work-with-pull-requests

We also have a tutorial to cover how to submit a PR to `loopback-next`:

https://loopback.io/doc/en/lb4/submitting_a_pr.html

## The PR process

### Submit a PR

Provide some context

- Pointing to an existing issue
- Summarizing the problem/solution
- Linking to document changes/diagrams

Check the items from the PR template

Set the right labels

Consider to break complex PRs into a few smaller ones

### Assign a primary owner and nominate additional reviewers

When a PR is submitted, github automatically suggest some of reviewers based on
(CODEOWNERS)[https://github.com/strongloop/loopback-next/blob/master/CODEOWNERS]
and those who have worked on the code recently.

### Review a PR

The reviewers should make it explicit for different categories of feedbacks, such as:

- Must have/fix (the author must address such comments)
- Nice to have/fix (the author should try to address such comments but they don't
block the PR from being landed)
- Nitpick (up to the author to decide)
Comment on lines +57 to +60
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider adopting Conventional Comments:

  • Must have/fix: suggestion(blocking): <message>
  • Nice to have/fix: suggestion(non-blocking): <message>
  • Nitpick: nitpick: <message>


For `Must have/fix` Request changes

Ping peers if the process is stalled.

- https://google.github.io/eng-practices/review/reviewer/
- https://blog.pragmaticengineer.com/good-code-reviews-better-code-reviews/
- https://medium.com/palantir/code-review-best-practices-19e02780015f

Iterative commits

To address review comments, it's recommended to keep pushing new commits as the
PR will automatically pick up new changes pushed to the same source branch.

Such commits should be squashed and reorganized to maintain a clean history.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we mention a stance against merge commits?

Suggested change
Merge commits
When iterating on a PR, sometimes it may be necessary to pull in the latest changes from upstream to fix potential merge conflicts. A common approach is to use merge commits. However, LoopBack will not accept Merge Commits as they complicate the Git History and make reviewing more difficult. Instead, `git rebase` should be used to apply the PR commits on top of the latest commit from upstream.

### Approve a PR

### Veto a PR

### Land a PR

Minimum requirements:

- CI is green (with exceptions that the failure is caused by known issues)
- Approved by the owner
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should give the TSC the ability to act on behalf?

Suggested change
- Approved by the owner
- Approved by the owner or a TSC member

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking whether it should be TSC member or maintainer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that there's no reason why a maintainer can't approve it. Though I wonder what separates them from the code owner? One thing that comes into mind is being pinged for the PR.

With that consideration, should we formalise a process for deciding who becomes a code owner? This is probably out of scope of this PR, but it may be for future consideration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah. i see. I misunderstood that the "owner" is the PR author, instead of the code owner. I thought code owner is the maintainers of the repo ?

- Approved by mandatory reviewers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add also recommended requirements:

  • Code coverage is not decreased. Exceptions are allowed after reviewing which lines of code are not covered by tests.

If the author of a PR is a maintainer, he/she will be responsible for landing
the PR upon approvals. Otherwise, the owner of the PR should land it on behalf
of the community contributor.

Other maintainers can land the PR for the author too to avoid further delays
in case of preparing a release or fixing build breaks.

Depending on the nature of a PR, we'll decide the minimum duration that we keep
it open even if the minimum criteria have been met so that other maintainers will
have a chance to review the changes.

- Trivial changes, such as fixing typos, code formatting issues, eslint violations

### Follow up a PR

- Additional reviews/comments
- Create follow-up stories
- Submit follow-up PRs
- Revert a PR