# Trunk-based Development and Code Review Guideline
> 封面圖片: [Image Creator](https://www.bing.com/images/create/trunk-based-development-and-code-review-guideline/6525315afde843f480318e1e4ff7862b?id=2WmcpB%2fQ1p9qkP7C1vmcUQ%3d%3d&view=detailv2&idpp=genimg&FORM=GCRIDP&mode=overlay)
> 背景音樂 Spotify 歌單：[Songs about Git - playlist by Leon | Spotify](https://open.spotify.com/playlist/0MJBni0UzdnML1amikx0Rc?si=1a294df912704a82&nd=1)

:::info
預計在下次的活動會用這份資料來討論如何實踐 TBD
:::

1. Peer Programing 來取代 Code Review
2. 在 Remote Repository 永遠只有 main branch
3. 異動採取小批量並直接推動到 main branch，以快速取得反饋 (測試結果)，若有問題也能快速定位修正


> 結對的程式設計師比兩個獨立工作的程式設計師慢了 15 %，然而 「無錯誤」的程式碼量卻從 70% 增加到了 85%。由於測試和偵錯的成本通常比程式設計本身高出許多倍，結對程式設計的成果相當驚人。相對於獨立作業的程式設計師，結對通常會考慮到更多種設計選擇，從而獲得更簡潔且更可維護的設計方案，同時也能更早地發現設計上的缺陷。

> 有權力對變更評論「+1」的人都是資深工程師，他們還有許多其他職 責，所以不太可能分神關注初階開發人員所做的修復或生產力。這會導致一個可怕情形。當你在等待變更評閱時，其他開發人員正在提交他們的變更。所以在隨後一週內，你不得不把他們所有的程式碼變更都合併到你自己的筆記本電腦裡，再重新運行所有測試，確保自己所做的一切變更仍然有效，並且有時) 必須重新提交變更，再次等待評閱!

## Ref.

### [The DevOps Handbook](https://www.tenlong.com.tw/products/9786263245440)
> 鑑於以上種種原因，我們應該建立更類似同儕評閱的有效控制實踐，**減少向外部**尋求變更授權的依賴程度。

> 然而，即使是在寬鬆耦合的架構裡，當多個團隊每天進行數百個獨立的部署時，彼此干擾的風險 (例如:同時進行A/B測試) 依然存在。為了降低這些風險，我們可以使用聊天室來發布變更通告，並主動找出可能存在的衝突。

> 進行同儕評閱的適當時機,是將程式碼提交到版本控制系統中的主幹的時候。此 時,變更可能會影響到整個團隊,或者造成全局影響。至少,工程師同僚應該審 核我們的變更,但是對於風險更高的領域來說(例如:資料庫變更,或者在自動 化測試覆蓋率不高的情況下對業務的關鍵組件進行變更),可能就需要領域專家

> 為了解決以上問題，並消除所有延遲，他們最終撤除了整個 Gerrit 程式碼審查流程，然後採用結對程式設計，實現系統裡的程式碼變更,，將程式碼審查所需要的時間，從幾週縮短到了幾小時。

> 另一方面，關於一個足以證明評閱過程有效性的優秀 Pull Request 時，Tomayko 迅速列出了以下基本要素：
> - 必須足夠詳細說明變更的原因
> - 變更如何實施
> - 任何已識別的風險和因應對策

在書中提到的 TEAP-LARB 流程組合，使得在引進新技術時 (e.g. 監控技術、新形態資料庫)，需要歷經長時間的審查流程。以個人觀點，負責審查的委員會實際也只有做到監管職責，無法對於新技術的導入做任何的支援 (有些高高在上的感覺？)
> Mickman 的結論是，如果她的團隊要對所導入的技術自行承擔營運責任，那麼他們其實不需要 TEAP-LARB 這個流程。她補充:「我要讓大家都知道,，未來任何新的技術支援，都不需要通過 TEAP-LARB 流程進行審查。」

### [DevOps tech: Trunk-based development  |  Cloud Architecture Center  |  Google Cloud](https://cloud.google.com/architecture/devops/devops-tech-trunk-based-development)

減少分支數量以及確保每日合併至 main branch 至少一次
> Analysis of DevOps Research and Assessment (DORA) data from 2016 (PDF) and 2017 (PDF) shows that teams achieve higher levels of software delivery and operational performance (delivery speed, stability, and availability) if they follow these practices:
> 1. Have three or fewer active branches in the application's code repository.
> 2. Merge branches to trunk at least once a day.
> 3. Don't have code freezes and don't have integration phases.

改善主幹開發的建議：
1. 以小批量開發：TBD 實踐的關鍵
2. 進行同步代碼審查：確保開發成員優先執行程式碼審查
3. 實施全面的自動化測試：保護分支以僅在所有測試都通過時允許 PR 請求
4. 具有快速的構建：構建和測試過程應該在幾分鐘內完成
5. 建立核心倡導者和指導者小組：TBD 導師/教練

### [DevOps process: Working in small batches  |  Cloud Architecture Center  |  Google Cloud](https://cloud.google.com/architecture/devops/devops-process-working-in-small-batches)

小批量任務的兩個常見陷阱，未將任務細分得足夠小，以及在發送下游進行測試或發布前重新打包工作。任務需要能被切片成能在幾小時內完成的小特性，這也會要求開發人員具有分解任務的經驗/技能
> ### Common pitfalls with working in small batches
> When you break down work into small batches, you encounter two pitfalls:
> - Not breaking up work into small enough pieces. Your first task is to break down the work in a meaningful way. We recommend that you commit code independent of the status of the feature and that individual features take no more than a few days to develop.
Any batch of code that takes longer than a week to complete and check is too big. Throughout the development process, it's essential that you analyze how to break down ideas into increments that you can develop iteratively.
> - Working in small batches but then regrouping the batches before sending them downstream for testing or release. Regrouping work in this way delays the feedback on whether the changes have defects, and whether your users and your organization agree the changes were the right thing to build in the first place.

### [Practical Guide To Implement Trunk-Based Development | Unleash](https://www.getunleash.io/blog/how-to-implement-trunk-based-development-a-practical-guide)

在 TBD 的實踐文章，蠻常會看到儘早執行程式碼審查的建議，若使用 Pair Programming 或是 Mob Programming，是否就能代表於開發過程中融入審查階段？
> ### Use pair programming or mob programming
> In Trunk-Based Development, code review should be done immediately. Moreover, automated testing will not always ensure good quality code, so it is worth using techniques such as pair programming or mob programming to improve team communication and allow real-time code review.
> The benefits of these solutions are great, as they not only affect the quality of the code, but also help integrate the team and enable the valuable exchange of knowledge. The last one is especially important if you have juniors on your team.

### [Continuous Review](https://paulhammant.com/2013/12/05/continuous-review/)

在異動合併至 main branch 前進行程式碼審查，直到審查通過才進行合併
> Thus to Google “Continuous Review” is merely the speedy picking up of unreviewed pending commits. By ‘pending’, I mean that the commits have not hit the trunk yet. Indeed until they ‘pass’ review, they are not going to.

程式碼審查僅在 main branch 上持續進行
> Actually, I’m sure a Trunk-Based Development workflow (regardless of your local-branching model), with reviews of the commits on the trunk only and ASAP is best. “Continuous Review” isn’t my coin by the way, it’s from a comment by Sam Goines, but I like it.

### [Google's vs Facebook's Trunk-Based Development](https://paulhammant.com/2014/01/08/googles-vs-facebooks-trunk-based-development/)

沒有完成審查的程式碼，根本沒有必要合併
> Both Google and Facebook insist on code reviews before the commit is accepted into the remote repo’s trunk for all others to use. There’s no mechanism of code review that’s more efficient or effective.

### [Rapid release at massive scale - Engineering at Meta](https://engineering.fb.com/2017/08/31/web/rapid-release-at-massive-scale/)

對於想 Meta 如此規模的大公司，每日的異動提交多達數百筆，若是人工挑選異動發佈到 release branch 以不太可行
> Engineers would request cherry-picks — changes to the code that had passed a series of automated tests — to pull from the master branch into one of the daily pushes from the release branch. In general, we saw between 500 and 700 cherry-picks per day.
> 

採用 Canary Deployment，讓員工以及客戶當 QA 也是個可行做法，不過這就需要有完成的監控體系能迅速取得異常反饋
> We decided to move facebook.com to a quasi-continuous “push from master” system in April 2016. Over the next year, we gradually rolled it out, first to 50 percent of employees, then from 0.1 percent to 1 percent to 10 percent of production web traffic.
> Each of these progressions allowed us to test the ability of our tools and processes to handle the increased push frequency and get real-world signal. Our main goal was to make sure that the new system made people’s experience better — or at the very least, didn’t make it worse.

> Each release is rolled out to 100 percent of production in a tiered fashion over a few hours, so we can stop the push if we find any problems.
> 

### [Google Engineering Practices Documentation | eng-practices](https://google.github.io/eng-practices/)

PR 應是追求持續性的進步 (Continuous Improvement)，只要這個 PR 能帶來改善就應給予通過，無需追求完美
> In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect.

在 Google 內部的專案，會有 `OWNERS` 檔案來表列這個專案的負責人員，因此理論 PR 是需要請這群 owners 協助審查，或是至少將 CL 同步給 owners
> The best reviewer is the person who will be able to give you the most thorough and correct review for the piece of code you are writing.
> This usually means the owner(s) of the code, who may or may not be the people in the `OWNERS` file. Sometimes this means asking different people to review different parts of the CL.
> If you find an ideal reviewer but they are not available, you should at least CC them on your change.

:::warning
情境探討：
1. 專案沒有 Owners 制度
2. 專案只有一個 Owner，Owner 通常也是發出 PR 的人
:::

### [Code Review Developer Guide | eng-practices](https://google.github.io/eng-practices/review/)

沒撰寫測試的功能，根本沒有必要審查
> Ask for unit, integration, or end-to-end tests as appropriate for the change. In general, tests should be added in the same CL as the production code unless the CL is handling an **emergency**.


~~我們 G 家只會接收精英工程師，絕對不會接受……垃圾~~
若遇到難以閱讀的程式碼，一定是 They 的錯，要請發出 PR 的夥伴協助釐清說明
> If it’s too hard for you to read the code and this is slowing down the review, then you should let the developer know that and wait for them to clarify it before you try to review it. At Google, we hire great software engineers, and you are one of them.


審查者需要提供適當的反饋，尤其是在無法細查 PR 的變更時 (也許是對於業務邏輯的不熟悉)，那麼高階設計、隱私安全等層面審查，並在評論中反饋審查的範圍
> What if it doesn’t make sense for you to review every line? For example, you are one of multiple reviewers on a CL and may be asked:
> - To review only certain files that are part of a larger change.
> - To review only certain aspects of the CL, such as the high-level design, privacy or security implications, etc.
>
> In these cases, note in a comment which parts you reviewed. Prefer giving LGTM with comments .

> [name=Kehao]
> 假設對某個專案並不瞭解其業務邏輯，卻需要擔任程式碼審查員角色，個人會先嘗試對可讀性與可維護性做審查：
> - 可讀性
> - 是否可以從命名瞭解其意圖
> - 是否有足夠恰當的註釋瞭解業務流程規則
> - 可維護性
> - 是否符合 SOLID 原則
> - 相關文件是否有更新 (當然也有可能文件一開始就不存在)

### [Navigating a CL in review | eng-practices](https://google.github.io/eng-practices/review/reviewer/navigate.html)

先找到程式碼變更中的主要部分，通常是具有最多業務邏輯變更的部分。如果 PR 過於龐大導致無法判別何處主要部分，很可能是這個 PR 還需要再切分為多組小批量 PR，或是請 PR 發起者去指出重要部分。
> Find the file or files that are the “main” part of this CL. Often, there is one file that has the largest number of logical changes, and it’s the major piece of the CL. Look at these major parts first.
> This helps give context to all of the smaller parts of the CL, and generally accelerates doing the code review. If the CL is too large for you to figure out which parts are the major parts, ask the developer what you should look at first, or ask them to split up the CL into multiple CLs.

確認整個變更主要設計沒有問題後，可依照程式審查工具的順序來查看每個文件，或是先閱讀測試有助於了解變更的預期行為
> Once you’ve confirmed there are no major design problems with the CL as a whole, try to figure out a logical sequence to look through the files while also making sure you don’t miss reviewing any file.
Usually after you’ve looked through the major files, it’s simplest to just go through each file in the order that the code review tool presents them to you.
Sometimes it’s also helpful to read the tests first before you read the main code, because then you have an idea of what the change is supposed to be doing.

### [Speed of Code Reviews | eng-practices](https://google.github.io/eng-practices/review/reviewer/speed.html)

建議在一個工作日內回應程式碼審查請求
> One business day is the maximum time it should take to respond to a code review request (i.e., first thing the next morning).

適當的程式碼審查時機，可以是午餐過後、會議結束後或是上完洗手間後的工作斷點
> Instead, wait for a break point in your work before you respond to a request for review. This could be when your current coding task is completed, after lunch, returning from a meeting, coming back from the breakroom, etc.

### [Code Review Guidelines | GitLab](https://about.gitlab.com/handbook/engineering/workflow/code-review/)

在 Google 為專案 Owners 的存在，在 GitLab 叫做 Maintainers，一組 PR 雖可以由任何人擔任 Reviewes，但僅有專案 Maintainers 有核准權限
> Note that while all engineers can review all merge requests, the ability to accept merge requests is restricted to maintainers.

Maintainers 需要瞭解專案的業務邏輯與設計才能擔任
> To protect and ensure the quality of the codebase and the product as a whole, people become maintainers only once they have convincingly demonstrated that their reviewing skills are at a comparable level to those of existing maintainers.

> These contributions should be complex enough to give you an understanding of the project's unique domain and design.

不過 GitLab 本身是開源專案，因此審查時間倒是沒有抓得那麼緊
> Leave the merge request open for 1 week, to give the maintainers time to provide feedback to the manager/mentor.

理想 Developers 與 Maintainers 的比例應在 6 以下
> We aim to keep the engineer : maintainer ratio under 6, for both frontend and backend.

原始碼審查預設是分配給領域專家
> Our Code Review Guidelines states that we default to assigning reviews to team members with domain expertise.

定義 Review-response SLO，理想是當 PR 分派給 Reviewers 後的 2 個工作天內，完成審查作業
> The SLO is defined as:
> > Review-response SLO = (time when review is provided) - (time MR is assigned to reviewer)
>
> The SLO value depends on the author of the merge request:
> - From GitLab team members: Review-response SLO < 2 business days
> - From authors of Leading Organizations: Review-response SLO < 4 business days

在 Google 是 `OWNERS` 檔案，在 GitLab 是 `CODEOWNERS` 檔案
> Some GitLab projects use GitLab's `CODEOWNERS` file feature to manage approvals for specific file paths and types. In the gitlab-org/gitlab project, we use a combination of `CODEOWNERS` approval rules plus MR approval settings in order to follow segregation of duties best practices.

### [Code Review Guidelines - Yelp](https://engineeringblog.yelp.com/2017/11/code-review-guidelines.html)

這是一篇舊文章，不過在 Yelp 會為 PR 定義 Primary Reviewer，這算是一種非同步的 Peer Programing 嗎？
> A primary reviewer is responsible for the overall code review. They are as responsible for the final code as the person who wrote it. You should always explicitly have a primary reviewer listed so that everyone knows who has final responsibility.
>
> Don’t ship code without approval from your primary reviewer unless you are experiencing an emergency and your primary reviewer is unreachable.

每一位 Reviewer 會因為自己的專精而有不同的職責，譬如 DBA、資安專家即使不瞭解專案的業務邏輯，仍可以依據自己的專業審查程式碼
> Clearly define the responsibilities of each reviewer To avoid redundancy when multiple people are reviewing a piece of code, clearly define what section(s) each reviewer is responsible for and who will be the primary reviewer.
>
> This allows each person to focus on their area of expertise (in the case of people like DBAs) and keeps discussions manageable.

### [Chromium Docs - Respectful Code Reviews](https://chromium.googlesource.com/chromium/src/+/main/docs/cr_respect.md)

不好的 PR 可能來自於資訊落差導致
> A “bad” CL usually means one of the parties is in possession of information the other one isn’t aware of.

### [What Is Trunk-Based Development?](https://learning.oreilly.com/library/view/what-is-trunk-based/9781098146658/)


書中的 CTBD 是持續將異動直接推送到 main branch 上，藉此觸發測試快速反饋外，也能讓每個成員快速取得其他成員的異動 (需時刻 rebase 保持最新狀態)
> The tests run and pass locally. Jo and Max push their changes to the remote version of the main branch, as shown in the Git history in Figure 2-1, and then alert their colleagues that because code has moved around (into a new file with the extracted class), they should pull and merge the latest version of the main branch as soon as possible.

> After each commit, they pull the latest code from their colleagues, merge it in (with rebase), and run the tests again locally. Once everything passes, they push to the remote repository.

在書中的情境，會希望開發者於本機完成基本測試後 (e.g. 單元測試)，就直接將異動推送到 main branch，需要透過整合測試或是類生產環境才能測試出的問題，是允許在 main branch 上發生，然後儘快完成修復 (然而這也代表 main branch 上的每一個 commit 不是都能正常運作，即使問題立即另一組 commit 修復)
> In the midst of all this, their colleagues have also been pushing code constantly to the remote copy of the main branch, and the integration server has been handling repeated builds, most of which were successful. Today was a difficult day—they weren’t in top form.

作者提到有些人會不認同 CTBD 的做法，在於這些異動合併至 main branch 前，並沒有做好完善的前置作業 (e.g. 整合測試、程式碼審查)。作者提出小量合併能快速識別錯誤，因此也能快速撤銷問題，因此就應儘早合併越好
> People often don’t like the idea of CTBD because they think that when they merge their code back into the main branch, it won’t be ready for integration. But it’s likely that they’re trying to do too much at once.

> The sooner you merge, the faster you’ll discover any resulting issues.

作者認為程式碼審查是在集成後，透過小量變更與自動化測試來降低風險 (這也意味著沒做好自動化測試的團隊，不太能搞 CTBD)
> First, you can still review code as part of CTBD. There are a few ways to do this. You can review after integration. Instead of reviewing specific changesets as PRs encourage you to do, you can revisit whole areas of the code as a team after they’ve received significant changes (or at set intervals). Integrating before review can also mean deploying before review
> That involves a certain amount of risk, but you can lower this risk significantly by using small changes and automated tests.

對於法規標準 (e.g. HIPAA, SOC2, ISO/IEC 27001)，由於並沒有明文寫出合併前需要執行程式碼審查要求 (這些規範不會敘述怎麼實作的細節，如何實作通常是導入的公司資訊/資安單位來制定)，因此設計良好的 CTBD 與 CD pipeline，便能向監管單位解釋如何達到法規要求
> Also, unless you have some regulatory requirement (see the next section), don’t think of code review as a blocking step. Part of the point of CTBD is that you can discover within minutes whether your code works when merged with that of your colleagues.

> In some heavily regulated industries, branches are mandated as a way of enforcing code reviews. But in fact, standards such as HIPAA, SOC2, and ISO/IEC 27001 aren’t incompatible with CTBD. In some cases review is required before deployment but not before merge, and it’s generally not required even before deployment. What’s actually required is two sets of eyes and an audit trail; the goal is to make problems easier to analyze rather than to eradicate them altogether.
> When CTBD and continuous delivery are well managed with properly designed pipelines, a lot of regulators are happier with this way of working once the mechanics of smaller, safer changes are explained to them, as Dave Farley attests: “My experience, across the board, has been that regulators prefer these approaches, once they come to understand them, because they provide a better quality experience all around.”

> [name=Kehao]
> 以個人感想書中提出的 CTBD 過於激進，也需要良好的測試案例/涵蓋率去支撐
>> To work, CTBD needs an automated pipeline and a strong automated test suite, which may not exist on a legacy codebase. Also, legacy code may have a lot of tight coupling and dependencies, which is likely to slow down your builds and your tests. Chapter 3 describes how you can move toward CTBD while working with a legacy system that was not designed for it.

> [name=Kehao]
個人可能還是比較偏好 short-lived branches 的做法，並且每日至少合併至 main branch 一次，不過作者是反 branch 派的
>> Whether to use short-lived branches is a topic of debate among trunk-based development practitioners. Paul Hammant presents an idea called scaled TBD, in which larger teams use short-lived feature branches. The authors of Accelerate do not reject this approach: “We agree that working on short-lived branches that are merged into trunk at least daily is consistent with commonly accepted continuous integration practices.”3 In contrast, Dave Farley argues that “any form of branching runs counter, in principle, to the ideas of Continuous Integration,” and he insists, “Don’t branch! Don’t branch! Don’t branch!”

> [name=Kehao]
> 個人認為 CTBD 與個人認知最大的差異，在於合併至 main branch 前，是否需要完成自動化測試與程式碼審查。CTBD 雖然可以降低頻繁程式碼審查的認知負擔，但也提到了風險與延遲審查者的反饋，這是需要團隊去作出取捨的