How Pull Request Reviews Work in the Matter SDK

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, their approval generally counts as a “company approval” (see “When do PRs merge” below).

  • A separate group, 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. 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 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.