This was introduced as part of the hotfix PR to avoid hitting API rate
limits - but the condition was wrong. It was supposed to trigger in all
PR contexts, not only for the Test workflow.
When hitting a treewide, we would previously find the username for each
user and then check all of them for collaborator status - only to then
realize that this results in more than 15 reviewers and exit.
We can put a simple stop-gap in, even before de-duplicating the combined
lists of maintainers and owners as safe guard. We could still hit huge
numbers of code owners, but in practice we don't nearly as many as
maintainers, so this will be sufficient for now.
We use the files endpoint to get a list of all *names* of files touched
in the PR - but this endpoint will also actually download the files /
their diff, too. That's pointless and actually takes quite some time for
huge treewides.
We're just putting in a stop-gap for now, so that we're not burning more
than 1 API requests on this and don't spend so much time on it either. A
limit of 99 files will be more than enough for quite some time - we will
only need to raise this when we're able to represent package sets in
by-name properly and have "package set maintainers", who are not
committers.
Unfortunately it still happens frequently that, after enabling
auto-merge, GitHub is stuck even though all checks have passed, and
doesn't merge the PR. Any contributor can trigger GitHub again with an
approval of the PR - this will then immediately queue the PR for merge.
Adding a hint to the posted comment, should help users through this
without my intervention.
The recent change to use the result of requesting reviewers for setting
the `needs: reviewer` label caused a regression: It would not set the
label for PRs where no reviewers were requested, because *too many were
eligible*. Still - these PRs don't have reviewers, so they need
attention otherwise - via the label.
This was introduced shortly before merge of the reviewers.js file, but
not actually tested - I thought it was not easy to find a PR triggering
this warning. However, the scheduled run told me otherwise: The
staging-next PR is the perfect candidate.
We only recently introduced the owners.txt file to the comparison
artifact, so once the bot runs on a schedule it will it older artifacts
very quickly - and then can't find the owners file.
We can fallback to an empty owners list in this case, because an older
artifact also means an older workflow run previously, so this will have
pinged owners already.
This migrates the bash code to request reviewers to github-script. This
will allow multiple nice improvements later on, but at this stage it's
mostly a reduction in code and complexity.
We technically counted bot approvals and approvals by deleted users for
the approval labels as well. The former don't exist, yet, but if they
were, I don't think we'd count them. The latter should arguably *not* be
counted, because we can't tell anymore *who* approved, so we can't put
any weight on it as reviewers.
This simplifies the logic, too.
When a user tries to merge a PR, but is not allowed to, it is helpful to
explicitly list the users who *are* allowed. This helps explaining *why*
the merge-eligible label was set.
I objected to this proposal before, because it would incur too many API
requests. But after we have restructured the checklist, this is not
actually true anymore - we can now sensibly run this only when a comment
is posted and not whenever we check a PR for eligibility.
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.
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.
These were done manually by me, either due to not matching the regexes in the previous ones, or because of nixf-diagnose, which I have as a pre-commit hook.
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.
