github.com/portworx/docker@v1.12.1/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