github.com/brahmaroutu/docker@v1.2.1-0.20160809185609-eb28dde01f16/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-ready-to-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  ### Specialty group labels
    32  
    33  Those labels are used to raise awareness of a particular specialty group, either because we need
    34  help in reviewing the PR, or because of the potential impact of the PR on their work:
    35  
    36   * `group/distribution`
    37   * `group/networking`
    38   * `group/security`
    39   * `group/windows`
    40  
    41  ### Impact labels (apply to merged pull requests)
    42  
    43   * `impact/api`
    44   * `impact/changelog`
    45   * `impact/cli`
    46   * `impact/deprecation`
    47   * `impact/distribution`
    48   * `impact/dockerfile`
    49  
    50  ### Process labels (apply to merged pull requests)
    51  
    52  Process labels are to assist in preparing (patch) releases. These labels should only be used for pull requests.
    53  
    54  Label                           | Use for
    55  ------------------------------- | -------------------------------------------------------------------------
    56  `process/cherry-pick`           | PRs that should be cherry-picked in the bump/release branch. These pull-requests must also be assigned to a milestone.
    57  `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
    58  `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.
    59  `process/docs-cherry-picked`    | PRs that have been cherry-picked in the docs branch
    60  `process/merge-to-master`       | PRs that are opened directly on the bump/release branch, but also need to be merged back to "master"
    61  `process/merged-to-master`      | PRs that have been merged back to "master"
    62  
    63  
    64  ## Workflow
    65  
    66  An opened pull request can be in 1 of 5 distinct states, for each of which there is a corresponding
    67  label that needs to be applied.
    68  
    69  ### Triage - `status/0-triage`
    70  
    71  Maintainers are expected to triage new incoming pull requests by removing the `status/0-triage`
    72  label and adding the correct labels (e.g. `status/1-design-review`) before any other interaction
    73  with the PR. The starting label may potentially skip some steps depending on the kind of pull
    74  request: use your best judgement.
    75  
    76  Maintainers should perform an initial, high-level, overview of the pull request before moving it to
    77  the next appropriate stage:
    78  
    79   - Has DCO
    80   - Contains sufficient justification (e.g., usecases) for the proposed change
    81   - References the Github issue it fixes (if any) in the commit or the first Github comment
    82  
    83  Possible transitions from this state:
    84  
    85   * Close: e.g., unresponsive contributor without DCO
    86   * `status/1-design-review`: general case
    87   * `status/2-code-review`: e.g. trivial bugfix
    88   * `status/3-docs-review`: non-proposal documentation-only change
    89  
    90  ### Design review - `status/1-design-review`
    91  
    92  Maintainers are expected to comment on the design of the pull request.  Review of documentation is
    93  expected only in the context of design validation, not for stylistic changes.
    94  
    95  Ideally, documentation should reflect the expected behavior of the code.  No code review should
    96  take place in this step.
    97  
    98  There are no strict rules on the way a design is validated: we usually aim for a consensus,
    99  although a single maintainer approval is often sufficient for obviously reasonable changes. In
   100  general, strong disagreement expressed by any of the maintainers should not be taken lightly.
   101  
   102  Once design is approved, a maintainer should make sure to remove this label and add the next one.
   103  
   104  Possible transitions from this state:
   105  
   106   * Close: design rejected
   107   * `status/2-code-review`: general case
   108   * `status/3-docs-review`: proposals with only documentation changes
   109  
   110  ### Code review - `status/2-code-review`
   111  
   112  Maintainers are expected to review the code and ensure that it is good quality and in accordance
   113  with the documentation in the PR.
   114  
   115  New testcases are expected to be added. Ideally, those testcases should fail when the new code is
   116  absent, and pass when present. The testcases should strive to test as many variants, code paths, as
   117  possible to ensure maximum coverage.
   118  
   119  Changes to code must be reviewed and approved (LGTM'd) by a minimum of two code maintainers. When
   120  the author of a PR is a maintainer, he still needs the approval of two other maintainers.
   121  
   122  Once code is approved according to the rules of the subsystem, a maintainer should make sure to
   123  remove this label and add the next one. If documentation is absent but expected, maintainers should
   124  ask for documentation and move to status `status/3-docs-review` for docs maintainer to follow.
   125  
   126  Possible transitions from this state:
   127  
   128   * Close
   129   * `status/1-design-review`: new design concerns are raised
   130   * `status/3-docs-review`: general case
   131   * `status/4-ready-to-merge`: change not impacting documentation
   132  
   133  ### Docs review - `status/3-docs-review`
   134  
   135  Maintainers are expected to review the documentation in its bigger context, ensuring consistency,
   136  completeness, validity, and breadth of coverage across all existing and new documentation.
   137  
   138  They should ask for any editorial change that makes the documentation more consistent and easier to
   139  understand.
   140  
   141  Changes and additions to docs must be reviewed and approved (LGTM'd) by a minimum of two docs
   142  sub-project maintainers. If the docs change originates with a docs maintainer, only one additional
   143  LGTM is required (since we assume a docs maintainer approves of their own PR).
   144  
   145  Once documentation is approved (see below), a maintainer should make sure to remove this label and
   146  add the next one.
   147  
   148  Possible transitions from this state:
   149  
   150   * Close
   151   * `status/1-design-review`: new design concerns are raised
   152   * `status/2-code-review`: requires more code changes
   153   * `status/4-ready-to-merge`: general case
   154  
   155  ### Merge - `status/4-ready-to-merge`
   156  
   157  Maintainers are expected to merge this pull request as soon as possible. They can ask for a rebase
   158  or carry the pull request themselves.
   159  
   160  Possible transitions from this state:
   161  
   162   * Merge: general case
   163   * Close: carry PR
   164  
   165  After merging a pull request, the maintainer should consider applying one or multiple impact labels
   166  to ease future classification:
   167  
   168   * `impact/api` signifies the patch impacted the remote API
   169   * `impact/changelog` signifies the change is significant enough to make it in the changelog
   170   * `impact/cli` signifies the patch impacted a CLI command
   171   * `impact/dockerfile` signifies the patch impacted the Dockerfile syntax
   172   * `impact/deprecation` signifies the patch participates in deprecating an existing feature
   173  
   174  ### Close
   175  
   176  If a pull request is closed it is expected that sufficient justification will be provided. In
   177  particular, if there are alternative ways of achieving the same net result then those needs to be
   178  spelled out. If the pull request is trying to solve a use case that is not one that we (as a
   179  community) want to support then a justification for why should be provided.
   180  
   181  The number of maintainers it takes to decide and close a PR is deliberately left unspecified. We
   182  assume that the group of maintainers is bound by mutual trust and respect, and that opposition from
   183  any single maintainer should be taken into consideration. Similarly, we expect maintainers to
   184  justify their reasoning and to accept debating.
   185  
   186  ## Escalation process
   187  
   188  Despite the previously described reviewing process, some PR might not show any progress for various
   189  reasons:
   190  
   191   - No strong opinion for or against the proposed patch
   192   - Debates about the proper way to solve the problem at hand
   193   - Lack of consensus
   194   - ...
   195  
   196  All these will eventually lead to stalled PR, where no apparent progress is made across several
   197  weeks, or even months.
   198  
   199  Maintainers should use their best judgement and apply the `status/needs-attention` label. It must
   200  be used sparingly, as each PR with such label will be discussed by a group of maintainers during a
   201  review session. The goal of that session is to agree on one of the following outcomes for the PR:
   202  
   203   * Close, explaining the rationale for not pursuing further
   204   * Continue, either by pushing the PR further in the workflow, or by deciding to carry the patch
   205     (ideally, a maintainer should be immediately assigned to make sure that the PR keeps continued
   206     attention)
   207   * Escalate to Solomon by formulating a few specific questions on which his answers will allow
   208     maintainers to decide.
   209