moby/project/REVIEWING.md

# Pull request reviewing process

## Labels

Labels are carefully picked to optimize for:

 - Readability: maintainers must immediately know the state of a PR
 - Filtering simplicity: different labels represent many different aspects of
   the reviewing work, and can even be targeted at different maintainers groups.

A pull request should only be attributed labels documented in this section: other labels that may
exist on the repository should apply to issues.

### DCO labels

 * `dco/no`: automatically set by a bot when one of the commits lacks proper signature

### Status labels

 * `status/0-triage`
 * `status/1-design-review`
 * `status/2-code-review`
 * `status/3-docs-review`
 * `status/4-ready-to-merge`

Special status labels:

 * `status/failing-ci`: indicates that the PR in its current state fails the test suite
 * `status/needs-attention`: calls for a collective discussion during a review session

### Impact labels (apply to merged pull requests)

 * `impact/api`
 * `impact/changelog`
 * `impact/cli`
 * `impact/deprecation`
 * `impact/distribution`
 * `impact/dockerfile`

### Process labels (apply to merged pull requests)

Process labels are to assist in preparing (patch) releases. These labels should only be used for pull requests.

Label                           | Use for
------------------------------- | -------------------------------------------------------------------------
`process/cherry-pick`           | PRs that should be cherry-picked in the bump/release branch. These pull-requests must also be assigned to a milestone.
`process/cherry-picked`         | PRs that have been cherry-picked. This label is helpful to find PR's that have been added to release-candidates, and to update the change log
`process/docs-cherry-pick`      | PRs that should be cherry-picked in the docs branch. Only apply this label for changes that apply to the *current* release, and generic documentation fixes, such as Markdown and spelling fixes.
`process/docs-cherry-picked`    | PRs that have been cherry-picked in the docs branch
`process/merge-to-master`       | PRs that are opened directly on the bump/release branch, but also need to be merged back to "master"
`process/merged-to-master`      | PRs that have been merged back to "master"


## Workflow

An opened pull request can be in 1 of 5 distinct states, for each of which there is a corresponding
label that needs to be applied.

### Triage - `status/0-triage`

Maintainers are expected to triage new incoming pull requests by removing the `status/0-triage`
label and adding the correct labels (e.g. `status/1-design-review`) before any other interaction
with the PR. The starting label may potentially skip some steps depending on the kind of pull
request: use your best judgement.

Maintainers should perform an initial, high-level, overview of the pull request before moving it to
the next appropriate stage:

 - Has DCO
 - Contains sufficient justification (e.g., usecases) for the proposed change
 - References the Github issue it fixes (if any) in the commit or the first Github comment

Possible transitions from this state:

 * Close: e.g., unresponsive contributor without DCO
 * `status/1-design-review`: general case
 * `status/2-code-review`: e.g. trivial bugfix
 * `status/3-docs-review`: non-proposal documentation-only change

### Design review - `status/1-design-review`

Maintainers are expected to comment on the design of the pull request.  Review of documentation is
expected only in the context of design validation, not for stylistic changes.

Ideally, documentation should reflect the expected behavior of the code.  No code review should
take place in this step.

There are no strict rules on the way a design is validated: we usually aim for a consensus,
although a single maintainer approval is often sufficient for obviously reasonable changes. In
general, strong disagreement expressed by any of the maintainers should not be taken lightly.

Once design is approved, a maintainer should make sure to remove this label and add the next one.

Possible transitions from this state:

 * Close: design rejected
 * `status/2-code-review`: general case
 * `status/3-docs-review`: proposals with only documentation changes

### Code review - `status/2-code-review`

Maintainers are expected to review the code and ensure that it is good quality and in accordance
with the documentation in the PR.

New testcases are expected to be added. Ideally, those testcases should fail when the new code is
absent, and pass when present. The testcases should strive to test as many variants, code paths, as
possible to ensure maximum coverage.

Changes to code must be reviewed and approved (LGTM'd) by a minimum of two code maintainers. When
the author of a PR is a maintainer, he still needs the approval of two other maintainers.

Once code is approved according to the rules of the subsystem, a maintainer should make sure to
remove this label and add the next one. If documentation is absent but expected, maintainers should
ask for documentation and move to status `status/3-docs-review` for docs maintainer to follow.

Possible transitions from this state:

 * Close
 * `status/1-design-review`: new design concerns are raised
 * `status/3-docs-review`: general case
 * `status/4-ready-to-merge`: change not impacting documentation

### Docs review - `status/3-docs-review`

Maintainers are expected to review the documentation in its bigger context, ensuring consistency,
completeness, validity, and breadth of coverage across all existing and new documentation.

They should ask for any editorial change that makes the documentation more consistent and easier to
understand.

The docker/docker repository only contains _reference documentation_, all
"narrative" documentation is kept in a [unified documentation
repository](https://github.com/docker/docker.github.io). Reviewers must
therefore verify which parts of the documentation need to be updated. Any
contribution that may require changing the narrative should get the
`impact/documentation` label: this is the signal for documentation maintainers
that a change will likely need to happen on the unified documentation
repository. When in doubt, it’s better to add the label and leave it to
documentation maintainers to decide whether it’s ok to skip. In all cases,
leave a comment to explain what documentation changes you think might be needed.

- If the pull request does not impact the documentation at all, the docs review
  step is skipped, and the pull request is ready to merge.
- If the changes in
  the pull request require changes to the reference documentation (either
  command-line reference, or API reference), those changes must be included as
  part of the pull request and will be reviewed now. Keep in mind that the
  narrative documentation may contain output examples of commands, so may need
  to be updated as well, in which case the `impact/documentation` label must
  be applied.
- If the PR has the `impact/documentation` label, merging is delayed until a
  documentation maintainer acknowledges that a corresponding documentation PR
  (or issue) is opened on the documentation repository. Once a documentation
  maintainer acknowledges the change, she/he will move the PR to `status/4-merge`
  for a code maintainer to push the green button.

Changes and additions to docs must be reviewed and approved (LGTM'd) by a minimum of two docs
sub-project maintainers. If the docs change originates with a docs maintainer, only one additional
LGTM is required (since we assume a docs maintainer approves of their own PR).

Once documentation is approved, a maintainer should make sure to remove this label and
add the next one.

Possible transitions from this state:

 * Close
 * `status/1-design-review`: new design concerns are raised
 * `status/2-code-review`: requires more code changes
 * `status/4-ready-to-merge`: general case

### Merge - `status/4-ready-to-merge`

Maintainers are expected to merge this pull request as soon as possible. They can ask for a rebase
or carry the pull request themselves.

Possible transitions from this state:

 * Merge: general case
 * Close: carry PR

After merging a pull request, the maintainer should consider applying one or multiple impact labels
to ease future classification:

 * `impact/api` signifies the patch impacted the Engine API
 * `impact/changelog` signifies the change is significant enough to make it in the changelog
 * `impact/cli` signifies the patch impacted a CLI command
 * `impact/dockerfile` signifies the patch impacted the Dockerfile syntax
 * `impact/deprecation` signifies the patch participates in deprecating an existing feature

### Close

If a pull request is closed it is expected that sufficient justification will be provided. In
particular, if there are alternative ways of achieving the same net result then those needs to be
spelled out. If the pull request is trying to solve a use case that is not one that we (as a
community) want to support then a justification for why should be provided.

The number of maintainers it takes to decide and close a PR is deliberately left unspecified. We
assume that the group of maintainers is bound by mutual trust and respect, and that opposition from
any single maintainer should be taken into consideration. Similarly, we expect maintainers to
justify their reasoning and to accept debating.

## Escalation process

Despite the previously described reviewing process, some PR might not show any progress for various
reasons:

 - No strong opinion for or against the proposed patch
 - Debates about the proper way to solve the problem at hand
 - Lack of consensus
 - ...

All these will eventually lead to stalled PR, where no apparent progress is made across several
weeks, or even months.

Maintainers should use their best judgement and apply the `status/needs-attention` label. It must
be used sparingly, as each PR with such label will be discussed by a group of maintainers during a
review session. The goal of that session is to agree on one of the following outcomes for the PR:

 * Close, explaining the rationale for not pursuing further
 * Continue, either by pushing the PR further in the workflow, or by deciding to carry the patch
   (ideally, a maintainer should be immediately assigned to make sure that the PR keeps continued
   attention)
 * Escalate to Solomon by formulating a few specific questions on which his answers will allow
   maintainers to decide.

## Milestones

Typically, every merged pull request get shipped naturally with the next release cut from the
`master` branch (either the next minor or major version, as indicated by the
[`VERSION`](https://github.com/docker/docker/blob/master/VERSION) file at the root of the
repository). However, the time-based nature of the release process provides no guarantee that a
given pull request will get merged in time. In other words, all open pull requests are implicitly
considered part of the next minor or major release milestone, and this won't be materialized on
GitHub.

A merged pull request must be attached to the milestone corresponding to the release in which it
will be shipped: this is both useful for tracking, and to help the release manager with the
changelog generation.

An open pull request may exceptionally get attached to a milestone to express a particular intent to
get it merged in time for that release. This may for example be the case for an important feature to
be included in a minor release, or a critical bugfix to be included in a patch release.

Finally, and as documented by the [`PATCH-RELEASES.md`](PATCH-RELEASES.md) process, the existence of
a milestone is not a guarantee that a release will happen, as some milestones will be created purely
for the purpose of bookkeeping