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