github.com/demonoid81/moby@v0.0.0-20200517203328-62dd8e17c460/project/REVIEWING.md (about)

     1  # Pull request reviewing process
     2  
     3  ## Labels
     4  
     5  Labels are carefully picked to optimize for:
     6  
     7   - Readability: maintainers must immediately know the state of a PR
     8   - Filtering simplicity: different labels represent many different aspects of
     9     the reviewing work, and can even be targeted at different maintainers groups.
    10  
    11  A pull request should only be attributed labels documented in this section: other labels that may
    12  exist on the repository should apply to issues.
    13  
    14  ### DCO labels
    15  
    16   * `dco/no`: automatically set by a bot when one of the commits lacks proper signature
    17  
    18  ### Status labels
    19  
    20   * `status/0-triage`
    21   * `status/1-design-review`
    22   * `status/2-code-review`
    23   * `status/3-docs-review`
    24   * `status/4-merge`
    25  
    26  Special status labels:
    27  
    28   * `status/failing-ci`: indicates that the PR in its current state fails the test suite
    29   * `status/needs-attention`: calls for a collective discussion during a review session
    30  
    31  ### Impact labels (apply to merged pull requests)
    32  
    33   * `impact/api`
    34   * `impact/changelog`
    35   * `impact/cli`
    36   * `impact/deprecation`
    37   * `impact/distribution`
    38   * `impact/dockerfile`
    39  
    40  ### Process labels (apply to merged pull requests)
    41  
    42  Process labels are to assist in preparing (patch) releases. These labels should only be used for pull requests.
    43  
    44  Label                           | Use for
    45  ------------------------------- | -------------------------------------------------------------------------
    46  `process/cherry-pick`           | PRs that should be cherry-picked in the bump/release branch. These pull-requests must also be assigned to a milestone.
    47  `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
    48  `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.
    49  `process/docs-cherry-picked`    | PRs that have been cherry-picked in the docs branch
    50  `process/merge-to-master`       | PRs that are opened directly on the bump/release branch, but also need to be merged back to "master"
    51  `process/merged-to-master`      | PRs that have been merged back to "master"
    52  
    53  
    54  ## Workflow
    55  
    56  An opened pull request can be in 1 of 5 distinct states, for each of which there is a corresponding
    57  label that needs to be applied.
    58  
    59  ### Triage - `status/0-triage`
    60  
    61  Maintainers are expected to triage new incoming pull requests by removing the `status/0-triage`
    62  label and adding the correct labels (e.g. `status/1-design-review`) before any other interaction
    63  with the PR. The starting label may potentially skip some steps depending on the kind of pull
    64  request: use your best judgement.
    65  
    66  Maintainers should perform an initial, high-level, overview of the pull request before moving it to
    67  the next appropriate stage:
    68  
    69   - Has DCO
    70   - Contains sufficient justification (e.g., usecases) for the proposed change
    71   - References the GitHub issue it fixes (if any) in the commit or the first GitHub comment
    72  
    73  Possible transitions from this state:
    74  
    75   * Close: e.g., unresponsive contributor without DCO
    76   * `status/1-design-review`: general case
    77   * `status/2-code-review`: e.g. trivial bugfix
    78   * `status/3-docs-review`: non-proposal documentation-only change
    79  
    80  ### Design review - `status/1-design-review`
    81  
    82  Maintainers are expected to comment on the design of the pull request.  Review of documentation is
    83  expected only in the context of design validation, not for stylistic changes.
    84  
    85  Ideally, documentation should reflect the expected behavior of the code.  No code review should
    86  take place in this step.
    87  
    88  There are no strict rules on the way a design is validated: we usually aim for a consensus,
    89  although a single maintainer approval is often sufficient for obviously reasonable changes. In
    90  general, strong disagreement expressed by any of the maintainers should not be taken lightly.
    91  
    92  Once design is approved, a maintainer should make sure to remove this label and add the next one.
    93  
    94  Possible transitions from this state:
    95  
    96   * Close: design rejected
    97   * `status/2-code-review`: general case
    98   * `status/3-docs-review`: proposals with only documentation changes
    99  
   100  ### Code review - `status/2-code-review`
   101  
   102  Maintainers are expected to review the code and ensure that it is good quality and in accordance
   103  with the documentation in the PR.
   104  
   105  New testcases are expected to be added. Ideally, those testcases should fail when the new code is
   106  absent, and pass when present. The testcases should strive to test as many variants, code paths, as
   107  possible to ensure maximum coverage.
   108  
   109  Changes to code must be reviewed and approved (LGTM'd) by a minimum of two code maintainers. When
   110  the author of a PR is a maintainer, he still needs the approval of two other maintainers.
   111  
   112  Once code is approved according to the rules of the subsystem, a maintainer should make sure to
   113  remove this label and add the next one. If documentation is absent but expected, maintainers should
   114  ask for documentation and move to status `status/3-docs-review` for docs maintainer to follow.
   115  
   116  Possible transitions from this state:
   117  
   118   * Close
   119   * `status/1-design-review`: new design concerns are raised
   120   * `status/3-docs-review`: general case
   121   * `status/4-ready-to-merge`: change not impacting documentation
   122  
   123  ### Docs review - `status/3-docs-review`
   124  
   125  Maintainers are expected to review the documentation in its bigger context, ensuring consistency,
   126  completeness, validity, and breadth of coverage across all existing and new documentation.
   127  
   128  They should ask for any editorial change that makes the documentation more consistent and easier to
   129  understand.
   130  
   131  The docker/docker repository only contains _reference documentation_, all
   132  "narrative" documentation is kept in a [unified documentation
   133  repository](https://github.com/demonoid81/moby.github.io). Reviewers must
   134  therefore verify which parts of the documentation need to be updated. Any
   135  contribution that may require changing the narrative should get the
   136  `impact/documentation` label: this is the signal for documentation maintainers
   137  that a change will likely need to happen on the unified documentation
   138  repository. When in doubt, it’s better to add the label and leave it to
   139  documentation maintainers to decide whether it’s ok to skip. In all cases,
   140  leave a comment to explain what documentation changes you think might be needed.
   141  
   142  - If the pull request does not impact the documentation at all, the docs review
   143    step is skipped, and the pull request is ready to merge.
   144  - If the changes in
   145    the pull request require changes to the reference documentation (either
   146    command-line reference, or API reference), those changes must be included as
   147    part of the pull request and will be reviewed now. Keep in mind that the
   148    narrative documentation may contain output examples of commands, so may need
   149    to be updated as well, in which case the `impact/documentation` label must
   150    be applied.
   151  - If the PR has the `impact/documentation` label, merging is delayed until a
   152    documentation maintainer acknowledges that a corresponding documentation PR
   153    (or issue) is opened on the documentation repository. Once a documentation
   154    maintainer acknowledges the change, she/he will move the PR to `status/4-merge`
   155    for a code maintainer to push the green button.
   156  
   157  Changes and additions to docs must be reviewed and approved (LGTM'd) by a minimum of two docs
   158  sub-project maintainers. If the docs change originates with a docs maintainer, only one additional
   159  LGTM is required (since we assume a docs maintainer approves of their own PR).
   160  
   161  Once documentation is approved, a maintainer should make sure to remove this label and
   162  add the next one.
   163  
   164  Possible transitions from this state:
   165  
   166   * Close
   167   * `status/1-design-review`: new design concerns are raised
   168   * `status/2-code-review`: requires more code changes
   169   * `status/4-ready-to-merge`: general case
   170  
   171  ### Merge - `status/4-ready-to-merge`
   172  
   173  Maintainers are expected to merge this pull request as soon as possible. They can ask for a rebase
   174  or carry the pull request themselves.
   175  
   176  Possible transitions from this state:
   177  
   178   * Merge: general case
   179   * Close: carry PR
   180  
   181  After merging a pull request, the maintainer should consider applying one or multiple impact labels
   182  to ease future classification:
   183  
   184   * `impact/api` signifies the patch impacted the Engine API
   185   * `impact/changelog` signifies the change is significant enough to make it in the changelog
   186   * `impact/cli` signifies the patch impacted a CLI command
   187   * `impact/dockerfile` signifies the patch impacted the Dockerfile syntax
   188   * `impact/deprecation` signifies the patch participates in deprecating an existing feature
   189  
   190  ### Close
   191  
   192  If a pull request is closed it is expected that sufficient justification will be provided. In
   193  particular, if there are alternative ways of achieving the same net result then those needs to be
   194  spelled out. If the pull request is trying to solve a use case that is not one that we (as a
   195  community) want to support then a justification for why should be provided.
   196  
   197  The number of maintainers it takes to decide and close a PR is deliberately left unspecified. We
   198  assume that the group of maintainers is bound by mutual trust and respect, and that opposition from
   199  any single maintainer should be taken into consideration. Similarly, we expect maintainers to
   200  justify their reasoning and to accept debating.
   201  
   202  ## Escalation process
   203  
   204  Despite the previously described reviewing process, some PR might not show any progress for various
   205  reasons:
   206  
   207   - No strong opinion for or against the proposed patch
   208   - Debates about the proper way to solve the problem at hand
   209   - Lack of consensus
   210   - ...
   211  
   212  All these will eventually lead to stalled PR, where no apparent progress is made across several
   213  weeks, or even months.
   214  
   215  Maintainers should use their best judgement and apply the `status/needs-attention` label. It must
   216  be used sparingly, as each PR with such label will be discussed by a group of maintainers during a
   217  review session. The goal of that session is to agree on one of the following outcomes for the PR:
   218  
   219   * Close, explaining the rationale for not pursuing further
   220   * Continue, either by pushing the PR further in the workflow, or by deciding to carry the patch
   221     (ideally, a maintainer should be immediately assigned to make sure that the PR keeps continued
   222     attention)
   223   * Escalate to Solomon by formulating a few specific questions on which his answers will allow
   224     maintainers to decide.
   225  
   226  ## Milestones
   227  
   228  Typically, every merged pull request get shipped naturally with the next release cut from the
   229  `master` branch (either the next minor or major version, as indicated by the
   230  [`VERSION`](https://github.com/demonoid81/moby/blob/master/VERSION) file at the root of the
   231  repository). However, the time-based nature of the release process provides no guarantee that a
   232  given pull request will get merged in time. In other words, all open pull requests are implicitly
   233  considered part of the next minor or major release milestone, and this won't be materialized on
   234  GitHub.
   235  
   236  A merged pull request must be attached to the milestone corresponding to the release in which it
   237  will be shipped: this is both useful for tracking, and to help the release manager with the
   238  changelog generation.
   239  
   240  An open pull request may exceptionally get attached to a milestone to express a particular intent to
   241  get it merged in time for that release. This may for example be the case for an important feature to
   242  be included in a minor release, or a critical bugfix to be included in a patch release.
   243  
   244  Finally, and as documented by the [`PATCH-RELEASES.md`](PATCH-RELEASES.md) process, the existence of
   245  a milestone is not a guarantee that a release will happen, as some milestones will be created purely
   246  for the purpose of bookkeeping