The owners check is not reproducible, because it depends on the state of
the NixOS org on GitHub. Owners can rename their accounts or they can
leave the organisation and access to Nixpkgs can be removed from teams.
All of this breaks the owners check for reasons unrelated to the PR at
hand.
This PR makes the check for the owners file conditionally required: Only
when the ci/OWNERS file is actually modified a failed check will block
merging the PR. When that's not the case, the check will still fail
visibily in the checklist, but the failure can be ignored.
This is especially relevant for the Merge Queue, which should not be
entirely blocked whenever any of these events happen.
Also, it allows passing the checks in a fork when testing, where the
owners check will *always* fail, because the respective teams and
members are never part of the "user org" that a fork is.
The required status checks should depend on exactly one workflow,
triggered via pull_request_target or merge_group. Anything that is
triggered by pull_request is for testing purposes of the workflows
themselves only.
Posting the status manually allows us to avoid the strange "skipped ==
success" logic and properly skip the `unlock` job for pull_request
events in the next commit.
This should be much easier to understand than the previous logic.
When a contributor mistakenly sets the wrong target branch for a Pull
Request, this can lead to bad consequences for CI. Most prominent is the
mass ping of codeowners, that is already handled in
`ci/request-reviews/verify-base-branch.sh`. But there are other things
that go wrong:
- After eval, a mass ping of maintainers would still be possible, in
theory. Practically, this doesn't happen, because we have a limit of 10
reviewer requests at the same time.
- This will most often contain a change to `ci/pinned.json`, thus the
full Eval matrix of all Lix/Nix versions will be run, burning a lot of
resources.
- The PR will be labelled with almost all labels that are available.
We can improve on the current situation with some API calls to determine
the "best" merge-base for the current PR. We then consider this as the
"real base". If the current target is not the real base, we fail the
prepare step, which is early enough to prevent all other CI from
running.
This moves the no-channel-base check into the prepare script to exit
early and prevent all of CI to run against those branches. We also
provide better output by posting a "Changes Requested" review, using the
existing infrastructure from the old cherry-picks check.
The review will be dismissed automatically once the branch has been
corrected, because the commits check will run and do it.
This is the very first step to extending the commits job to do more than
just cherry-picks in the future: It could check reverts or merge
commits, but also the commit message format and more.
Of course, cherry-picks are still just checked on the stable branches as
before. For now, this allows us to run the part that dismisses automated
reviews automatically. This helps us when we do branch related checks in
the prepare step, which would also create such a review. To avoid
cluttering multiple reviews across a PR, we'll want all of these reviews
to be handled by the same code, thus this change.
Changes to the merge-group workflow should also validate that the file
is hooked up correctly and works - otherwise we risk merging CI changes
that cause the merge queue to fail consistently.
Previously, the test for the push workflow was running on the HEAD
commit of the PR only. It would be better to run it on the merged result
instead, just like any other tests we run in a PR.
This workflow runs the PR and Push workflow files on a `pull_request`
trigger. The intent is to test changes to the workflow files
immediately. Previously, these were run directly from the respective
workflow files.
The new approach allows us to move the logic to run this only when
workflow files changed from the pull_request trigger into a job. This
has the advantage that older jobs are cleaned up, when the PR changes
from a state of "workflow files changed" to "no workflow files changed".
This can happen when changing a PR's base from staging to master, in
which case changes from master would temporarily appear in the PR as
changes. When these include changes to workflow files, this would
trigger the PR workflow via `pull_request`. Once the base is changed,
the PR is closed and re-opened, so CI runs again - but since it's on the
same commit and the new run doesn't trigger `pull_request`, the results
of the previous run are still kept and displayed. These results may
include cancelled or failed jobs, which are impossible to recover from
without another force-push.
Checking this condition at run-time is only possible, because we move it
into a separate workflow, turning the `pr.yml` workflow into a re-usable
workflow. This will make sure to skip the whole workflow at once, when
no change was detected, which will prevent the "no PR failures" job from
appearing as skipped - which would imply "success" and make the PR
mergeable immediately. Instead the "no PR failures" job is not shown at
all for this trigger, which is generally what we want.
Do the same for `push.yml` for consistency.
Both `nix flake check` and `nix-env` are single-threaded, so no reason
to serialize their calls and waste time.
This brings down the runtime for this job from ~1:51 to ~1:27 in my
tests.
The python-updates branch is not a "development" branch in the sense of
ci/README.md's classification. That's because it allows force pushes.
When rewrites are possible, cherry-picking from this branch should not
be allowed, because the commit references will potentially end up out of
sync.
These kind of branches are now termed "Work-in-Progress" branches. Up
until recently these branches didn't work well for Pull Requests
targeting them, because Eval wouldn't run on them with a push event and
thus, Eval in the PR couldn't succeed either. That's now fixed, PRs
towards *any* WIP branch should work correctly.
Instead of uploading the outpaths as artifact, this uploads them via
cachix. Most of all, this makes CI less brittle, because Eval in PRs
will still be able to succeed, even if no workflow run for the push
event could be found on the target branch. It will just take longer.
This also makes moving Eval into the Merge Queue easier to do: When
downloading artifacts from a different run, these would always have to
match on the right event, too. By pulling from cachix, the same workflow
can support target branches with merge queue and without merge queue at
the same time. The latter would still use the push event, while the
former could use the merge_group event.
Last but not least, this should fix Eval on PRs targeting `wip-`
branches and any other branches that the push event doesn't trigger on.
These would never find an Eval result from the target branch and could
never show rebuilds accurately. Now these PRs should work at a slightly
higher runtime cost.
With the latest performance improvements, this only adds up to 1s, but
reduces complexity in managing pinned checkouts explicitly, especially
when we need *both* pinned checkouts in the next commit.
Instead of fetching up to 3 times on each new checkout, we now fetch all
the commits we're going to need at once. Afterwards, we checkout the
different worktrees in parallel, which doesn't give us much, yet,
because it would still be IO-bound on its own. Inconsistent IO
performance on disk is also the biggest limitation for checkout right
now, where checkout times range everywhere from 20s to 40s.
By checking out the worktrees on a tmpfs, the actual checkout only takes
1s and benefits from parallelization. The overall checkout time is now
8-11s, depending on the number of commits.
That's a reduction of 10-30s and we get this speedup for almost every
job in the PR workflow, which is huge.
This potentially has a nice side-effect for Eval, too: Because the repo
is in RAM, Eval seems to run slightly faster, up to 10 seconds less.
The two as-at inputs, were not boolean anymore, but GHA didn't complain.
Input types are actually not a thing for composite actions - they are
only documented for callable workflows.
For PRs targeting a fast moving branch like master, it might not
actually help because the merge commit will still be different on each
run. But on slower moving branches or when testing in a fork, it can
speed things up.
Since each of these build results is only a few hundred bytes without
any dependencies, it has essentially no cost.
Not adding this to `treefmt`, because this looks at *all* the files in
the repo, so needs to re-run even when iterating on CI only.
Pushing the source path to cachix is pointless, because the only source
we're using is Nixpkgs - and that will always be available already via
checkout. No need to ever substitute it, so no need to push it either.
This allows setting up a custom cachix cache in a contributor's fork.
That's most valuable when working on CI itself, testing and interating
in a fork. It's required when working on cachix related features
directly.
This moves the logic to "check whether the PR can be merged and
determine the relevant commits" into the PR / prepare job directly -
since that's the only place where it is supposed to be used.
Because of the if condition in get-merge-commit, this logic was run
multiple times, for example in the lint workflow, where only one of
targetSha or mergedSha was provided as input. However, this input was
thrown away directly. This might not be a big problem, because this was
not expensive, so far. But with the next commit, this will become more
so.
This also separates the logic a bit cleaner - `prepare` figures out all
the parameters for the whole PR workflow, while `checkout` handles the
consistency around these checkouts.
These two jobs mostly used the same code anyway, so they can easily run
in the same job. This uses fewer resources and avoids running (and
failing) the codeowners review job on Pull Requests from development
branches like staging-next or haskell-updates.
It also opens the door for more improvements later on, for example it
will be much easier to implement a shared reviewer limit for both owners
and maintainers now.
This runs the "check owners" job as part of the main PR workflow, with
multiple advantages:
- The job doesn't run anymore when undrafting a PR, where it's not
required, because the OWNERS file's contents didn't change.
- A valid OWNERS file is now a requirement to merge a PR.
- The OWNERS file is always checked on the exact same test merge commit
that the remainder of the workflows are running on as well.
This just moves the code over to ci/github-script to make it easy to
test and iterate on locally.
The name `prepare` is chosen, because the script will be extended with
the other steps from "PR / prepare" next.
The filtered checkout we used before was a nice idea, but only worked
for small numbers of commits in a PR. It would fetch the whole history
initially, but only fetch objects on demand. This turns out to be much
too slow in a PR with 18 commits, regularly hitting the 10 minute
timeout, even when running it locally.
The new approach uses regular checkouts again. In contrast to the old
style, before we switched to the filtered checkout, this only fetches
exactly the commits we need - and all of them at once. This is
significantly faster than both other approaches, and scales much better.
A bigger number of commits doesn't have much of an effect, if any at
all.
The `cancelled()` condition seems to only apply when *the whole
workflow* was cancelled. This is not the case when a single job is
cancelled due to timeout.
We can replicate this by checking each needs.result manually.
This currently times out after 3 minutes. Give it a bit more time. 10
minutes might be excessive, but we only really want to guard against a
stuck job taking 6 hours.
This currently happens, for still unknown reasons, for the "check cherry
picks" job. The job gets cancelled by GHA mid-way. This should be the
same as an error, because an important check didn't run: Merging should
be blocked and auto-merge should not succeed.
