Related: #433640, #433641.

Like the other PR, this causes too many rebuilds to go to master, can you retarget the branch to staging? If you haven't done that before, there is an explanation in CONTRIBUTING.md in the root of the repo.
Thanks!

Like the other PR, this causes too many rebuilds to go to master, can you retarget the branch to staging? If you haven't done that before, there is an explanation in CONTRIBUTING.md in the root of the repo. Thanks!

@r-vdp Done.

@wolfgangwalther I think here we have an example where a staging PR fails «no PR failures» due to the docs build being cancelled due to timeout, a not-so-surprising thing to happen on staging — what is the case where «cancelled» should actually be treated as hard-failure in this workflow?

The problem is a different one: We have two triggers to run the PR workflow on pull_request_target (regular PRs) and pull_request (when testing changes to CI itself). You can see that all the pull_request_target jobs pass just fine. The pull_request trigger was only run for a few secs while changing the branch, because at that time GHA thought there were changes to the .github folder. Since these changes are not there anymore after both the target branch was changed and the rebase done, this trigger is not running anymore - and thus the cancelled job remains as "failed" and is not updated.

It's on my radar already, but fixing this takes a bit more work.

In such a case, you can double check whether PR / no PR failures (pull_request_target) is skipped. If it is skipped, that means the PR is good (for GHA). You can ignore PR / no PR failures (pull_request) in this case.

Uhm, if it is OK to merge over red, it is not OK to have this failure red…

GitHub consistently calls this failure «cancelled», and the workflow checks for «cancelled» separately from «failed». Is it likely that GitHub actually reports «failed» to the workflow here? If not, is there any case where «cancelled» should be red failure and not some kind of neutral/skipped/cancelled (whatever can be actually set)?

Uhm, if it is OK to merge over red, it is not OK to have this failure red…

Well, the pull_request target part should just not play any role in "required status checks". And it should just not be run at all. Then there wouldn't be any red here.

GitHub consistently calls this failure «cancelled», and the workflow checks for «cancelled» separately from «failed». Is it likely that GitHub actually reports «failed» to the workflow here? If not, is there any case where «cancelled» should be red failure and not some kind of neutral/skipped/cancelled (whatever can be actually set)?

Yes, there are cases of cancelled that need to be red: When a job times out with the timelimit we set for each job, it will appear as cancelled. Last week we had some cases where that happened and the "required status checks" part still reported "all green". This was barely visible at all, so the current "red" reporting is much better, because these cases need to be hard fails.

As I said, I am working on this, so that it won't happen anymore.

Well, the pull_request target part should just not play any role in "required status checks".

There are probably exceptions to that?

And it should just not be run at all.

You mean that branch switching should be done differently?

When a job times out with the timelimit we set for each job, it will appear as cancelled. <…> This was barely visible at all, so the current "red" reporting is much better, because these cases need to be hard fails.

Wait what? Timeouts on staging need to be hard fails? When?

Well, the pull_request target part should just not play any role in "required status checks".

There are probably exceptions to that?

Sorry, a "target" accidentally sneaked in there - please ignore that. The pull_request triggered workflows should not block a PR. We use them to test new workflows, when we work on CI. Of course the feedback of those is important. But since they interact with other parts of CI, they can't be guaranteed to always pass, even if the changes are good. That's the case, for example, when the interface between push events on the target branch and pull_request_target events in the PR changes - then the pull_request event will already use the new interface, but there was no related push, yet.

And it should just not be run at all.

You mean that branch switching should be done differently?

I mean that CI should not run on that intermediate state that often happens when switching master -> staging or the other way around, where the branch temporarily contains a lot of commits from the other branch. These other commits often also contain changes to the workflows, which had already been merged earlier. Right now, these trigger the pull_request stuff - but they don't need to. They had already been tested.

You might know the comments that the codeowners job posts from time to time: "This PR includes xyz commits from another branch, you probably want to rebase". This check prevents the codeowners job from doing those nasty mass pings in this scenario. But that's the only thing that is guarded by this check right now - and I am just saying, that we need to guard all other jobs on the same as well. This is a temporary state, that needs to be fixed first, before it makes sense to run CI again.

When a job times out with the timelimit we set for each job, it will appear as cancelled. <…> This was barely visible at all, so the current "red" reporting is much better, because these cases need to be hard fails.

Wait what? Timeouts on staging need to be hard fails? When?

This has nothing to do with staging. For example, the "check cherry picks" job currently has a timeout of 3 minutes. But it was implemented very inefficiently, so it timed out - it would have needed an hour or more for some backports with many commits. This timeout would silently be "OK to merge" in terms of required status checks. This is actually dangerous, because it means some backports would not have been checked properly anymore, assuming the check cherry pick job did its work. Now, these timeouts cause hard fails.

GitHub Actions CI is designed that it should never timeout on staging. We're building the manual, the shell etc. (all the "Build / ...") based on the pinned nixpkgs revision in ci/pinned.json, which is always cached. Of course, OfBorg can timeout on staging, but that's not included in the required status checks anyway and entirely separate.

There are no exceptions to be made (wrt to GHA CI) for staging or so. The only exceptions that should be made right now is after a switch from master to staging (or similar from release branch to master etc.), where this specific case that I detailed here can happen. I hope to solve this soon, though, so "merging with red checks" should not become a habit / something to regularly do.

I hope that clarifies it a bit more?

OK, I see the plan, thanks!

(Obviously, I now hate a little bit more the ill-fitting boolean logic GitHub uses instead of absolutely obvious at-least-tristate here…)

GitHub actually has more than two states (pass/fail), they have neutral, cancelled, skipped etc., too. The problem is, that these can't be set from inside a job - it's only possible to set them via API, when posting status checks manually (as OfBorg does).

First, you cannot set extra states from inside the job, second, summary overall is green or red or in-progress, as far as I have ever seen.

Please note that the failure mode discussed above should not happen anymore, it should be fixed as of #435547. If you come across another case where you feel like you need to merge despite CI being red / CI is red when it shouldn't, please let me know.

Oh great, thanks!