Test-infra: github status labels show for jobs that do not run against a PR

[BenTheElder]

Ignoring even all of the cases where we change the jobs configuration, if PR foo modifies some file and then later updates to no longer modify this file it seems we will leave up a GitHub context/status for jobs against the initial PR.

I think this is actually rooted in the fact that we persist the job spec for a PR when re-running it so stale PRs can get into a weird state. I'm not sure what we can or should do about this, but it has caused a bit of confusion. EG https://github.com/kubernetes/kubernetes/pull/49654#issuecomment-334829743

See also https://github.com/kubernetes/test-infra/issues/4662

[BenTheElder]

/cc @cjwagner @kargakis @spxtr @luxas

[BenTheElder]

In particular pull-kubernetes-e2e-kubeadm-gce — Parent Job Status Changed: Job triggered. was left on a lot of PRs in k/k and has been a recurring source of confusion as to PRs being green. One option we might take is to just improve the documentation around jobs being required or not.

[jcbsmpsn]

Improving the documentation doesn't help a lot unless there is a link to the documentation from the yellow check. Otherwise the documentation is too far away from the PR.

[BenTheElder]

@jcbsmpsn we use the detail link to point to the job results on gubernator, so I don't think linking to the documentation from the yellow check instead would be an improvement.

Reviewers in particular though should be aware of jobs being in the required-context or not. There are more not-required jobs than required because we don't want to block PRs but we do want them to be run and some of them to be visible. Some of these jobs are going to be blocking eventually but are not yet, EG: https://github.com/kubernetes/test-infra/issues/4642

[spiffxp]

https://developer.github.com/v3/repos/statuses/#statuses

We can't remove status contexts, best we would be able to do is overwrite them for a given commit if they're known to be stale.

Status contexts that are required for merge have a little "Required" label next to them in the PR (though this information doesn't persist after merge, eg: https://github.com/kubernetes/kubernetes/pull/52823#event-1264076561

[BenTheElder]

This is still a problem, I'm not sure what causes this but the kubeadm job shows up as "Yellow" on PRs in k/k very commonly (xref: https://github.com/kubernetes/test-infra/issues/4931)
/cc @luxas

We can "fix" this when #4932 is fixed but there is still a bug related to this somewhere and it will resurface if this job ever becomes blocking...

[BenTheElder]

xref: https://github.com/kubernetes/test-infra/pull/5655#discussion_r154221498

[kargakis]

Yeah, let's use this issue to track adding a skip command.

[kargakis]

I have started working on the skip plugin.

[kargakis]

@BenTheElder can you verify when you see a stale PR that GetCombinedStatus for its HEAD does include the stale context?

[BenTheElder]

@kargakis I can in a bit, I need to debug #5859, @cjwagner would you have a second to verify this? If not I can later, I definitely want to see this sorted out.

[cjwagner]

I can verify it if I could find such an example, but I don't see any PRs that have a stale context. I tried to reproduce by pushing a new commit to kubernetes/kubernetes#55339 after having run the pull-kubernetes-cross job manually, but the context was skipped properly.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[BenTheElder]

I think we've done what we can here.