github.com/akerouanton/docker@v1.11.0-rc3/project/REVIEWING.md (about)

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