Skip to content

Commit

Permalink
docs: Update PR review process (#18233)
Browse files Browse the repository at this point in the history
* docs: Update PR review process

* Update docs/src/maintain/review-pull-requests.md

Co-authored-by: Amaresh  S M  <amareshsm13@gmail.com>

* Update docs/src/maintain/review-pull-requests.md

Co-authored-by: Amaresh  S M  <amareshsm13@gmail.com>

* Update docs/src/maintain/review-pull-requests.md

Co-authored-by: Amaresh  S M  <amareshsm13@gmail.com>

* Clarify what to do for PRs with open issues

---------

Co-authored-by: Amaresh  S M <amareshsm13@gmail.com>
  • Loading branch information
nzakas and amareshsm committed Mar 30, 2024
1 parent b07d427 commit 7747097
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 14 deletions.
2 changes: 2 additions & 0 deletions docs/src/maintain/manage-issues.md
Expand Up @@ -43,6 +43,8 @@ All of ESLint's issues and pull requests, across all GitHub repositories, are ma
* **Blocked**: The issue can't move forward due to some dependency
* **Ready to Implement**: These issues have all the details necessary to start implementation
* **Implementing**: There is an open pull request for each of these issues or this is a pull request that has been approved
* **Second Review Needed**: Pull requests that already have one approval and the approver is requesting a second review before merging.
* **Merge Candidates**: Pull requests that already have at least one approval and at least one approver believes the pull request is ready to merge into the next release but would still like a TSC member to verify.
* **Completed**: The issue has been closed (either via pull request merge or by the team manually closing the issue)

We make every attempt to automate movement between as many columns as we can, but sometimes moving issues needs to be done manually.
Expand Down
53 changes: 39 additions & 14 deletions docs/src/maintain/review-pull-requests.md
Expand Up @@ -26,7 +26,7 @@ The bot will add a comment specifying the problems that it finds. You do not nee

Once the bot checks have been satisfied, you check the following:

1. Double-check that the commit message tag ("Fix:", "New:", etc.) is correct based on the issue (or, if no issue is referenced, based on the stated problem).
1. Double-check that the pull request title is correct based on the issue (or, if no issue is referenced, based on the stated problem).
1. If the pull request makes a change to core, ensure that an issue exists and the pull request references the issue in the commit message.
1. Does the code follow our conventions (including header comments, JSDoc comments, etc.)? If not, please leave that feedback and reference the [Code Conventions](../contribute/code-conventions) documentation.
1. For code changes:
Expand All @@ -37,28 +37,55 @@ Once the bot checks have been satisfied, you check the following:

**Note:** If you are a team member and you've left a comment on the pull request, please follow up to verify that your comments have been addressed.

## Who Can Merge a Pull Request
## Required Approvals for Pull Requests

TSC members, Reviewers, Committers, and Website Team Members may merge pull requests, depending on the contents of the pull request.
Any committer, reviewer, or TSC member may approve a pull request, but the approvals required for merging differ based on the type of pull request.

Website Team Members may merge a pull request in the `eslint.org` repository if it is:
One committer approval is required to merge a non-breaking change that is:

1. A documentation change
1. A bug fix (for either rules or core)
1. A dependency upgrade
1. Related to the build
1. A chore

Committers may merge a pull request if it is a non-breaking change and is:
For a non-breaking feature, pull requests require approval from one reviewer or TSC member, plus one additional approval from any other team member.

For a breaking change, pull requests require an approval from two TSC members.

::: important
If you approve a pull request and don't merge it, please leave a comment explaining why you didn't merge it. You might say something like, "LGTM. Waiting three days before merging." or "LGTM. Requires TSC member approval before merging" or "LGTM. Would like another review before merging."
:::

## Moving a Pull Request Through the Triage Board

When a pull request is created, whether by a team member or an outside contributor, it is placed in the "Needs Triage" column of the Triage board automatically. The pull request should remain in that column until a team member begins reviewing it.

If the pull request does not have a related issue, then it should be moved through the normal [triage process for issues](./manage-issues) to be marked as accepted. Once accepted, move the pull request to the "Implementing" column.

If the pull request does have a related issue, then:

* If the issue is accepted, move the pull request to the "Implementing" column.
* If the issue is not accepted, move the pull request to the "Evaluating" column until the issue is marked as accepted, at which point move the pull request to "Implementing".

Once the pull request has one approval, one of three things can happen:

1. The pull request has the required approvals and the waiting period (see below) has passed so it can be merged.
1. The pull request has the required approvals and the waiting period has not passed, so it should be moved to the "Merge Candidates" column.
1. The pull request requires another approval before it can be merged, so it should be moved to the "Second Review Needed" column.

When the pull request has a second approval, it should either be merged (if 100% ready) or moved to the "Merge Candidates" column if there are any outstanding concerns that should be reviewed before the next release.

## Who Can Merge a Pull Request

TSC members, reviewers, committers, and website team members may merge pull requests, depending on the contents of the pull request, once it has received the required approvals.

Website Team Members may merge a pull request in the `eslint.org` repository if it is:

1. A documentation change
1. A bug fix (for either rules or core)
1. A dependency upgrade
1. Related to the build tool
1. A chore

In addition, committers may merge any non-breaking pull request if it has been approved by at least one TSC member.

TSC members may merge all pull requests, including those that committers may merge.

## When to Merge a Pull Request

We use the "Merge" button to merge requests into the repository. Before merging a pull request, verify that:
Expand All @@ -83,9 +110,7 @@ Otherwise, team members should observe a waiting period before merging a pull re

The waiting period ensures that other team members have a chance to review the pull request before it is merged.

If the pull request was created from a branch on the `eslint/eslint` repository (as opposed to a fork), delete the branch after merging the pull request. (GitHub will display a "Delete branch" button after the pull request is merged.)

**Note:** You should not merge your own pull request unless you're received feedback from at least one other team member.
**Note:** You should not merge your pull request unless you receive the required approvals.

## When to Close a Pull Request

Expand Down

0 comments on commit 7747097

Please sign in to comment.