Required status checks in foreman.git

Development
1

You may notice that in foreman.git some statuses are now required to merge:

  • Redmine issues (modern PR Processor)
  • foreman (Jenkins)
  • upgrade (Jenkins)
  • continuous-integration/travis-ci/pr (Travis)

If one of these fails, only admins can merge a PR. I’ve enabled these to prevent accidental merges when tests failed, but realized after that perhaps we want some discussion around this.

Let me know what you think.

Note we have similar required checks in smart-proxy.git in place for some time.

2

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:

  1. 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.
  2. 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).

3

This is a different setting. Initially I forgot it, but I’ve disabled it. So it just requires some status checks to be green to allow the merge button.

True, but shouldn’t that motivate us to fix transient failures? :slight_smile:

I really miss the Gitlab feature to merge a PR when the tests have passed which would automate the checking back to merge.

What made me enable it was a commit that was merged but never ran the tests in the first place. That caused the tests to fail and break others.

4

Ah, great! :slight_smile:

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.

5

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.

6

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.

1 Like
7

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.

1 Like
8

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 :slight_smile: