6.1 KiB

Исходник Постоянная ссылка Ответственный История

Disable Squash and Merge

Status: Accepted
Deciders: Lauren Zugai, Ivo Planemac, Barry Chen, Wil Clouser, Vijay Budhram
Date: 2022-03-25

Technical Story: #11721

Context and Problem Statement

In late September 2021, we enabled "squash and merge" for pull requests through the GitHub UI for optional engineering convenience. However, enabling this option caused several issues to surface. Through team discussions, we have chosen to turn the option off (for now). This document, while not an architecture decision, serves to capture a summary of the experiment and problems around squash and merge with our current git workflow. Changing development or release strategies is out of scope for this ADR, but those conversations are ongoing.

FxA's commit guidelines standardize our commit formatting, and we prefer one commit per issue or PR closed. These preferences help us maintain a clean git and changelog history - not only does it serve as documentation when needing to git blame a line and help reviewers understand the breadth of changes in PR review, but one commit per task can help us debug problems that arise after merging and also makes cherry-picking for dot releases easier. Because of these preferences, engineers sometimes need to run an interactive rebase, squash commits into one, and force push branches if they've been previously pushed with multiple commits, or if changes are requested on PR. Squashing and merging through GH helps mitigate the need to force push through the terminal at the final review step because engineers can squash commits, edit the commit message, and merge the PR in GH.

GitHub sets user's default merge strategy to whatever they used last (squash and merge or regular merge) and doesn't allow disabling the option on certain branches. These caveats, alongside human error and automated dependency upgrades, have caused a number of problems we've identified:

Accidentally squashing and merging train branches into main causes merge conflicts for the next patch release because of differing parent commits
Accidentally forgetting to edit the commit message when squashing and merging an approved PR means PRs can be merged without meeting our commit guidelines
Accidentally regular merging a branch that was meant to be squashed and merged means multiple unformatted commits can land for a single task
While squashing and merging Dependabot (package.json) and Bananafox (yarn.lock) commits on automated dependency PRs seemed like a nice pro for keeping squash and merge, there's been a couple of instances where something funky happened with a bad squash and merge dependency update which caused Bananafox to get out of sync and commit on multiple PRs without package.json changes. This caused an issue with a dot release that could have been mitigated by being able to cherry-pick in only the needed change without the yarn.lock commit, see this PR and and others around this time, and so it seems more ideal to keep these in separate commits until Dependabot works properly with yarn 2 so we can retire Bananafox

Considered Options

Continue allowing squash and merge through GH
Disable squash and merge

Decision Outcome

Chosen option: "option 2", disable squash and merge, because with our current workflow, team discussions have concluded there doesn't appear to be adequate workarounds to alleviate the problems listed above while leaving squash and merge enabled. We will also disable force pushing on train-* branches, and keep our rule to never force push to main. This is not a final decision; we will continue to have conversations around development strategy process improvements and viability of squash and merge in the future.

Positive Consequences

Train branches will no longer be accidentally squashed and merged, reducing merge conflicts and burden on release owners
While there is an extra step to squashing commits locally, pushing, and regular merging through GH rather than doing it all at once with a squash and merge button in GH, this extra step allows us to see exactly what's going to be merged when we click "merge." The 2nd and 3rd problems laid out above should be alleviated
Commit messages can be reviewed as part of the PR review. We don't need to have a hard rule for this, but it's oftentimes nicer reviewing a PR that has the type/scope/summary and commit message body filled out already, rather than r+'ing a PR with a bunch of "WIP" commits and trusting that it will be squashed into one commit that follows guidelines once the PR is merged
Disabling force pushing on train branches can help alleviate some of the risk around allowing force pushing, since train-* branches are shared
Better preservation of commit signing integrity. When we squash and merge through GH, GH signs the commit for us

Negative Consequences

Squashing and merging through GH can be convenient for engineers
Interactive rebase to squash and then force push a branch can feel scary since it overrides history
There may be a rare circumstance where we may want to push

While force pushing is not risk-free, there are ways to soften any risk of loss of work. Engineers can push to their fork before squashing locally to preserve a copy of history at that point in time, run a git diff against their locally squashed branch and what's been pushed, and/or use the --force-with-lease option when pushing. We also have branch protection against force pushing on main, and can add this protection to train branches (train-*). Force pushing on any other branch that has more than one person using it, e.g. when pairing, should be done with a lot of care and coordination and is safest with --force-with-lease.

6.1 KiB Исходник Постоянная ссылка Ответственный История