This allows committers to approve PRs with additional, optional nits
that the author-maintainer can either address or merge immediately
without these changes.
It also allows committers to approve a PR for merge, while still waiting
for other maintainers to give their feedback - they can then merge the
PR directly instead of passing it back to the committer.
While it was already the case that only merge comments *after* the
latest push were acted on, the logic wasn't easy to understand. This
change should make it more obvious, specially in combination with the
next commit, that all steps (comments, approvals, merge) must happen on
the same SHA - the current head SHA of the PR.
All other conditions equal, there is no reason to prevent maintainers
from backporting changes to their packages. Maintainers are probably in
the *best* position to tell whether a certain change is backportable or
not - because they know the package well.
This supports AND on the first and OR on the second level, which is
needed for some follow up work like backports, approval based merges or
trusted maintainers.
We have not observed this merge method being used in practice, yet. Not
in the new bot, not in the old bot. It seems like auto-merge works for
all cases.
No special casing anymore, all conditions are in the same place. This
also has the benefit of hiding the "has maintainers eligible for merge"
condition from comments, because it is only really relevant for
labeling.
We have no chance of getting a token that can request the team endpoints
in the pull_request context. This makes sense, because non-members of
the org are also not allowed to view the teams' memberships.
Thus, just fake an empty team - that's fine for the Test workflow.
Running the nixpkgs-merge-bot in GitHub Actions instead of a separate
workflow has multiple advantages:
- A much better development workflow, with improved testability.
- The ability to label PRs with a "merge-bot eligible" label from the
same codebase.
- Using more data for merge strategy decisions, for example the number
of rebuilds.
This commits re-implements most of the features from the current
nxipkgs-merge-bot directly in the bot workflow. Instead of reacting to
webhook events, this now runs on the regular 10 minute schedule. Some
merges might be delayed a few minutes, but that should not be a problem
in practice.
To give the user early feedback, there are additional workflows running
when a comment or review is posted. These react with "eyes" to make the
user aware that the comment has been recognized.
The only feature not taken over was the size check for files in the PR.
This kind of check is not really relevant for maintainer merges only -
if we want to prevent bigger files from making it into the tree, then we
need a generic CI check, which is out of scope for the merge-bot.
Other than that, everything should be implemented - any omissions are by
accident.
This workflow / script is already doing more than must labeling: it's
already auto-closing package request issues.
Since we're going to migrate the nixpkgs-merge-bot into this workflow,
we'll rename things to a more generic name.
To be able to disable the pr.yml workflow on GitHub, we need to rename
it to a different name. Let's use the long name for consistency with
merge-group.yml. This only affects the GitHub-internal name, not the
visible name in the PR checklist, which is still "PR". This visible name
is also used by nixpkgs-review, so that won't break.
Instead of setting the maintainer-related labels based entirely on Eval
results, this uses the new maintainer map from the target branch. This
allows labeling PRs correctly, that had been created *before* a
contributor became a maintainer of the respective package.
Creates a team sync workflow that pushes the current state of teams to a
JSON file, which can then be ingested by `lib.teams` to expose member
lists.
Co-Authored-By: Alexander Bantyev <alexander.bantyev@tweag.io>
Some PRs are empty on purpose, for example the yearly notification about
the election for voters. We should not close these because the merge
commit is empty - only if there was a change intended, but the merge
commit *becomes* empty, we should act.
If the change of a PR has already been merged to the target branch
elsewhere, the PR will not be auto-closed by GitHub - and will still
show the same original diff. Still, the temporary merge commit is
actually empty. This causes all kinds of strange CI behavior, from not
showing rebuilds to not pinging maintainers.
We check the merge commit during labeling anyway, to see whether a merge
conflict is present. It's easy to just look a the number of affected
files in this merge commit - and if there are none, we can just
automatically close the PR as no longer relevant.
This removes the "owners" check from codeowners-validator. With it, all
tokens and permissions can be removed, because these were only needed to
make these requests.
This solves the problem of codeowners-validator not supporting our new
nested team structure for nixpkgs-maintainers. To make the onboarding of
new teams easier, we moved all teams "under" the nixpkgs-maintainers
team. This makes them inherit the right privileges (triage) for Nixpkgs.
However, this inheritance is not recognized by codeowners-validator,
thus it assumes that these teams don't have access to Nixpkgs. This then
fails the owners check immediately.
Removing the owners check also has a few other advantages:
- This check depends on external state: If a user is renamed or a team
removed, the check will fail. This makes it a bad check for required
status checks or merge queues - the check might fail randomly,
independent of the current PR.
- Running this check in a fork will never work, because the respective
users and teams don't have access to the fork's repo.
Both of this required us to set `continue-on-error: true` most of the
time.
This allows the labels workflow to support issue management in two ways:
- New package request can potentially created with a `4.workflow:
auto-close` label immediately and be closed automatically this way.
- Existing package requests can be bulk-closed by adding this label.
This has the advantage of posting the explanatory comment at the same
time, which is not possible with regular bulk operations.
Not a problem for prepare/commits, but the labels comand will remove the
temp directory again, before it actually runs the command. Nothing good
will come out of that!
When a user deletes their account, they appear as a "ghost user". This
user is represented as `null` on API requests. If such a user had posted
a review before, this breaks a few places, which assume to be able to
access `user.login`.
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.
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.
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.
This needs the multiline flags, which enables `^` and `$` to match line
start and line end, not start and end of the whole string.
Not sure how this got past testing when initially merged.
Logging objects to stdout is not possible with `core.info`, so we
fallback to `console.log` instead. There's no functional difference for
these anyway.
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.
