Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure master only receives commits that have successfully build/tested on top of master #7130

Open
BigLep opened this issue Aug 18, 2021 · 3 comments
Labels
kind/enhancement Kind: Enhancement

Comments

@BigLep
Copy link
Member

BigLep commented Aug 18, 2021

Current State:

We have seen examples like this:

  1. PR is old and CI passed at some point
  2. PR is merged (meaning the PR commits are added on top of master)
  3. Due to other changes that had slipped in since CI ran, code no longer builds.

Our current suggestion to rebase master into the PR manually before merging, verifying build/tests pass, and then merge.

This is different than:

image

because that check requires PRs to be completely up-to-date with master and doesn't allow any discretion. The issue with requiring that every branch to become completely up to date is that we need to go around either manually rebasing, or clicking the "update branch" button every time a PR is merged. Merging 3 PRs means merging the first, waiting for CI to re-run on the second, merging it, waiting for CI to re-run on the third, merging it.

Proposal

The staging idea was basically a more automatic "require branches to be up to date". The idea was to let a staging branch test PRs against the latest master (and possibly run more extensive/slow tests). When tests pass on staging, master would be fast-forwarded to the last commit on staging with passing tests. If tests fail, automation would need to roll back to the last passing merge and re-build staging.

@BigLep BigLep added the kind/enhancement Kind: Enhancement label Aug 18, 2021
@Stebalien
Copy link
Member

Specifically,

  1. As merges in the staging branch pass CI (on the staging branch), master gets fast-forwarded to the most recent passing merge on staging.
  2. If a merge fails, staging is rebased on master, dropping the failing merge. The PR that failed the merge is then re-opened.

Qs:

  • Does that rebase workflow work? We could also re-do the merges from the automation.
  • Is CI run on every commit? I think so?
  • How confused will users be if they need to make PRs against staging?
  • Will this be annoying?

Prior art:

  • Rust does this with a bot and something they call "rollups". Instead of a staging branch, the bot will "roll up" a series of PRs into a batch and test the batch all at once. However, this doesn't quite fit with the standard "merge" workflow, and it requires even more tooling.

Closing thoughts:

@raulk raulk changed the title Master receives commits that have successfully build/tested on top of master Ensure master receives commits that have successfully build/tested on top of master Aug 19, 2021
@raulk raulk changed the title Ensure master receives commits that have successfully build/tested on top of master Ensure master only receives commits that have successfully build/tested on top of master Aug 19, 2021
@raulk
Copy link
Member

raulk commented Aug 19, 2021

  1. An automation that writes to master directly makes me very, very nervous. I think there are ways to achieve this without taking that risk.
  2. The difficulty of implementing this method lies in the ordering and queueing of branches that are flagged for merge into staging. There is a clear head of line blocking problem that machines can't solve.
  3. I'm game for automation, but there is other low-hanging low-effort fruit to focus on to achieve this very same goal, before we unleash creativity.
  4. For example, how about having an automation that merges master into every open (non-draft) PR every time that master moves?
    • This may lead to some inbox chattiness, but that's an incentive to keep the PR queue clear, and/or mark PRs that aren't ready as drafts.
  5. Alternatively, a simple automation that fails a "level with master" check on all open (non-draft) PRs as soon as master moves, and gets reset when a dev merges master into the branch? This would serve like a traffic light.

P.S.: I updated the issue title to be action-oriented.

@Stebalien
Copy link
Member

An automation that writes to master directly makes me very, very nervous. I think there are ways to achieve this without taking that risk.

Staging is effectively master-"next". The automation would just fast-forward.

The difficulty of implementing this method lies in the ordering and queueing of branches that are flagged for merge into staging. There is a clear head of line blocking problem that machines can't solve.

PRs would be directly merged into staging. If something fails, we'd back out and re-merge everything else. We can also test merges in parallel, so there's no real blocking.

For example, how about having an automation that merges master into every open (non-draft) PR every time that master moves?
...
Alternatively, a simple automation that fails a "level with master" check on all open (non-draft) PRs as soon as master moves, and gets reset when a dev merges master into the branch? This would serve like a traffic light.

GitHub already has this ("require PRs to be up-to-date" and a "update branch button"). We tried it, and disabled it, because it makes merging a sequence of PRs painful (many round-trips). This is, in fact the motivation for this issue (see the main issue body).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Kind: Enhancement
Projects
None yet
Development

No branches or pull requests

3 participants