| Age | Commit message (Collapse) | Author |
|---|
|
This allows re-using this elsewhere with a shared cache.
|
|
This makes reactions to merge comments and all the labeling a bit
quicker. Lower the number of backlog items to process per run
accordingly, so that we don't really need more API requests for it.
|
|
We used to employ the worst strategy for parallelism possibly: The rate
limiter capped us at one concurrent request per second, while 100+ items
were handled in parallel. This lead to every item taking the full
duration of the job to proceed, making the data fetched at the beginning
of the job stale at the end. This leads to smaller hiccups when
labeling, or to the merge-bot posting comments after the PR has already
been closed.
GitHub allows 100 concurrent requests, but considers it a best practice
to serialize them. Since serializing all of them causes problems for us,
we should try to go higher.
Since other jobs are running in parallel, we use a conservative value of
20 concurrent requests here. We also introduce the same number of
workers going through the list of items, to make sure that each item is
handled in the shortest time possible from start to finish, before
proceeding to the next. This gives us roughly 2.5 seconds per individual
item - but speeds up the overall execution of the scheduled job to 20-30
seconds from 3-4 minutes before.
|
|
By only ignoring already-handled comments when running non-dry, it's
much easier to look at existing PRs, for which the merge bot already
commented, and iterate on them locally.
It's dry mode anyway, so it won't hurt to get a few more merge comments
in the console output.
|
|
We previously used auto-merge first and then enqueued explicitly on the
assumption that auto-merge would fail if the PR was actually in
mergeable state already. This turned out to be false.
Instead, we currently face the problem of auto-merge sometimes getting
stuck. This seems to happen when, at the time of enabling auto-merge,
the required status checks already passed and the PR would be ready to
go - but sometimes GitHub doesn't do it. This *can* be unblocked by
approving the PR again, which seems to run the internal "let's check
whether we can merge this" procedures on the GitHub side again.
However, we can probably also solve this by just explicitly trying to
enqueue the PR first - and only if that fails, fall back to auto-merge.
I previously argued against that, based on a potential race condition,
in which a PR could become ready to merge between these two requests -
at which point the auto-merge operation would fail, if the original
assumption was true. But since we don't observe this, we might as well
switch.
|
|
When we recently refactored the code to use the maintainer map for
related labels, we made a mistake: When a PR has no packages with
maintainers returned from eval, the label would internally be set to `0`
instead of `false`.
The code would then go on compare the before and after labels with
strict equality - and assume a difference, because `0 !== false`. Thus,
it seemed like new labels needed to be set, so the PUT request was
actually sent. Of course, the labels were actually the same - when
filtering the labels to be set, the `0` would also be treated as falsy,
so the label would not be set. This would result in no visible change in
the PR, but internall GitHub would replace the `updated_at` timestamp
for that PR - after all we replaced all labels.
Repeatedly updating *all* PRs we're looking at quickly causes problems,
because we are going to look at the same PRs *again* in the next cycle -
essentially causing infinite recursion. The bot became slower and slower
over time, because it had to process more and more PRs each run.
Simply casting this to a proper Boolean, should get us out of the mess
soon.
|
|
|
|
When a deleted user had approved a PR, this will cause the merge-bot to
fail.
|
|
I didn't like r-ryantm "authoring"; so I changed that to "created"
earlier. Arguably, using "opened" is more consistent with what is
actually checked and can consistently be used for both.
|
|
The by-name check would previously be green when the
`pkgs/by-name/README.md` file was changed. This would still not mean the
maintainer was able to merge the PR, because there'd be no maintainer
for that file, but the feedback was not 100% accurate.
|
|
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.
|
|
This makes runChecklist mostly a pure function (except for logging) to
allow calling it repeatedly later.
|
|
This removes the need to `await` committers further down in the function
and allows re-using the cache for other teams later.
|
|
|
|
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.
|
|
This makes it more visible which PRs are merge-bot eligible, by setting
a label respectively.
|
|
By not keeping the node_id in the comments resulting from a failed
merge, these merges will be automatically retried.
|
|
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.
|
|
The labeler currently breaks for unmaintained packages after the recent
change to use maintainer maps.
|
|
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.
|
|
These are style-only changes, that are not enforced via tooling - but
used mostly consistently in the other github-script files.
|
|
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.
|
|
|
|
These can now be removed after enough time has passed.
Advanced search is only the default from November 4, according to the
GitHub docs at:
https://docs.github.com/en/rest/search/search?apiVersion=2022-11-28#search-issues-and-pull-requests
|
|
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.
|
|
Co-authored-by: Wolfgang Walther <walther@technowledgy.de>
|
|
|
|
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 allows re-using postReview in the next commit.
|
|
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 has severity "important", which is not a `core` function. Falling
back to `core.info` for all unknown values now.
|
|
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.
|
