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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wait for CI to finish instead of rejecting immediately on bors r+ when CI is still "pending" #785

Merged
merged 23 commits into from Dec 11, 2019

Conversation

@zli-simspace
Copy link
Contributor

zli-simspace commented Oct 29, 2019

Hi!

Our company uses Bors and I've made these changes (for our own use) that we thought could also be useful upstream.

Problem

This issue describes the motivation for this PR pretty well:

#430

The gist of it that we want to mark a PR as ready to merge before CI has finished building and instead of an immediate

馃憥 Rejected by PR status

we'd like Bors to wait until CI is finished.

Solution

We distinguish between Github statuses that are errors and waiting differently (instead of treating them all as a failed passed_status. In the case where there are no errors and some waiting statuses, notify with a response and wait polling with an exponential backoff until we're in some other state than "no errors and some waiting statuses" (i.e., patch_preflight returns something else).

Once that happens, report back and take action as Bors did before.

Waiting on Github's waiting status can timeout, which is considered a failure and Bors won't merge the PR. This a timeout can be set in bors.toml as prerun_timeout_sec. The default is 30 minutes.

Tests

I've added two tests for the new functionality.

Relevant issues

#430

Though #430 was closed in favour of #390, a timeout makes much more sense for #430. Our specific pain point does not relate to Github approvals and was really about the issue with

  • waiting for CI (an automatic process),
  • coming back to type bors r+ (manual) and then
  • have CI build the staging branch (another automatic process.
zli-simspace added 20 commits Aug 14, 2019
Avoids modifying the Ecto Database.Repo directly as much as possible
and instead use the webhook to simulate notifications from "GitHub".

Uncleaned first version.
鈥us.

Still need cleanup and parametrization.
Wait for CI to finish instead of rejecting immediately on bors r+ when CI is still "pending"
passed_status is now a combination of no_error_status and no_waiting_status
Playing it safe. Not sure where it belongs or in what form.
Copy link
Member

notriddle left a comment

This mostly looks pretty good. I'll try running a description of your change across the peanut gallery to see what they think, but mostly the idea seems pretty solid.

lib/web/controllers/project_controller.ex Outdated Show resolved Hide resolved
@zli-simspace

This comment has been minimized.

Copy link
Contributor Author

zli-simspace commented Oct 29, 2019

Great, thanks for the quick initial response on this PR!

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Oct 29, 2019

Okay, here's the RFC with all the draft documentation for this. If it's wrong, you should be able to log into the forum (with your github account) to edit it.

@zli-simspace zli-simspace mentioned this pull request Oct 30, 2019
bors bot added a commit that referenced this pull request Nov 22, 2019
782: Bump terser-webpack-plugin from 2.1.3 to 2.2.1 in /assets r=notriddle a=dependabot-preview[bot]

Bumps [terser-webpack-plugin](https://github.com/webpack-contrib/terser-webpack-plugin) from 2.1.3 to 2.2.1.


786: Add FriendlyMock r=notriddle a=zli-simspace

As discussed in #785, this PR adds `FriendlyMock`, a "friendlier" version of ServerMock for developer convenience.

It is intended to be called from the REPL iex. I had trouble getting the nested maps on ServerMock in the right state otherwise.

From FriendlyMock's moduledoc:

>  Helper functions for ServerMock for common operations without having to
>  modify state by hand.
>
>  Tries to lookup values instead of requiring full %{} associative arrays.
>
>  Assumes a single GitHub instance with a single repository and single user.
>
>  Does everything through webhook notifications. Does not use
>  Database.Repo.insert directly! (One exception: adding a reviewer,
>  which is normally done through Bors' web UI.)


Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: zli <zli@simspace.com>
Copy link
Contributor

bbuchalter left a comment

My team at Gusto has just adopted bors and would love to see this feature land.

My suggestions here are mostly cosmetic, but might improve bors usability for both developers and end-users.

lib/worker/batcher.ex Outdated Show resolved Hide resolved
lib/worker/batcher.ex Outdated Show resolved Hide resolved
@@ -15,6 +15,9 @@ defmodule BorsNG.Worker.Batcher.BorsToml do

defstruct status: [], block_labels: [], pr_status: [],
timeout_sec: (60 * 60),
# Half an hour. Give up when more time is spent waiting for this amount
# between two successive polls.

This comment has been minimized.

Copy link
@bbuchalter

bbuchalter Dec 5, 2019

Contributor

This comment is confusing to me. Based on my reading of the PR, maybe this would be better?

prerun_timeout_sec controls how long bors will wait for all GitHub status checks to be completed before taking action. If this value is set to 0, bors will not wait for status checks to be completed. If all status checks are not completed within prerun_timeout_sec, bors will return an error message.

This comment has been minimized.

Copy link
@zli-simspace

zli-simspace Dec 9, 2019

Author Contributor

Yeah, I think your suggestion is good.

If all status checks are not completed within prerun_timeout_sec,

The only thing to note is that I was lazy and the actual timeout is proportional to prerun_timeout_sec but isn't prerun_timeout_sec. prerun_timeout_sec is the amount the last exponential backoff needed to wait.

You can get one timeout from the other by calculating the difference of two geometric series (or the inverse of that).

It was done out of laziness and to have fewer moving parts. But feel free to suggest an improvement to that as well (ideally one that isn't too complicated).

This comment has been minimized.

Copy link
@bbuchalter

bbuchalter Dec 9, 2019

Contributor

Lazy is fine, just so long as it's clear. I think we just need to explain it with words so folks understand their configuration choices.

@@ -28,6 +28,15 @@ defmodule BorsNG.Worker.Batcher.Message do
{"Delayed for higher-priority pull requests", :running}
end

def generate_message({:preflight, :waiting}) do
":clock1: Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set."

This comment has been minimized.

Copy link
@bbuchalter

bbuchalter Dec 5, 2019

Contributor

Two changes I'd like to see here:

  • Let's use GitHub's language: they call these things status checks
  • It'd be great if this message explained how long Bors is configured to wait until erroring. We'd need access to prerun_timeout_sec - not sure if we can get that here easily.
    ":clock1: Bors is waiting for all GitHub status checks to complete and will continue polling until $TIME_IN_FUTURE
lib/worker/batcher/message.ex Outdated Show resolved Hide resolved
@zli-simspace

This comment has been minimized.

Copy link
Contributor Author

zli-simspace commented Dec 9, 2019

@bbuchalter thanks for your improvements! They look good to me.

Applied suggestions from code review

Co-Authored-By: Brian Buchalter <brian.buchalter@gusto.com>
@dchaley

This comment has been minimized.

Copy link

dchaley commented Dec 9, 2019

to this being a very useful PR that captures the promise of a smart, deferred merge queue. Anything blocking merge?

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Dec 9, 2019

Nothing's really blocking it at this point, no.

@zli-simspace

This comment has been minimized.

Copy link
Contributor Author

zli-simspace commented Dec 9, 2019

Oh, I thought I had to wait for the RFC to close. Should I be r+ing it myself?

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Dec 9, 2019

I was waiting for the RFC to close, yes. It'll literally be closed in a day, and nobody's said anything, so I'll probably merge it then.

@phanle

This comment has been minimized.

Copy link
Contributor

phanle commented Dec 11, 2019

Can't wait to see this feature landed. This would reduce greatly context switches for our team at Gusto!

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Dec 11, 2019

bors r+

bors bot added a commit that referenced this pull request Dec 11, 2019
Merge #785
785: Wait for CI to finish instead of rejecting immediately on bors r+ when CI is still "pending" r=notriddle a=zli-simspace

Hi!

Our company uses Bors and I've made these changes (for our own use) that we thought could also be useful upstream.

## Problem

This issue describes the motivation for this PR pretty well:

#430

The gist of it that we want to mark a PR as ready to merge before CI has finished building and instead of an immediate

> 馃憥 Rejected by PR status

we'd like Bors to wait until CI is finished.

## Solution

We distinguish between Github statuses that are errors and waiting differently (instead of treating them all as a failed `passed_status`. In the case where there are no errors and some waiting statuses, notify with a response and wait polling with an exponential backoff until we're in some other state than "no errors and some waiting statuses" (i.e., `patch_preflight` returns something else).

Once that happens, report back and take action as Bors did before.

Waiting on Github's waiting status can timeout, which is considered a failure and Bors won't merge the PR. This a timeout can be set in `bors.toml` as `prerun_timeout_sec`. The default is 30 minutes.

## Tests

I've added two tests for the new functionality.

## Relevant issues

#430

Though #430 was closed in favour of #390, a timeout makes much more sense for #430. Our specific pain point does not relate to Github approvals and was really about the issue with

- waiting for CI (an automatic process),
- coming back to type `bors r+` (manual) and then
- have CI build the `staging` branch (another automatic process.


Co-authored-by: zli <zli@simspace.com>
Co-authored-by: zli-simspace <46386524+zli-simspace@users.noreply.github.com>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Dec 11, 2019

Build succeeded

@bors bors bot merged commit eca5d35 into bors-ng:master Dec 11, 2019
3 checks passed
3 checks passed
Community-TC (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
bors Build succeeded
Details
notriddle added a commit to bors-ng/bors-ng.github.io that referenced this pull request Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.