Jason D. Clinton | c38b61d8 | 2021-04-20 20:02:14 | [diff] [blame] | 1 | # Mandatory Code-Review and Native OWNERS |
Yulan Lin | 55ae6a3 | 2020-07-31 17:58:29 | [diff] [blame] | 2 | |
Andrew Williams | e223ab9 | 2021-07-16 23:40:27 | [diff] [blame] | 3 | Beginning on March 24, 2021, committers@ of Chromium are no longer able to |
Jason D. Clinton | c38b61d8 | 2021-04-20 20:02:14 | [diff] [blame] | 4 | circumvent code review and OWNERS approval on CLs. The full |
| 5 | [Code Review](code_reviews.md) documentation has been updated to reflect this. |
Yulan Lin | 55ae6a3 | 2020-07-31 17:58:29 | [diff] [blame] | 6 | |
Jason D. Clinton | c38b61d8 | 2021-04-20 20:02:14 | [diff] [blame] | 7 | Previously, these were circumventable by self-code-review and because the |
| 8 | enforcement was done by presubmit, although rarely done by external |
| 9 | contributors. Now, Gerrit will disallow both bypasses. Within Google, where |
| 10 | these bypasses were more common, Googlers can find Google-specific information |
| 11 | in the internal announcements and landing site. |
Yulan Lin | 55ae6a3 | 2020-07-31 17:58:29 | [diff] [blame] | 12 | |
| 13 | Periodic updates and FAQs will be sent to chromium-dev@chromium.org |
Joe Mason | cffd2d7 | 2021-03-08 22:22:39 | [diff] [blame] | 14 | and updated on this page. |
| 15 | |
| 16 | ## FAQS |
| 17 | |
Jason D. Clinton | c38b61d8 | 2021-04-20 20:02:14 | [diff] [blame] | 18 | ### Do I need a reviewer to merge CL's to another branch, even though they were already reviewed on main? |
Joe Mason | cffd2d7 | 2021-03-08 22:22:39 | [diff] [blame] | 19 | |
Jason D. Clinton | c38b61d8 | 2021-04-20 20:02:14 | [diff] [blame] | 20 | Yes, but within 14 days of the original change you can add Rubber Stamper bot (rubber-stamper@appspot.gserviceaccount.com) as the reviewer. |
| 21 | |
| 22 | ### I have a question, whom should I contact? |
| 23 | |
| 24 | Send questions about this document to chromium-dev@chromium.org. Googlers can |
| 25 | use an internal-specific email alias that was announced, separately. |
| 26 | |
| 27 | ### How will major refactorings be handled? I regularly need to submit 100s of CLs across the trees; getting OWNERS approval from everyone will be too hard. |
| 28 | |
| 29 | We have created a process for landing such changes: |
| 30 | [Chrome Large Scale Changes](/docs/process/lsc/large_scale_changes.md). |
| 31 | |
| 32 | This process allows approved, large refactorings to bypass OWNERS for the |
| 33 | duration, using a special label `Owners-Override`. However, these changes will |
| 34 | still need a second human (anyone with committers `Code-Review +1` powers) to |
| 35 | vote. |
| 36 | |
Kentaro Hara | 0cdc607 | 2021-10-15 00:35:16 | [diff] [blame] | 37 | ### What should I do when I need to get Owners-Override for one-off CLs? |
| 38 | |
| 39 | One-off CLs do not need to go through Large Scale Changes. If the CLs make |
| 40 | only mechanical changes associated with changes in //base/ APIs, //build/ APIs, |
| 41 | //content/ APIs, //url/ APIs or //third_party/blink/public/APIs, the API owners can set `Owners-Override`. |
| 42 | |
John Abd-El-Malek | 704bca0 | 2022-12-14 18:47:59 | [diff] [blame] | 43 | For other one-off CLs, [Chrome ATLs](../ATL_OWNERS) can set `Owners-Override`. |
Kentaro Hara | 0cdc607 | 2021-10-15 00:35:16 | [diff] [blame] | 44 | |
Jason D. Clinton | c38b61d8 | 2021-04-20 20:02:14 | [diff] [blame] | 45 | ### How does Rubber Stamper bot work? |
| 46 | |
| 47 | Rubber Stamper applies the Bot-Commit label to conforming CLs, allowing them to |
| 48 | bypass code review. It supports various benign files, clean cherry-picks, and |
| 49 | clean reverts that should be exempt from code review. |
| 50 | |
| 51 | Rubber Stamper never provides OWNERS approval, by design. It's intended to be |
| 52 | used by those who have owners in the directory modified or who are sheriffs. If |
| 53 | it provided both code review and OWNERS approval, that would be an abuse vector: |
| 54 | that would allow anyone who can create a revert or cherry-pick to land it |
| 55 | without any other person being involved (e.g. the silent revert of security |
| 56 | patches). |
| 57 | |
Kentaro Hara | 0cdc607 | 2021-10-15 00:35:16 | [diff] [blame] | 58 | When you need to get Owners-Override for sheriffing CLs, reach out to Active |
| 59 | Sheriffs or Release Program Managers. If they are not available, send an email |
| 60 | to [email protected]. |
| 61 | |
Jason D. Clinton | c38b61d8 | 2021-04-20 20:02:14 | [diff] [blame] | 62 | ### Will trivial files require code-review? |
| 63 | |
Andrew Williams | e223ab9 | 2021-07-16 23:40:27 | [diff] [blame] | 64 | Rubber Stamper auto-reviewer (described above) reviews CLs that meet strict |
Jason D. Clinton | c38b61d8 | 2021-04-20 20:02:14 | [diff] [blame] | 65 | criteria. (The list of file types is Google-internal.) For example: directories |
| 66 | with no code. |
| 67 | |
| 68 | Essentially, if we can programmatically prove that the CL is benign, then we |
| 69 | should allow a bot to rubber-stamp it so that Gerrit allows submission. One can |
| 70 | imagine that the classes of CLs that fit in this category would grow over time. |
| 71 | |
| 72 | ### Will clean cherry-picks on release branches need review? |
| 73 | |
| 74 | Yes, Rubber Stamper adds CR+1 (Browser) to clean cherry picks. Adding the bot as |
| 75 | a reviewer to your CL will cause it to scan and approve it. |
| 76 | [email protected] is the bot but just typing "Rubber |
| 77 | St..." will autocomplete the full address for you. However, it doesn't provide |
| 78 | OWNERS approval so, if you are cherry-picking a CL that you don't have OWNERS |
| 79 | on, you can get that approval from the Release Program Manager who approved the |
| 80 | cherry-pick. |
| 81 | |
| 82 | ### Does documentation require code review? |
| 83 | |
| 84 | Documentation will require code review. |
| 85 | |
| 86 | There has been much discussion on this topic but senior leaders came to a |
| 87 | majority conclusion that the quality increase in documentation from requiring |
| 88 | code review outweighed any productivity headwinds. |
| 89 | |
| 90 | We will revisit this in the future to evaluate how it is working (or not, as the |
| 91 | case may be). |
| 92 | |
| 93 | ### How do we ensure top-level and parent directory OWNERS aren't overloaded? |
| 94 | |
| 95 | We updated the developer documentation stating that CL authors should |
| 96 | prioritize OWNERS closer to the leaf nodes and not to use top-level owners |
| 97 | because those folks are likely overloaded and the likelihood of a high response |
| 98 | latency or the CL getting lost is high. OWNERS recommendations from Gerrit are |
| 99 | in-line with this. |
| 100 | |
| 101 | ### Does Gerrit block direct push? |
| 102 | |
| 103 | Yes. For break-glass scenarios, there are several folks who have the ability to |
| 104 | direct push, including others' CLs. |
| 105 | |
| 106 | ### OWNERS enforcement and no-TBR are different things. Why did they roll out simultaneously? |
| 107 | |
| 108 | While they are separate, both impact the integrity of Chrome source code and |
| 109 | artifacts and have tangible impacts on developer workflows. For example: TBR was |
| 110 | used to bypass OWNERS and the rollout of this policy prevented this bypass. In |
| 111 | consultation with senior leaders, we decided that rolling both out |
| 112 | simultaneously allowed for more streamlined communication and change management |
| 113 | for the contributor community. |