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.
(cherry picked from commit a146035a2b)
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.
(cherry picked from commit d086c6c6b3)
This lead to reaching secondary API limits in a treewide recently, so we
better limit it to where we actually need it.
(cherry picked from commit cd7f83638e)
We still use a few too many API requests by checking team members for
collaborator status - we can improve on that in the future.
(cherry picked from commit 17199e5ff6)
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.
(cherry picked from commit 9efe926863)
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.
(cherry picked from commit 51acc56dcb)
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.
(cherry picked from commit 4658d0d5a3)
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.
(cherry picked from commit d76ffa4136)
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.
(cherry picked from commit c4548e58fb)
This should fix the bug where the "needs: reviewer" label was set too
early, just to be removed immediately, because reviewers were then
requested.
(cherry picked from commit e68b0aef13)
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.
(cherry picked from commit a23d0ab24c)
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.
(cherry picked from commit df6a9a739d)
This partially reverts commit 1197fe48da.
GitHub just doesn't schedule these narrow intervals. 10 minutes is
alright in practice.
(cherry picked from commit 74d6ba3ab4)
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.
(cherry picked from commit 1e6124a504)
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.
(cherry picked from commit 2d6602908b)
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.
(cherry picked from commit 747d9e2d34)
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.
(cherry picked from commit 1197fe48da)
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.
(cherry picked from commit 810b9ba51d)
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.
(cherry picked from commit c768b4243e)
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.
(cherry picked from commit 5407abeb7d)
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.
(cherry picked from commit e0c0b2c54c)
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.
(cherry picked from commit ffdc8205e5)
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.
(cherry picked from commit 9a637aa7a4)
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.
(cherry picked from commit 91c4d9236b)
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.
(cherry picked from commit 84d6678f3b)
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.
(cherry picked from commit 6848f93842)
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.
(cherry picked from commit 6a3c294f6f)
This removes the need to `await` committers further down in the function
and allows re-using the cache for other teams later.
(cherry picked from commit c7766c637f)
This moves the parsing of ci/OWNERS into the Nix sandbox. We also get
rid of checking out the nixpkgs repo another time in the reviewers
workflow - we already have everything we need in the eval/compare job.
The creation of owners.txt in this way is only temporary, it should
eventually be moved further, similar to how maintainers.json is
currently migrating to a maintainer map for the whole repo stored on the
target branch as artifact.
(cherry picked from commit 3bef0dcf65)
All the github related logic is now bundled in `request-reviewers.sh`.
This allows moving the `get-code-owners.sh` file into the eval/compare
step in the next commit.
(cherry picked from commit 18ab6b721e)
This is just a refactor, no functional change. It is a preparation for a
future change, where `get-code-owners.sh` can be moved entirely into
eval/compare. This can only happen once we removed the remaining `gh
api` calls from it.
(cherry picked from commit d177d6057d)
Instead of requesting owners and maintainer separately, each with their
own limit of 10 review requests, we now run this together. This unties
the logic and allows easier refactoring. Also, it gives us a consistent
threshold of when not to request reviews anymore, which I set to 15.
Before, this could have been anything between 10 and 20, depending on
how the reviewers distributed over owners and maintainers.
(cherry picked from commit b2d1946eb2)
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.
(cherry picked from commit 421974863f)
By not keeping the node_id in the comments resulting from a failed
merge, these merges will be automatically retried.
(cherry picked from commit 89ace76ff1)
