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

build: stop breaking master (a.k.a. implement Bors) #22499

Closed
couchand opened this Issue Feb 8, 2018 · 16 comments

Comments

Projects
None yet
7 participants
@couchand
Member

couchand commented Feb 8, 2018

Let's stop breaking master.

The Not Rocket Science Rule Of Software Engineering (source)

automatically maintain a repository of code that always passes all the tests

Why

Breaking master is a bad thing:

  • People should be able to reliably build a fresh pull from master.
  • Rebasing onto the latest master is the right thing to do, this should be reliable.
  • Borked commits in our master history break git bisect.
  • Noise in our build notifications can hide real problems that come up.

(Also:) tiny changes should have a merge-on-green option so you don't have to sit there waiting for TC to complete.

What

Bors-NG.

  • A continuous integration bot.
  • Awaits reviewer approval on PRs, builds them as though they were merged, only merges on green.
  • Updates PR with merge status.

How

  • 🏗 Setup TeamCity to build Bors' staging and trying branches
  • 🤖 Set up hosting for Bors
    • :octocat: Create GitHub app for our self-hosted Bors
    • 🚢 Deploy container to GCE
    • 📇 Setup DNS
    • 🔐 Switch to use HTTPS
  • 📄 Add a bors.toml file to the each branch:
  • ☑️ Validate that everything's working as expected
    • 🤕 Try to merge a broken PR (#24294)
    • 🎋 Try to merge something to a target branch other than master (#24298)
    • ⛔️ Try to merge a PR with the label do-not-merge (#24100)
  • 📖 Document Bors and the new workflow (#24292)
  • 👨‍👧‍👦 Add every user as a reviewer
  • ℹ️ Inform the team about the new workflow
  • 🚯 Disable manual merges to each branch
    • 🚳 master
    • 🔕 release-2.0
    • 🚫 release-1.1
    • 🙅‍♂️ release-1.0
  • 💸 Profit

Open Questions

  • Release branches?

On the one hand, cherry-picks to them are infrequent enough that it's not as critical. On the other hand, many of the same considerations apply as much or more to release branches, particularly right before picking a release candidate. Let's punt it for now, but it's probably a good idea to do as a next step.

We get them out of the box, they will require two extra steps per branch: cherry-picking #24100 to each one and then disabling of manual merges - see #22499 (comment).

  • Flaky tests?

This won't fix those, but it surely won't make them worse. Perhaps it will even give people more of an incentive to root out any issues with flakiness.

@couchand couchand added the C-cleanup label Feb 8, 2018

@couchand couchand self-assigned this Feb 8, 2018

@couchand

This comment has been minimized.

Member

couchand commented Feb 8, 2018

@jordanlewis

This comment has been minimized.

Member

jordanlewis commented Feb 8, 2018

Nice write up! I'm all for trying this out.

One (relatively minor) downside that I think should be mentioned is that average time-to-merge may actually go up as a result of this strategy, because each PR will have to be built against the most recent master. Let's say I've submitted a small PR. The reviewer waits until CI is green to review (cycle 1), reviews, and hits the LGTM button soon after. In the intervening moments, let's say another PR got merged to master. At that point, I will have to ask the bot to rebuild the branch against master if it's changed (cycle 2).

I'm not trying to say this delay is worse than occasionally broken master, but just wanted to point it out for the record!

Could you elaborate on your first point - "Setup TeamCity to build staging and trying branches"? I'm not sure what that means, and I think it might already be done.

@bdarnell

This comment has been minimized.

Member

bdarnell commented Feb 8, 2018

You've got a couple of steps out of order: you have to inform the team about the new workflow before disabling manual merges.

Also, as to timing: Don't change anything before 2.0 is released. We don't want to disrupt everyone's workflow at this point in the development cycle.

A lot of our recent master breaks have come from the fact that we don't run the full test suite in race mode on PRs. Just turning that back on will improve the situation without changing workflow (although one advantage of the move to bors would be that by moving when the CI build happens relative to the decision to merge, we don't have to be as sensitive to the time it takes to build)

@couchand

This comment has been minimized.

Member

couchand commented Feb 8, 2018

@jordanlewis I believe your concern about delay is mitigated by Bors' batching strategy. All the PRs approved in the past x minutes get merged together onto the staging branch. If the batch fails, Bors bisects the various PRs to identify which ones can cleanly merge and gets those on. (I'm not sure I understand your point about waiting until CI is green to review, perhaps we should chat in person).

@bdarnell ack on the wait for next week. I had expected those last two steps would happen roughly simultaneously, but it's probably worth reordering them. Done.

@couchand

This comment has been minimized.

Member

couchand commented Feb 8, 2018

Oh, and @jordanlewis yes, I expect TeamCity is already building the bors branches by default, but wanted to include it here to make sure that when I went to start checking things off I make sure to validate that's the case.

@jordanlewis

This comment has been minimized.

Member

jordanlewis commented Feb 8, 2018

I'm not sure I understand your point about waiting until CI is green to review

Maybe I'm the only one but if I get to a review and it's got a failed build that doesn't look like a flake, I will sometimes not review it until later. But yeah I suppose I was exaggerating there - I don't think people typically wait for a full cycle to complete.

I expect TeamCity is already building the bors branches by default

If bors keeps its own branches that it expects to get built, that aren't PR branches, then we will in fact need to configure TC to do something different. Thanks for clarifying!

@bdarnell

This comment has been minimized.

Member

bdarnell commented Feb 8, 2018

ack on the wait for next week.

Wait for the release of 2.0, not just the cutting of the branch.

@notriddle

This comment has been minimized.

@couchand

This comment has been minimized.

Member

couchand commented Mar 21, 2018

I'm looking at this again with the hope of getting it going as soon as we can. @bdarnell what do you think about going through the first four steps of the process before the release date? I'm ready to go with those, and as far as I can tell those steps won't impact anyone else.

couchand added a commit to couchand/cockroach that referenced this issue Mar 21, 2018

build: add bors.toml
Contributes to cockroachdb#22499
Release note (build change): Begin using Bors to automate merging PRs
to master.

couchand added a commit to couchand/cockroach that referenced this issue Mar 21, 2018

build: add bors.toml
Teaches Bors to wait for a successful build on TeamCity before merging
approved PRs to master.

Contributes to cockroachdb#22499
Release note (build change): Begin using Bors to automate merging PRs
to master.
@benesch

This comment has been minimized.

Member

benesch commented Mar 21, 2018

@couchand

This comment has been minimized.

Member

couchand commented Mar 21, 2018

I've noticed two open Bors issues that mildly affect usability:

  • bors-ng/bors-ng#165, which suggests supporting teams as reviewers. without it, we'll be manually adding folks to the reviewers list.
  • bors-ng/bors-ng#163, which observes that it's not sufficient for issue numbers to be in the commit message, they must be in the PR description to be closed when Bors merges the PR.
@bdarnell

This comment has been minimized.

Member

bdarnell commented Mar 21, 2018

Yeah, you can go ahead and start working on this.

@couchand couchand added this to the 2.1 milestone Mar 21, 2018

couchand added a commit to couchand/cockroach that referenced this issue Mar 26, 2018

build: add bors.toml
Teaches Bors to wait for a successful build on TeamCity before merging
approved PRs to master, and not to merge PRs labeled do-not-merge.

Contributes to cockroachdb#22499
Release note (build change): Begin using Bors to automate the process of
merging PRs to master.

craig bot added a commit that referenced this issue Mar 27, 2018

Merge #24100
24100: build: add bors.toml r=benesch a=couchand

Teaches Bors to wait for a successful build on TeamCity before merging approved PRs to master.

Contributes to #22499
Release note (build change): Begin using Bors to automate merging PRs to master.

craig bot added a commit that referenced this issue Mar 27, 2018

Merge #24100
24100: build: add bors.toml r=benesch a=couchand

Teaches Bors to wait for a successful build on TeamCity before merging approved PRs to master.

Contributes to #22499
Release note (build change): Begin using Bors to automate merging PRs to master.

craig bot added a commit that referenced this issue Mar 27, 2018

Merge #24100
24100: build: add bors.toml r=jordanlewis a=couchand

Teaches Bors to wait for a successful build on TeamCity before merging approved PRs to master.

Contributes to #22499
Release note (build change): Begin using Bors to automate merging PRs to master.

craig bot added a commit that referenced this issue Mar 27, 2018

Merge #24100
24100: build: add bors.toml r=couchand a=couchand

Teaches Bors to wait for a successful build on TeamCity before merging approved PRs to master.

Contributes to #22499
Release note (build change): Begin using Bors to automate merging PRs to master.
@benesch

This comment has been minimized.

Member

benesch commented Mar 27, 2018

Woohoo! Bors has merged its first PR!

couchand added a commit to couchand/cockroach that referenced this issue Mar 28, 2018

docs: add mention of bors to CONTRIBUTING.md
As of cockroachdb#24100 we can use Bors to merge PRs to master, and once
cockroachdb#22499 is complete that will be the standard practice.  This
adds documentation to make this clear.

Contributes to cockroachdb#22499
Release note: None
@vilterp

This comment has been minimized.

Contributor

vilterp commented Mar 28, 2018

I think people understand why breaking master is bad; can you explain more about how bors prevents merge skew? (which is what usually breaks master)

Definitely excited for this if it fixes merge skew!

@tbg

This comment has been minimized.

Member

tbg commented Mar 28, 2018

Bors prevents merge skew because it tests the results of the merge and not the HEAD of the PR branch. Or rather, bors prevents merge skew which results in test failures. If you have merge skew that has everything pass, nobody can detect it 🙈 But this is very rare.

Bors serializes all merges and does so with some optimizations to save on CI runs (roll-ups of multiple changes into one merge, etc).

couchand added a commit to couchand/cockroach that referenced this issue Mar 28, 2018

craig bot added a commit that referenced this issue Mar 28, 2018

Merge #24298
24298: build: testing bors merging to an alternate branch r=couchand a=couchand

Testing that Bors will merge correctly to the right branch.

Contributes to: #22499
Release note: None

couchand added a commit to couchand/cockroach that referenced this issue Mar 28, 2018

docs: add mention of bors to contributing docs
As of cockroachdb#24100 we can use Bors to merge PRs to master, and once cockroachdb#22499 is
complete that will be the standard practice.  This adds documentation to make
the new procedure clear.

Contributes to cockroachdb#22499
Release note: None

couchand added a commit to couchand/cockroach that referenced this issue Mar 29, 2018

docs: add mention of bors to contributing docs
As of cockroachdb#24100 we can use Bors to merge PRs to master, and once cockroachdb#22499 is
complete that will be the standard practice.  This adds documentation to make
the new procedure clear.

Contributes to cockroachdb#22499
Release note: None

couchand added a commit to couchand/cockroach that referenced this issue Mar 29, 2018

build: add bors.toml
Teaches Bors to wait for a successful build on TeamCity before merging
approved PRs to master, and not to merge PRs labeled do-not-merge.

Contributes to cockroachdb#22499
Release note (build change): Begin using Bors to automate the process of
merging PRs to master.

couchand added a commit to couchand/cockroach that referenced this issue Mar 29, 2018

build: add bors.toml
Teaches Bors to wait for a successful build on TeamCity before merging
approved PRs to master, and not to merge PRs labeled do-not-merge.

Contributes to cockroachdb#22499
Release note (build change): Begin using Bors to automate the process of
merging PRs to master.

couchand added a commit to couchand/cockroach that referenced this issue Mar 29, 2018

build: add bors.toml
Teaches Bors to wait for a successful build on TeamCity before merging
approved PRs to master, and not to merge PRs labeled do-not-merge.

Contributes to cockroachdb#22499
Release note (build change): Begin using Bors to automate the process of
merging PRs to master.

craig bot added a commit that referenced this issue Mar 29, 2018

Merge #24323
24323: cherrypick-2.0: build: add bors.toml r=bdarnell a=couchand

A cherry-pick of #24100.

Teaches Bors to wait for a successful build on TeamCity before merging
approved PRs to master, and not to merge PRs labeled do-not-merge.

Contributes to #22499
Release note (build change): Begin using Bors to automate the process of
merging PRs to master.

cc @cockroachdb/release @bdarnell @benesch

craig bot added a commit that referenced this issue Mar 29, 2018

Merge #24324
24324: cherrypick-1.1: build: add bors.toml r=bdarnell a=couchand

A cherry-pick of #24100.

Teaches Bors to wait for a successful build on TeamCity before merging
approved PRs to master, and not to merge PRs labeled do-not-merge.

Contributes to #22499
Release note (build change): Begin using Bors to automate the process of
merging PRs to master.

cc @cockroachdb/release @bdarnell @benesch

craig bot added a commit that referenced this issue Mar 29, 2018

Merge #24325
24325: cherrypick-1.0: build: add bors.toml r=bdarnell a=couchand

A cherry-pick of #24100.

Teaches Bors to wait for a successful build on TeamCity before merging
approved PRs to master, and not to merge PRs labeled do-not-merge.

Contributes to #22499
Release note (build change): Begin using Bors to automate the process of
merging PRs to master.

cc @cockroachdb/release @bdarnell @benesch

craig bot added a commit that referenced this issue Mar 29, 2018

Merge #24325
24325: cherrypick-1.0: build: add bors.toml r=bdarnell a=couchand

A cherry-pick of #24100.

Teaches Bors to wait for a successful build on TeamCity before merging
approved PRs to master, and not to merge PRs labeled do-not-merge.

Contributes to #22499
Release note (build change): Begin using Bors to automate the process of
merging PRs to master.

cc @cockroachdb/release @bdarnell @benesch

craig bot added a commit that referenced this issue Mar 29, 2018

Merge #24325
24325: cherrypick-1.0: build: add bors.toml r=bdarnell a=couchand

A cherry-pick of #24100.

Teaches Bors to wait for a successful build on TeamCity before merging
approved PRs to master, and not to merge PRs labeled do-not-merge.

Contributes to #22499
Release note (build change): Begin using Bors to automate the process of
merging PRs to master.

cc @cockroachdb/release @bdarnell @benesch

knz added a commit to couchand/cockroach that referenced this issue Apr 2, 2018

docs: add mention of bors to contributing docs
As of cockroachdb#24100 we can use Bors to merge PRs to master, and once cockroachdb#22499 is
complete that will be the standard practice.  This adds documentation to make
the new procedure clear.

Contributes to cockroachdb#22499
Release note: None

craig bot added a commit that referenced this issue Apr 2, 2018

Merge #24145 #24292 #24410 #24420
24145: server: embed the pprof ui r=couchand a=tschottdorf

Now that pprof has a web ui, we can just embed it into the running
server directly.

It turns out that for the most part, pprof has the right abstractions
built in, and so the code for this came out fairly clean. I needed a
few upstream modifications which are currently pending; it's looking
good that they'll be merged:

https://github.com/google/pprof/pulls/tschottdorf

Release note: None

24292: docs: add mention of bors to CONTRIBUTING.md r=couchand a=couchand

As of #24100 we can use Bors to merge PRs to master, and once
#22499 is complete that will be the standard practice.  This
adds documentation to make this clear.

Contributes to #22499
Release note: None

24410: opt: generate constraints for var IN tuple, tuple inequalities r=RaduBerinde a=RaduBerinde

With these changes, the constraints we have so far should go a long way when we start pushing them across joins.

#### opt: generate constraints for var IN tuple

Release note: None

#### opt: generate constraints from tuple inequalities

Release note: None


24420: build: teach Bors about the CLA check r=couchand a=couchand

When introducing Bors in #24100, I only thought to include the TeamCity build
status, but since we require the license check for regular, GitHub-initiated
merges, this was a regression.  This change corrects that, adding the CLA to
the list of statuses to check.

Release note: None

knz added a commit to couchand/cockroach that referenced this issue Apr 2, 2018

docs: add mention of bors to contributing docs
As of cockroachdb#24100 we can use Bors to merge PRs to master, and once cockroachdb#22499 is
complete that will be the standard practice.  This adds documentation to make
the new procedure clear.

Contributes to cockroachdb#22499
Release note: None

craig bot added a commit that referenced this issue Apr 2, 2018

Merge #24145 #24292 #24410 #24420
24145: server: embed the pprof ui r=couchand a=tschottdorf

Now that pprof has a web ui, we can just embed it into the running
server directly.

It turns out that for the most part, pprof has the right abstractions
built in, and so the code for this came out fairly clean. I needed a
few upstream modifications which are currently pending; it's looking
good that they'll be merged:

https://github.com/google/pprof/pulls/tschottdorf

Release note: None

24292: docs: add mention of bors to CONTRIBUTING.md r=couchand a=couchand

As of #24100 we can use Bors to merge PRs to master, and once
#22499 is complete that will be the standard practice.  This
adds documentation to make this clear.

Contributes to #22499
Release note: None

24410: opt: generate constraints for var IN tuple, tuple inequalities r=RaduBerinde a=RaduBerinde

With these changes, the constraints we have so far should go a long way when we start pushing them across joins.

#### opt: generate constraints for var IN tuple

Release note: None

#### opt: generate constraints from tuple inequalities

Release note: None


24420: build: teach Bors about the CLA check r=couchand a=couchand

When introducing Bors in #24100, I only thought to include the TeamCity build
status, but since we require the license check for regular, GitHub-initiated
merges, this was a regression.  This change corrects that, adding the CLA to
the list of statuses to check.

Release note: None
@couchand

This comment has been minimized.

Member

couchand commented Apr 11, 2018

Hey, it's all done. Hooray! 🎉

@couchand couchand closed this Apr 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment