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.