Hi,
Following-up on:
I’d like to re-enable partially the checks that Tom added and that were reverted.
Since there was some issues with the code-formatter and the “changes requests” part of the checks last time, I’d like to leave them out and focus on the pre-merge testing in this RFC (see below).
Goal
The goal is to reduce the amount of breakage of our bots, and in particular keep the pre-merge config as green as possible.
What is the change
First what this isn’t: there is no policy change, no existing practice should be affected. We can add as much documentation to make this explicit (suggestions welcome).
The goal here is to avoid people breaking the build inadvertently. As a bot maintainer I find every week a few PRs that lands with a broken pre-merge testing. The author often just didn’t notice that the build was broken. This is time consuming as I end up chasing which PR to revert and have to do it. It is also disruptive for everyone who try to open a PR rebased at HEAD when it is broken.
GitHub has a feature to help preventing this: when clicking on the merge button it can display a warning before allowing to merge:
This does not prevent merging: one can still inspect the pre-merge failure, looks at the buildbot, and consider that the build is already broken at HEAD in the same way and merge. The actually change is this one extra box to tick before merging!
Also we’re updating the developer guide, see the proposed changes.
Previous problems that led to the rollback / Why is this different?
The change implemented by @tstellar included two other checks:
-
Warn on the code formatter (clang-format): this created concerns because of false positive. Let’s keep this out-of-scope for the sake of making progress: we’ll open another RFC for the code formatter.
-
Warn on trying to merge without getting approval (this was brought up by @rengolin in Block Github merge on changes requested?)
This was added with a side-effect: to be able to enable the warning when someone wants to merge without approval, we had to also tick the box which says “Require a pull request before merging”. This didn’t prevent direct pushes tomain
, but added the following annoying message:
This made the author of the push think they did something wrong by pushing to main without a pull-request.
We can’t get around removing this message because the way GitHub is structured.
So similarly to the code formatter, for the sake of making progress on the CI checks, I propose that we keep this for another RFC and keep this focused exclusively on the CI pre-merge check.
We will still get as a side effect of this that folks who push directly to main will see this message from git
:
remote: Resolving deltas: 100% (1/1), completed with 1 local object.
remote: Bypassed rule violations for refs/heads/main:
remote:
remote: - Required status check “buildkite/github-pull-requests” is expected.
Pinging some folks who chimed in the previous discussion: @tstellar @rengolin @preames @AaronBallman @clattner @rnk