Add status-based test exemptions

[jlebon]

These patches bring status-based test exemptions similar to what we already have for Travis CI.

In most cases, the same tests are run on both PRs and the auto branch. If these tests output a commit status, we can be a bit smarter about testing. For example, if the PR is fully rebased and already passed the statuses we would expect on the auto branch, then there's no point in retesting it.

The first two patches do some clean ups and refactoring in preparation for the third patch, which introduces the status_based_exemption setting.

---

This change is

[emilio]

Hey, thanks for the PR! This is failing flake8, so it should be fixed before merge.

I'm not the most adequate person to review this so...

r? @Manishearth

[frewsxcv]

flake8 has some opinions:

homu/server.py:126:75: F821 undefined name 'ex'
homu/server.py:535:83: F821 undefined name 'ex'
homu/main.py:477:38: E211 whitespace before '('
homu/main.py:587:1: E302 expected 2 blank lines, found 1
homu/main.py:646:9: E129 visually indented line with same indent as next logical line
homu/main.py:700:9: E129 visually indented line with same indent as next logical line
homu/main.py:742:5: E303 too many blank lines (2)
homu/main.py:750:9: E129 visually indented line with same indent as next logical line

[jlebon]

⬆️ Updated to please flake8.

[Manishearth]

This makes the thing un-lazy, since s is unconditionally evaluated. Set s to lambda: ... and use it directly

[Manishearth]

Overall looks good.

Most CI services run two builds -- one per commit, and one per PR. For status builds we only provide the "per commit" context (continuous-integration/travis-ci/branch, etc). Exemption will only work if the commit was made against master. The "per PR" context (continuous-integration/travis-ci/pr) is a build against master at the time of the build being initiated. This has a much higher chance of being exempted. Additionally, PRs from external branches won't have the "per commit" tests being run.

Perhaps we should allow an optional alternate context for the same status in the cfg, and if any of the two contexts pass, exempt the build?

[Manishearth]

The other issue with the "per commit" status is that it can be spoofed by changing the travis settings for your fork.

[cgwalters]

If we're concerned about spoofing, a simple fix would be to support something like an creator option:

[repo.ostree.status.atomicjenkins]
context = "Jenkins (Red Hat Internal)"
creator = "rh-atomic-bot"

(It's part of the status data github returns)

[Manishearth]

But the bot never creates the commit in a PR, it only creates a merge commit later. Exemption is for stuff done before you even r+.

[jlebon]

Most CI services run two builds -- one per commit, and one per PR. For status builds we only provide the "per commit" context (continuous-integration/travis-ci/branch, etc). Exemption will only work if the commit was made against master. The "per PR" context (continuous-integration/travis-ci/pr) is a build against master at the time of the build being initiated. This has a much higher chance of being exempted.

I'm not sure I follow this, could you expand? The status-based exemption added here isn't meant to exempt testing of the PR itself by external CI tools like Travis, but rather the "second" testing of the commits on the auto branch. (It sounds like you're talking about telling the CI service to skip even the "first" test of the PR/branch, is that the case?)

Perhaps we should allow an optional alternate context for the same status in the cfg, and if any of the two contexts pass, exempt the build?

I'm guessing you're referring to the travis case here (where we expect /push on auto but look for /pr on the PR) ? That exemption logic is left alone and doesn't actually fall under the new status-based exemption path. Or are you suggesting to allow users to provide a map of "equivalence" for each status between branch and PR contexts to generalize the travis case?

[Manishearth]

(It sounds like you're talking about telling the CI service to skip even the "first" test of the PR/branch, is that the case?)

No 😄

Or are you suggesting to allow users to provide a map of "equivalence" for each status between branch and PR contexts to generalize the travis case?

Yes 😄

I know the travis logic was left alone, but I want users to have the possibility of using that logic for any CI service. Appveyor for example provides two contexts so it can be used there.

[jlebon]

Ahh, got it thanks for the clarification! Will make the appropriate tweaks.

[jlebon]

⬆️ Added a new pr_context setting.

[cgwalters]

I didn't review every bit of the code in detail - there are a lot of changes, but (as usual) it all looks good at a high level. I have a bit of uncertainty around the semantics of this in different cases (e.g. for users doing fast-forwards vs merges).

How much did you test this? I have no problem updating our staging homu.

One thing I've been thinking about that might help is to have two testers - one that's fast/sanity, and one that does expensive tests. The PR tester could do both, but prioritize sanity testing PRs first. The expensive test would be run on the merge queue, and I think we could equally well support the "rollup" merge for fast forwards too (if we don't today).

[jlebon]

I didn't review every bit of the code in detail - there are a lot of changes, but (as usual) it all looks good at a high level. I have a bit of uncertainty around the semantics of this in different cases (e.g. for users doing fast-forwards vs merges).

Thanks! By "different cases" do you mean different settings (e.g. linear and autosquash) or different scenarios (when the PR is fully rebased and when it's not but the merge is tested)?

How much did you test this? I have no problem updating our staging homu.

I got a set up here. I only tested it with linear and autosquash on, though I don't expect issues with the other configurations since the actual merge path was not touched and is shared with the previously existing travis exemption.

One thing I've been thinking about that might help is to have two testers - one that's fast/sanity, and one that does expensive tests.

Yup, I can definitely see the usefulness for something like this as a next step. Status-based exemption would help here too so that homu doesn't have to wait for the expensive test if the PR tester already got to it by the time the PR gets accepted.

[jlebon]

I only tested it with linear and autosquash on, ...

To clarify, I only tested with those on (since I consider my changes orthogonal to those), but I did test all the the new code paths as well as the travis exception. You can see the PR history on https://github.com/jlebon/rpm-ostree to see what it looks like.

[Manishearth]

(probably will review this soon, sorry for the delay)

[Manishearth]

Looks good to me with the pr_context changes. Thanks!

[cgwalters]

One thing to bear in mind is @jlebon and I are using homu with fast forwards and squashing, hence we use a fixup! workflow. But this repo ironically isn't actually using homu, so the merged series still has the fixup!.