| Age | Commit message (Collapse) | Author |
|---|
|
master/release-* branches
|
|
|
|
Dismissals are done automatically by commits.js, even for reviews from
check-target-branches.js. This is not desirable.
The solution is
(1) do not decline to post a review because it was already dismissed
(because it may have not been dismissed by a human, and circumstances may
have changed), and
(2) reword the auto-dismissal message to not imply that whatever
problems were present are fixed.
|
|
This restricts github automatically merging master with commit message
"Merge branch 'NixOS:master' into ...". Although we don't explicitly
prohibit not space after colons, we don't use it in any of the examples.
It's also enforced in https://www.conventionalcommits.org/en/v1.0.0/
|
|
Follow-up to 7cf59724104ef43e73e96176acbb8a5be79b1921
While the JS script already returned early, we can save a few resources
by skipping the job entirely when there's no `pull_request` context.
|
|
|
|
This is an experiment and can be reverted a few days from now; if
the results are positive on GitHub’s end, then we may want to make
the merge conflict checks run less frequently than the rest of the
labelling tasks.
|
|
|
|
Per matrix.
|
|
I fixed this for the maintainer maps, but the artifact that was causing the
particular issue that prompted me to try to fix it was actually the
pagination cursor. So fix that too.
Related: #464046.
|
|
Context: #464046
|
|
Should fix https://github.com/nixos/nixpkgs/pull/463686#issuecomment-3563126753
|
|
Should help debug "Review cannot be requested from pull request author."
in https://github.com/NixOS/nixpkgs/actions/runs/19591357890/job/56110301046#step:6:4726.
|
|
Should prevent "Unhandled error: HttpError: Artifact has expired", as was
present in e.g. https://github.com/NixOS/nixpkgs/actions/runs/19594659032.
|
|
The prepare script is currently failing for staging-25.11 PR's, because
it assumes that a release-25.11 exists respectively. This is not the
case in the transition phase before branch-off.
We can fix this by always including the current target branch in the
branches to check for, even if it's not a WIP branch. This means we
might check some branches twice, but that's better than erroring out
entirely when the branch is in fact correct.
|
|
This reverts commit 402b41c1257ffc7cb88f2876b5fa20ab3ffe4f95.
GitHub' API repeatedly returns wrong data which causes closed PRs when
the changes had not been merged, yet.
We have closed a bit more than 100 PRs overall, most of them initially -
the feature is not really that important overall.
|
|
It makes not much sense to run all the checks for PRs when we can
already tell they are stale beforehand. In particular this should avoid
creating ~3.3k temporary merge commits every day, for PRs that surely
won't have had any change.
The number of merge commits *could* play a role in the growing size of
the fork network. We'll have GitHub look into the metrics before and
after this change to see whether that is any improvement.
|
|
|
|
|
|
|
|
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.
|
|
This lead to reaching secondary API limits in a treewide recently, so we
better limit it to where we actually need it.
|
|
We still use a few too many API requests by checking team members for
collaborator status - we can improve on that in the future.
|
|
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 should fix the bug where the "needs: reviewer" label was set too
early, just to be removed immediately, because reviewers were then
requested.
|
|
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 partially reverts commit 1197fe48da1d45506a3e95702ce5fb6437368598.
GitHub just doesn't schedule these narrow intervals. 10 minutes is
alright in practice.
|
|
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.
|
