| # How Pull Request Reviews Work in the Matter SDK |
| |
| ## Who reviews PRs? |
| |
| In general, **all reviews are welcome and encouraged**. The decision to merge a |
| pull request will take into account all feedback from reviews, including |
| comments and approvals. |
| |
| The weight of an approval towards merging a PR depends on the reviewer's GitHub |
| handle: |
| |
| - Anyone can approve a PR. If the approver is not a member of the |
| `project-chip` GitHub organization, their approval checkmark will be gray. |
| - If the approver is part of the `project-chip` organization, the checkmark |
| will be green; however, this does not inherently carry additional weight. |
| - If the approver is part of one of the `reviewers-<company>` groups in |
| [project-chip teams](https://github.com/orgs/project-chip/teams?query=reviewers-), |
| their approval generally counts as a "company approval" (see "When do PRs |
| merge" below). |
| - A separate group, |
| [matter-sdk-maintainers](https://github.com/orgs/project-chip/teams/matter-sdk-maintainers), |
| exists, and its members are specifically empowered to merge PRs that meet |
| the SDK quality bar. |
| |
| While PRs are generally reviewed, contributions to the SDK are mostly made by |
| CSA members. We encourage you to ask for reviews in the appropriate CSA Slack |
| channels once a PR is ready. |
| |
| To facilitate reviews, remember that your own review time is valuable. |
| Performing a quality review takes time. You are encouraged to help with the |
| review load, which will encourage others to review your PRs. |
| |
| ## When do PRs merge? |
| |
| - Pull requests must meet the |
| [pull request guidelines](pull_request_guidelines.md). Reviewers will |
| enforce this. |
| - Pull requests must pass CI. A PR should have a green CI status before it is |
| reviewed. |
| - We use several linters to catch common errors. Make sure to fix any |
| linter errors. |
| - Pull requests in `draft` status may not be reviewed, though some informal |
| reviews may still occur. |
| - Merge blocks are always respected: |
| - All comments must be resolved before a PR can be merged. |
| - A `Changes requested` status on a PR is always respected and must be |
| removed before a PR can be merged. This is typically done by the person |
| who requested the changes. |
| |
| ### Additional Rules |
| |
| Some directories and platforms have additional requirements, some of which are |
| automated. All will be validated by reviewers: |
| |
| - Changes to `darwin` require a reviewer from Apple (an approval from |
| `reviewers-apple`). |
| - `/examples` can have their own reviewer list. An approval from a reviewer on |
| that list can lead to a merge. For example, PRs for `examples/chef` will be |
| merged if a member of `reviewers-google` approves it. |
| |
| ### SDK Maintainers |
| |
| [Matter SDK Maintainers](https://github.com/orgs/project-chip/teams/matter-sdk-maintainers) |
| are explicitly empowered to merge a PR when it meets the quality bar. |
| |
| SDK maintainers will: |
| |
| - Validate that the guidelines and quality bar are met. |
| - Ensure sufficient review coverage (including review by the maintainer, if |
| applicable) to maintain a high-quality bar. |
| - Explicitly **push back on time pressure and will not lower the quality bar |
| to meet deadlines**. |
| - Prioritize quality over the PR author's convenience. |
| - Historically, more code has been written than there is bandwidth to |
| review. |
| - They may ask for PRs to be split, sequenced, or for tests to be added, |
| even if this is extra work for the PR author. |
| - They will use their best judgment on what needs to be addressed |
| immediately versus what can be deferred to a follow-up. |
| |
| SDK maintainers are explicitly empowered to mark any PR for merging via an |
| `sdk-maintainer-approved` label. This includes PRs authored by someone from the |
| same company, with some exceptions: |
| |
| - Large architectural changes should be reviewed by several companies or |
| discussed in advance. |
| - A completely new approach may require reviews from several companies. |
| |
| ### Multi-Company Reviews |
| |
| Currently, a PR will be merged if two independent company representatives have |
| approved it and all other conditions (such as CI passing, no open comments, and |
| no blockers) are met. |
| |
| ### `fast-track` |
| |
| A `fast-track` label exists for SDK leads to use for merging trivial changes or |
| good changes that have not yet received reviews. |
| |
| This label was created before `matter-sdk-maintainers` existed, and its usage is |
| now expected to be **very infrequent**. |
| |
| ### Admin Merges |
| |
| In some cases, administrator-forced merges may be appropriate or required. These |
| should be infrequent. Some examples include: |
| |
| - CI is broken and is either unfixable in a timely manner or waiting for a |
| multi-hour CI run is not practical. |
| - Hotfixes to unblock other users. |
| - Rollbacks of breakages where the rollback is clean and we do not want to |
| wait for CI. |