Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 1 | # Merge Request Process |
| 2 | |
| 3 | [TOC] |
| 4 | |
| 5 | ## tl;dr |
| 6 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 7 | * Release managers (and delegates like the security team) must review all |
| 8 | merges made to release branches |
| 9 | * Merge criteria become more strict as the stable release date approaches; use |
| 10 | Chromium Dash's [Branches page](https://chromiumdash.appspot.com/branches) to |
| 11 | understand which branches are active and what merges are acceptable for each |
| 12 | branch |
| 13 | * Ensure your change is [safe to merge](#verifying-eligibility-and-safety) |
| 14 | before initiating the merge review process unless it's time-sensitive |
| 15 | * Use Monorail's [project queries](#monitoring-merge-requests) to track your |
| 16 | approved merges as well as your pending requests |
| 17 | * Use Gerrit or git to land your merge only after it's been approved |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 18 | |
| 19 | ## Introduction |
| 20 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 21 | Chromium is a main-first development team; generally, all code should land on |
| 22 | main then roll out to stable users only after the milestone containing the code |
| 23 | is branched, stabilized and shipped to the stable channel (to learn more about |
| 24 | the release cycle, click |
| 25 | [here](https://chromium.googlesource.com/chromium/src.git/+/main/docs/process/release_cycle.md)). |
| 26 | This is because merging (also known as cherry-picking) code to an older release |
| 27 | branch introduces risk and costs time across the team. However, there are times |
| 28 | when the benefits outweigh the costs and a merge might be appropriate, e.g. to |
| 29 | fix a web platform regression, address a crash or patch a security vulnerability. |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 30 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 31 | To ensure we make the right decisions, release managers leverage a merge review |
| 32 | process to evaluate each request. They'll ask questions about the reason you |
| 33 | would like to merge a change and the risk of the merge itself, and you'll work |
| 34 | together to make a judgement call on whether or not the merge should be |
| 35 | approved or rejected. |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 36 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 37 | Generally, merges follow these high-level steps: |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 38 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 39 | * Developers update bug with relevant details and request a merge using the |
| 40 | *Merge-Request-##* label, then wait for review |
| 41 | * Release managers and automation review and approve, reject, or ask |
| 42 | questions about the merge within two business days |
| 43 | * Developers wait for review and, if approved, land the merge ASAP |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 44 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 45 | For details on each step, see below. |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 46 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 47 | **NOTE:** Because security issues (identified with *Type=Bug-Security*) follow |
| 48 | a more complex flow, you may simply mark security issues as *Fixed* in Monorail |
| 49 | and [automation](#security-merge-triage) will handle the remainder of the merge |
| 50 | request process flow for you; simply process the merge if it is requested and |
| 51 | approved. |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 52 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 53 | ## Requesting a merge |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 54 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 55 | ### Verifying eligibility and safety |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 56 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 57 | Before requesting a merge, first ensure your change is a good merge candidate: |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 58 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 59 | * Ensure it meets the merge criteria (via |
| 60 | [Chromium Dash](https://chromiumdash.appspot.com/branches)) of the branch(es) |
| 61 | you'd like to merge to; merge criteria become more strict the older the |
| 62 | branch is, more details on criteria [below](#merge-criteria-phases) |
| 63 | * Verify merging the change to an older branch would be safe, e.g. unlikely to |
| 64 | introduce new regressions, no major merge conflicts, automated test coverage |
| 65 | present, etc; chat with your TL for input if you're not sure |
| 66 | * Confirm your change fixes the issue at hand, preferably by testing on and |
| 67 | monitoring the canary channel for 24 hours post-release (see |
| 68 | [Chromium Dash](https://chromiumdash.appspot.com/commits) to determine if |
| 69 | your change has shipped) |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 70 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 71 | * You may skip this step if a release manager or security team member has |
| 72 | told you that the merge is urgent, e.g. is actively blocking a release |
Alex Mineer | 40d33c3 | 2018-03-15 23:45:14 | [diff] [blame] | 73 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 74 | ### Updating crbug/ |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 75 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 76 | Next, ensure you have a crbug/ (generally the bug being fixed by the merge) |
| 77 | with the following information present and accurate: |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 78 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 79 | * Title and description clearly describing the bug being fixed |
| 80 | * Priority (*Pri-#*), OS (*OS-OS*) and target milestone(s) (*Target-##*) |
| 81 | * Owner, generally the person requesting / performing the merge |
| 82 | * [Release block label](./release_blockers.md) if applicable |
| 83 | (*ReleaseBlock=Channel*) |
| 84 | * Issue status: |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 85 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 86 | * Fixed: You're confident the issue is fixed on main, e.g. you've |
| 87 | locally built and tested the issue, no additional crash reports are |
| 88 | generated after the fix was released, etc (most issues) |
| 89 | * Assigned / Started: Diagnostic merges only, e.g. to merge code to track |
| 90 | down the root cause of an issue that only exists on branch |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 91 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 92 | ### Applying merge request label |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 93 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 94 | Once you've verified all the above, you're ready to request a merge! Simply add |
| 95 | the label *Merge-Request-##* to the issue (where ## indicates the milestone |
| 96 | you'd like to merge to), and use multiple labels for multiple milestones, e.g. |
| 97 | *Merge-Request-91 Merge-Request-92* for M91 and M92. Please also copy the |
| 98 | following questions and answer them in a comment on the issue: |
Alan Cutter | cba85fbf | 2019-10-18 06:21:47 | [diff] [blame] | 99 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 100 | 1. Why does your merge fit within the merge criteria for these milestones |
| 101 | ([Chrome Browser](https://chromiumdash.appspot.com/branches), |
| 102 | [Chrome OS](https://goto.google.com/cros-release-branch-merge-guidelines))? |
| 103 | 2. What changes specifically would you like to merge? Please link to Gerrit. |
| 104 | 3. Have the changes been released and tested on canary? |
| 105 | 4. Is this a new feature? If yes, is it behind a Finch flag and are |
| 106 | experiments active in any release channels? |
| 107 | 5. [Chrome OS only]: Was the change reviewed and approved by the |
| 108 | [Eng Prod Representative](https://goto.google.com/cros-engprodcomponents)? |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 109 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 110 | ## Monitoring merge requests |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 111 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 112 | After you've applied the *Merge-Request-##* label, automation will evaluate |
| 113 | your request and may either approve it, reject it, or pass it along to a |
| 114 | release manager for manual evaluation; see [here](#merge-request-triage) to |
| 115 | learn more about this automation. If manual review is required, release |
| 116 | managers strive to answer all merge requests within two business days, but |
| 117 | extenuating circumstances may cause delays. |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 118 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 119 | At this point, following along via bug comments sent by email will always keep |
| 120 | you in the loop, but you can also use the following saved project queries in |
| 121 | Monorail (dropdown to the left of the search bar) to track your merges: |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 122 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 123 | * [Approved and TBD merges](https://bugs.chromium.org/p/chromium/issues/list?q=owner%3Ame&can=41025836): |
| 124 | Merges that require your follow-up, either by landing the relevant |
| 125 | merge (if approved) or determining whether or not a merge is actually |
| 126 | required and if so, requesting it (if TBD) |
| 127 | * [Requested merges](https://bugs.chromium.org/p/chromium/issues/list?q=owner%3Ame&can=41025837): |
| 128 | Merges that are waiting for input from release managers or automation; feel |
| 129 | free to ping bugs that sit in this queue for two business days (assuming you |
| 130 | verified that the change was already deployed to canary ahead of requesting a |
| 131 | merge) |
| 132 | * [Rejected and NA merges](https://bugs.chromium.org/p/chromium/issues/list?q=owner%3Ame&can=41025838): |
| 133 | Merges that were either rejected by release managers, or not applicable to be |
| 134 | merged; generally, no action is needed for these items unless you disagree |
| 135 | with a merge's rejection and wish to escalate |
| 136 | * [All merges](https://bugs.chromium.org/p/chromium/issues/list?q=owner%3Ame&can=41025839): |
| 137 | Includes every possible merge state, useful when wanting to find an item you |
| 138 | considered for merging but can't recall the state it was last in. |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 139 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 140 | For a description of each label used to track the merge process, see the |
| 141 | appendix [below](#merge-states-and-labels). |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 142 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 143 | ## Landing an approved merge |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 144 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 145 | Once your merge has been approved for a given milestone (via the release |
| 146 | manager or automation applying the *Merge-Approved-##* label), you have two |
| 147 | options to land the merge: |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 148 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 149 | * Gerrit UI, easiest for clean cherry-picks or those requiring only minor |
| 150 | changes |
| 151 | * git, for more complex cherry-picks and / or when local verification may be |
| 152 | beneficial |
Alan Cutter | e338ed9 | 2021-05-06 04:57:19 | [diff] [blame] | 153 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 154 | Regardless of which method you choose, please ensure you land your cherry-pick |
| 155 | ASAP so that it can be included in the next release built from the branch; if |
| 156 | you don't merge your cherry-pick soon after approval, it will eventually be |
| 157 | rejected for merge. |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 158 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 159 | ### Using Gerrit UI |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 160 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 161 | Select the "More" button in the Gerrit UI, then choose "Cherry Pick". When |
| 162 | prompted for a branch, enter *refs/branch-heads/####*, where #### corresponds |
| 163 | to the release branch you are merging to (available on |
| 164 | [Chromium Dash](https://chromiumdash.appspot.com/branches)). |
Abdul Syed | e21aa1b | 2017-07-06 17:13:06 | [diff] [blame] | 165 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 166 | Once the cherry-pick CL is prepared, you can have it approved and landed by |
| 167 | adding Rubber Stamper ([email protected]) as a |
| 168 | reviewer and setting Auto-Submit+1;the Rubber Stamper bot will approve and |
| 169 | submit the CL to CQ on your behalf. |
Ian Barkley-Yeung | 3af975e | 2021-04-06 19:00:35 | [diff] [blame] | 170 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 171 | *Note: the Rubber Stamper does not provide OWNERS approval, and only works |
| 172 | within 7 days of the original change; Googlers can learn more |
| 173 | [here](https://goto.google.com/rubber-stamper-user-guide).* |
Ian Barkley-Yeung | 3af975e | 2021-04-06 19:00:35 | [diff] [blame] | 174 | |
Alex Mineer | c5b6302 | 2021-08-26 21:36:05 | [diff] [blame^] | 175 | ### Using git |
| 176 | |
| 177 | The commands below should set up your environment to be able to successfully |
| 178 | upload a cherry-pick to a release branch, where *####* corresponds to the |
| 179 | release branch you are merging to (available on |
| 180 | [Chromium Dash](https://chromiumdash.appspot.com/branches)): |
| 181 | |
| 182 | ``` |
| 183 | $ gclient sync --with_branch_heads |
| 184 | $ git fetch |
| 185 | $ git checkout -b BRANCH_NAME refs/remotes/branch-heads/#### |
| 186 | $ git cl upstream branch-heads/#### |
| 187 | $ git cherry-pick -x COMMIT_HASH_MAIN |
| 188 | ``` |
| 189 | |
| 190 | From here, your environment should be ready to adjust the change as required; |
| 191 | use ninja to build and test your changes, and when ready upload for review: |
| 192 | |
| 193 | ``` |
| 194 | $ git cl upload |
| 195 | ``` |
| 196 | |
| 197 | **Adjust the change description** to omit the "Change-Id: ..." line from |
| 198 | original patch, otherwise you may experience issues when uploading the change |
| 199 | to Gerrit. Once complete, use Gerrit to initiate review and approval of the |
| 200 | merge as TBR has been discontinued. |
| 201 | |
| 202 | Other tips & tricks when merging with git via release branches: |
| 203 | * Consider using multiple working directories when creating the release branch |
| 204 | * Editing the change description to denote this is a merge (e.g. "Merge to |
| 205 | release branch" at the top) will help reviewers distinguish between the |
| 206 | cherry-pick and the original change |
| 207 | |
| 208 | ## Merge automation |
| 209 | |
| 210 | The release team has built automation via |
| 211 | [Sheriffbot](https://www.chromium.org/issue-tracking/autotriage) to assist in |
| 212 | several merge flows: security merge triage, general merge request triage, and |
| 213 | preventing missed merges. |
| 214 | |
| 215 | ### Security merge triage |
| 216 | |
| 217 | Given the additional complexity inherent in security merges, the security team |
| 218 | has built custom automation to handle this flow end to end; simply mark any |
| 219 | security issue as *Fixed* and Sheriffbot will evaluate applicable milestones, |
| 220 | determine if merges are required and automatically request them if need be. |
| 221 | |
| 222 | ### Merge request triage |
| 223 | |
| 224 | To reduce release manager toil, Sheriffbot performs the first pass review of |
| 225 | all merge requests; it may auto-approve the issue if it can detect the issue |
| 226 | meets the right criteria for the current merge phase (e.g. a ReleaseBlock-Dev |
| 227 | issue requesting a merge before beta promotion), and it may auto-reject the |
| 228 | issue similarly (e.g. a Pri-3 issue requesting a merge post-stable). If it |
| 229 | cannot decide, it will pass the issue to a release manager for manual review. |
| 230 | |
| 231 | Generally, Sheriffbot takes action on merge requests only after one of the two |
| 232 | conditions below are met: |
| 233 | |
| 234 | * One or more changelists (via Gitwatcher) are present on the merge request |
| 235 | issue, and all changes have been landed for >= 24 hours |
| 236 | * No changelists are present on the merge request issue, and the merge request |
| 237 | label has been applied for >= 24 hours |
| 238 | |
| 239 | These conditions help ensure any relevant changelists have had sufficient |
| 240 | runtime in our canary channel and thus are low risk for introducing a new |
| 241 | regression onto our release branch. |
| 242 | |
| 243 | ### Preventing missed merges |
| 244 | |
| 245 | To avoid the situation where a critical issue is present on a release branch |
| 246 | but the fix isn't merged, Sheriffbot evaluates all release-blocking issues |
| 247 | targeting a milestone that has already branched and adds a *Merge-TBD-##* label |
| 248 | if the issue was marked as fixed after branch day but hasn't been merged. |
| 249 | When this occurs, developers should evaluate the issue and either request a |
| 250 | merge if required (e.g. the fix did miss the release branch point) by adding |
| 251 | the *Merge-Request-##* label, or add the *Merge-NA-##* label if not (e.g. the |
| 252 | fix is present in the release branch already or the merge is unnecessary for |
| 253 | other reasons). |
| 254 | |
| 255 | ## Appendix |
| 256 | |
| 257 | ### Merge criteria phases |
| 258 | |
| 259 | The table below describes the different phases that each milestone progresses |
| 260 | through during its release cycle; this data is available via the |
| 261 | Chromium Dash [front-end](https://chromiumdash.appspot.com/branches) and |
| 262 | [API](https://chromiumdash.appspot.com/fetch_milestones). |
| 263 | |
| 264 | | Branch Phase | Period Begins | Period Ends | Acceptable Merges Include Fixes For: | |
| 265 | |--------------------------|-----------------|-----------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| |
| 266 | | branch | M(X) Branch | M(X) Beta | Polish issues for Finch-gated features (no workflow changes), any new regressions, any release blockers, any security issues, any string issues (.GRD changes) | |
| 267 | | beta | M(X) Beta | M(X) Stable Cut | Non-functional issues for Finch-gated features (e.g. add metrics, fix crash), noticeable new regressions, any release blockers, any security issues, urgent string issues (.GRD changes) | |
| 268 | | stable_cut | M(X) Stable Cut | M(X) Stable | Urgent new regressions, all release blockers, important security issues (medium severity or higher), emergency string issues (.GRD changes) | |
| 269 | | stable | M(X) Stable | M(X+1) Stable | Urgent new regressions (especially user reports), urgent release blockers, important security issues (medium severity or higher) | |
| 270 | | extended (if applicable) | M(X+1) Stable | M(X+2) Stable | Important security issues (medium severity or higher) applicable to Windows, Mac or Chrome OS | |
| 271 | |
| 272 | ### Merge states and labels |
| 273 | |
| 274 | The table below describes the different merge states and labels used to track |
| 275 | them. All labels follow the form *Merge-[State]-##*, where ## corresponds to |
| 276 | the applicable milestone. If multiple merges are required, these labels may |
| 277 | appear multiple times on the same bug in different states (e.g. a merge request |
| 278 | could have both *Merge-Approved-92* and *Merge-Rejected-91* at the same time). |
| 279 | |
| 280 | | Label / State | Step Owner | Next Steps | |
| 281 | |---------------|------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| |
| 282 | | Request | Release manager | Automation will review and either approve / reject directly, or pass the review to a release manager for manual evaluation | |
| 283 | | Review | Release manager | Release manager will evaluate and either approve, reject, or request additional information within two business days | |
| 284 | | Approved | Issue owner | Issue owner should cherry-pick the fix to the appropriate release branch ASAP | |
| 285 | | Merged | None | N/A; merge has already been landed, no further work required for given milestone | |
| 286 | | Rejected | Issue owner | Issue owner should re-add *Merge-Request-##* to escalate if they feel the merge was erroneously rejected and should be re-evaluated | |
| 287 | | TBD | Issue owner | Issue owner should evaluate if a merge is required, then remove *Merge-TBD-##* and replace it with *Merge-NA-##* (if no merge needed) or *Merge-Request-##* (if merge needed) | |
| 288 | | NA | None | N/A; merge is not required to the relevant milestone, no further work required for given milestone | |