Skip to content

Latest commit

 

History

History
83 lines (76 loc) · 5.19 KB

PROCESS.md

File metadata and controls

83 lines (76 loc) · 5.19 KB
Raw

Code Review using OWNERS files

This is a simplified description of our full PR testing and merge workflow that conveniently forgets about the existence of tests, to focus solely on the roles driven by OWNERS files.

The Code Review Process

  • The author submits a PR
  • Phase 0: Automation suggests reviewers and approvers for the PR
    • Determine the set of OWNERS files nearest to the code being changed
    • Choose at least two suggested reviewers, trying to find a unique reviewer for every leaf OWNERS file, and request their reviews on the PR
    • Choose suggested approvers, one from each OWNERS file, and list them in a comment on the PR
  • Phase 1: Humans review the PR
    • Reviewers look for general code quality, correctness, sane software engineering, style, etc.
    • Anyone in the organization can act as a reviewer with the exception of the individual who opened the PR
    • If the code changes look good to them, a reviewer types /lgtm in a PR comment or review; if they change their mind, they /lgtm cancel
    • Once a reviewer has /lgtm'ed, Bot applies an lgtm label to the PR
  • Phase 2: Humans approve the PR
    • The PR author /assign's all suggested approvers to the PR, and optionally notifies them (eg: "pinging @foo for approval")
    • Only people listed in the relevant OWNERS files, can act as approvers, including the individual who opened the PR.
    • Approvers look for holistic acceptance criteria, including dependencies with other features, forwards/backwards compatibility, API and flag definitions, etc
    • If the code changes look good to them, an approver types /approve in a PR comment or review; if they change their mind, they /approve cancel
    • Bot updates its comment in the PR to indicate which approvers still need to approve
    • Once all approvers (one from each of the previously identified OWNERS files) have approved, Bot applies an approved label
  • Phase 3: Automation merges the PR:
    • If all of the following are true:
      • All required labels are present (eg: lgtm, approved)
      • Any blocking labels are missing (eg: there is no do-not-merge)
    • And if any of the following are true:
      • there are no presubmit prow jobs configured for this repo
      • there are presubmit prow jobs configured for this repo, and they all pass after automatically being re-run one last time
    • Then the PR will automatically be merged

Quirks of the Process

There are a number of behaviors we've observed that while possible are discouraged, as they go against the intent of this review process. Some of these could be prevented in the future, but this is the state of today.

  • An approver's /lgtm is simultaneously interpreted as an /approve
    • While a convenient shortcut for some, it can be surprising that the same command is interpreted in one of two ways depending on who the commenter is
    • Instead, explicitly write out /lgtm and /approve to help observers, or save the /lgtm for a reviewer
    • This goes against the idea of having at least two sets of eyes on a PR, and may be a sign that there are too few reviewers (who aren't also approver)
  • Technically, anyone who is a reviewer of the project can drive-by /lgtm a PR
    • Drive-by reviews from non-members are encouraged as a way of demonstrating experience and intent to become a member or reviewer.
    • Drive-by /lgtm's from reviewers may be a sign that our OWNERS files are too small, or that the existing reviewers are too unresponsive
    • This goes against the idea of specifying reviewers in the first place, to ensure that author is getting actionable feedback from people knowledgeable with the code
  • Reviewers, and approvers are unresponsive
    • This causes a lot of frustration for authors who often have little visibility into why their PR is being ignored
    • Many reviewers and approvers are so overloaded by GitHub notifications that @mention'ing is unlikely to get a quick response
    • If an author /assign's a PR, reviewers and approvers will be made aware of it on their PR dashboard
    • An author can work around this by manually reading the relevant OWNERS files, /unassign'ing unresponsive individuals, and /assign'ing others
    • This is a sign that our OWNERS files are stale; pruning the reviewers and approvers lists would help with this
  • Authors are unresponsive
    • This costs a tremendous amount of attention as context for an individual PR is lost over time
    • This hurts the project in general as its general noise level increases over time
    • Instead, close PR's that are untouched after too long (we currently have a bot do this after 90 days)