awesomeWM
Require PR merges to pass the CI pipeline
Currently, we only require the PR's tip to pass the CI pipeline, with the pipeline definition from that commit. The CI does run after a merge, checking the merge commit, but it doesn't prevent the merge and there is no indicator for failure (except for a tiny status icon on the main repo page). This leads to situations where changes to the CI pipeline are not considered by PRs that branched off of master before that change was merged.
Case in point: None (or almost) none of the currently open PRs have been rebased onto the docs linting changes. Even if their docs changes have issues, the CI wouldn't see them until after they are merged. And then those would appear as unrelated issues in all new branches created after the merge until someone fixes them as a follow-up to the original PR.
Tools like bors provide a way to automatically make sure that a PR's merge commit passes CI, not just its branch tip.
Yes that's something we can do. I think we should first fix Mergify and make the auto-merge rules more strict.
The ldoc part are just warnings right now due to this issue and the fact the linting code lack needs to be a bit more battle tested before it becomes a CI failure. But the overall point stands and we had many problems in the past where 2 parallel PRs collided "for real".
Back when we used Travis, the CI was also running all tests for all commits and failing the build. I know the current one compiles all commit, I need to check if it actually runs the tests for each of them and fail the build.
I know the current one compiles all commit, I need to check if it actually runs the tests for each of them and fail the build.
It only runs check-unit.
Running the whole test suite wouldn't be a problem, except that it takes about 3 minutes per commit (more than 6 hours for your doc linting PR, instead of 15 minutes).
I think we should first fix Mergify
I always wondered what good the auto-merge is when it blocks on humans approving the PR. It only saves a few seconds over the second reviewer clicking "Merge". And these days, two members having reviewed a PR before CI finished is very rare.
Either way, I'm pretty sure that Bors covers enough of what Mergify does here with
status = [] # add the checks that must pass
required_approvals = 2
up_to_date_approvals = true
block_labels = "no-mergify" # or whatever
Whoever reviews second comments bors r+ and once all checks pass, Bors will merge. Might even work so that if the first reviewer triggers Bors, it queues until the second approval, but I'm not entirely sure about that bit.
make the auto-merge rules more strict
What else would there be besides increasing approvals-required (for which there aren't enough active members around)?
Running the whole test suite wouldn't be a problem, except that it takes about 3 minutes per commit (more than 6 hours for your doc linting PR, instead of 15 minutes).
We did that with Travis and it helped for a while. Then they put a 50 minute limit and it wasn't helping much. Then they put a concurrency limit and now it was a nuisance. I think we should enable it again. Most PRs are in the >= 10 commit range. I also split the doc PR along the module line and artificially caused the commit count to be this high. It makes git blame + git show less painful than a single commit which changes every file. But if the CI took ultra long, I might have made a different choice :P.
I always wondered what good the auto-merge is when it blocks on humans approving the PR. It only saves a few seconds over the second reviewer clicking "Merge". And these days, two members having reviewed a PR before CI finished is very rare.
A bit of history here. Mergify original author is Jd, who is also the original author of JDWM/AwesomeWM. At first, the idea was pretty much to beta test it. I proposed a bunch of nice features, which eventually got implemented (thanks!), but we never enabled them (and this was the dark age of post-acquisition Travis CI where everything got less reliable every other week).
- Rebase all PRs with more than 1 commit to the current HEAD
- Use a cherry-pick/squash strategy for single commit PRs
- Add pull request number to the commit message
- Add commit count, original parent commit and other metadata to merge commits
- Add reviewers to the commit message
autosquashallows the git-rebasefixup!commits to be applied clean. Right now we accidentally let a few in the main branch
It also avoided approved PRs getting stuck unmerged because "umm, oh, someone else might have an opinion" kind of blackhole.If you use git log --graph or tig/gitk on the current history, some parts look like a real spaghetti. Mergify can make this more linear.
@jd is it possible to rebase, then wait to see if the tests pass before merging?
is it possible to rebase, then wait to see if the tests pass before merging?
According to https://docs.mergify.com/actions/rebase/ : Yes.
But I would prefer to not use it because
GitHub Applications are not allowed to force-push to a branch. To get around that limitation, Mergify tries to impersonate the pull request author or one of the repository members to force-push the branch.
With a first look at the "queue action" in Mergify (https://docs.mergify.com/actions/queue/#queue-action-page), it seems to be possible to do run the CI again the PR merge commit in master. So I don't think we need to use bors or any other tool here. We should investigate further the features available in Mergify and configure it to perform such tests.
Mergify's Queues seem to work quite differently from Bors:
Mergify would automatically merge the
mainin the base branch. The continuous integration system would have rerun and marked the pull request as failing the test, removing it from the merge queue altogether.
Their features sounds to me like an automated "merge the base branch into the PR", so that every PR would end up with at least one "Merge master into <this_feature_branch>" commit, more if the merge fails.
Bors, on the other hand, utilizes a separate staging branch, where it performs the test merge and doesn't touch the PR or base branch until the actual merge needs to be performed.