While I like the fact that it blocks merging stuff that could break by mistake, there are two problems with the fact only admins can override this:
Every PR merged will cause all other PRs to be marked as needing rebase, blocking any merges even if they were just rebased a couple of days ago. While the git ui offers to merge develop into them, it will cause an additional merge commit which we don’t want. Also, every time a PR is rebased it triggers another test run (as it should, but not always needs) that takes around an hour, during which we can’t merge the PR because it isn’t green.
If there is a transient failure in one of the tests that we are aware of, non-admins will have to re-run tests and wait an hour again for them to hopefully be green instead of just merging right away.
I expect all these extra test runs might also increase the load on jenkins, which while isn’t as limited as it was in the past, still has limited slots.
Therefor, I suggest removing these limits, and perhaps implementing a more “friendly warning” type of message asking for a rebase if the code has converged too much (maybe after a certain time or number of commits).
Would it be possible to only make sure tests did execute as a required check (maybe as part of the pr bot check)? that would solve the issue of mistakenly merging untested code without blocking merges of PRs that have transient failures.
I don’t like it because it motivates people to hit the merge button without thinking. And I still want to be able to merge PRs that have transient failures. That’s way too much patronisation for my taste.
I’ve updated the required checks for now to only require redmine issue check to pass.
I know there have been cases in the past where reviewers have merged code that failed tests, but I believe it is their responsibility to check if failures are related or not and revert or fix if they merge something that breaks develop tests.
IMHO it’s sad our testsuite is so unreliable that we can’t require it. In fact, it’s very concerning.
If we have common transient failures, IMHO it’s better to fix or drop them rather than requiring all maintainers to know that.
However, I do see that it’s annoying when you can’t merge something even though you know it’s ok. I wish GH had an option that every maintainer (rather than only admins) can click twice with “I know what I’m doing”. That would prevent (most) accidental merges where the test suite hasn’t ran yet while providing sufficient flexibility.
It can be annoying, but I would love them being required as it would motivate us to solve the transient failures instead of getting used to them.
I see unreliable test suite as a big issue. Not mentioning it looks bad for one-time contributors.
I vote for required test suite pass