Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Backport release-24.11] workflows/eval: Request reviews from changed package maintainers #370709

Merged
merged 19 commits into from
Jan 11, 2025
Merged

[Backport release-24.11] workflows/eval: Request reviews from changed package maintainers #370709

merged 19 commits into from
Jan 11, 2025

Conversation

nix-backports[bot]
Copy link

@nix-backports nix-backports bot commented Jan 3, 2025 •
edited by wolfgangwalther
Loading

Bot-based backport to release-24.11, triggered by a label in #366046.

  • Before merging, ensure that this backport is acceptable for the release.
    • Even as a non-commiter, if you find that it is not acceptable, leave a comment.

Includes the following PRs:

@infinisil @github-actions
workflows/eval: Request reviews from changed package maintainers
db45da1 
Currently we need to rely on ofborg requesting reviews from package
maintainers, which takes a while with ofborg's eval queue. Since
recently we're doing faster evaluations with GitHub Actions, which contain all
necessary information to determine reviewers of changed packages the
same way ofborg does. This PR takes advantage of that.

(cherry picked from commit b9d800d)
@infinisil @github-actions
workflows/eval: Use maintainer GitHub IDs for review requests of chan…
0916ec4 
…ged packages

The handles can change over time and there's nothing guaranteeing the
ones in the maintainer list are up-to-date. In comparison GitHub IDs
never change.

(cherry picked from commit b844cba)
@nix-backports nix-backports bot mentioned this pull request Jan 3, 2025
Merged
1 task
@github-actions github-actions bot added 6.topic: policy discussion 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions labels Jan 3, 2025
@infinisil @wolfgangwalther
ci/request-reviews: split off a more reusable reviewer processing script
d29ca81 
We can reuse the new process-reviewers.json part for requesting
reviews from maintainers.

(cherry picked from commit 0371b7f)
@infinisil @wolfgangwalther
workflows/eval: Reuse process-reviewers.sh
b4b6dde 
Filters out the PR author and avoids rerequesting reviews from people
that already left a review. In a future commit, this can be expanded to
also avoid requesting reviews from people not in the org

(cherry picked from commit 0ebab0b)
@wolfgangwalther wolfgangwalther mentioned this pull request Jan 3, 2025
Merged
3 tasks
@wolfgangwalther
Copy link
Contributor

We get this in "Eval / Process":

error: cannot evaluate a function that has an argument without a value ('touchedFilesJson')
       Nix attempted to evaluate a function as a top level expression; in
       this case it must have its arguments supplied either by default
       values, or passed explicitly with '--arg' or '--argstr'. See
       https://nixos.org/manual/nix/stable/language/constructs.html#functions.
       at /home/runner/work/nixpkgs/nixpkgs/nixpkgs/ci/eval/compare/default.nix:11:3:
           10|   afterResultDir,
           11|   touchedFilesJson,
             |   ^
           12| }:

Do we need to backport something else as well?

@wolfgangwalther
Copy link
Contributor

Ah, what happens here: The first commit adds the touchedFilesJson argument, but runs the command against the base branch. This can only pass after merging. I assume the original PR failed the same way.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 3, 2025
@getchoo getchoo added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jan 3, 2025
@wolfgangwalther
Copy link
Contributor

Marked as draft until the issues that came up in #370456 are resolved.

@wolfgangwalther wolfgangwalther marked this pull request as draft January 4, 2025 13:54
infinisil and others added 3 commits January 4, 2025 14:55
@infinisil @wolfgangwalther
workflows/eval: Minor cleanup
0fb6e88 
The ${{ }} syntax is best avoided in scripts. While it wouldn't be a
problem here, let's do this for consistency

(cherry picked from commit ab248be)
@infinisil @wolfgangwalther
ci/request-reviews: Don't request reviews from non-repo-collaborators
1b7595a 
Fixes this problem for maintainer-based reviews when the maintainer
didn't yet accept or missed the automated invite:

    gh: Reviews may only be requested from collaborators. One or more of
the users or teams you specified is not a collaborator of the
NixOS/nixpkgs repository. (HTTP 422)

(cherry picked from commit 077007a)
@wolfgangwalther
ci/request-reviews: lowercase handles consistently
58e9df7 
This script assumed to get lowercased input before, but with the
addition of pinging maintainers that's not necessarily true anymore.

Since the checks for prAuthor and already-reviewed-by already lowercase,
make sure to lowercase the handles in the users array, too.

(cherry picked from commit 213dbf1)
@wolfgangwalther wolfgangwalther force-pushed the backport-366046-to-release-24.11 branch from fd28da3 to 58e9df7 Compare January 4, 2025 13:55
@wolfgangwalther
ci/request-reviews: rename code-owner related files
ff1e788 
Now that we have maintainer reviews as well, be a bit more explicit
about naming.

(cherry picked from commit cf0616f)
@wolfgangwalther
ci/request-reviews: use generic wording instead of "code owner"
d82f7fa 
This is now re-used for both code owners and maintainers.

(cherry picked from commit ffb0ace)
@wolfgangwalther
workflows/eval: make "requesting maintainer reviews" separate step
981355c 
Odd to have this in the "Tagging pull request" step, which is only about
labels otherwise.

(cherry picked from commit 2e61194)
@wolfgangwalther
ci/request-reviews: share code to request reviewers from gh api
bc3c6a9 
This makes it easier to add ofborg's request-1-by-1 logic, where failed
requests are OK for edge cases.

(cherry picked from commit 62779fb)
@wolfgangwalther
ci/request-reviews: request reviewers 1-by-1
53d0959 
This is to be able to ignore the odd failure for some users, who are
listed as collaborators, but still fail to be requested properly.

(cherry picked from commit 034613f)
@wolfgangwalther wolfgangwalther mentioned this pull request Jan 4, 2025
Merged
13 tasks
@wolfgangwalther
ci/request-reviews: limit to 10 maintainer review requests
982e96f 
This mirrors ofborg for now.

(cherry picked from commit 240c82b)
@wolfgangwalther wolfgangwalther mentioned this pull request Jan 5, 2025
Merged
13 tasks
@wolfgangwalther
workflows/eval: add eval summary before requesting reviewers
410510d 
This is to ensure that the eval summary is still set as commit status,
even when the review requests fail due to too many reviewers.

(cherry picked from commit 3c9794d)
@wolfgangwalther
Copy link
Contributor

We are still working on some things around maintainer reviews, but the current state is still ready to be backported imho. This will already be an improvement to get back maintainer reviews for backport PRs.

@wolfgangwalther wolfgangwalther marked this pull request as ready for review January 11, 2025 11:48
JohnRTitor and others added 5 commits January 11, 2025 14:41
@JohnRTitor @wolfgangwalther
actions/no-channel-pr: re run when base branch is updated
8ccc1cf 
Additionally, no permissions are needed so set it to an empty set
Signed-off-by: John Titor <50095635+JohnRTitor@users.noreply.github.com>
(cherry picked from commit 3ff50a2)
@trueNAHO @wolfgangwalther
workflows: lock Ubuntu runner to ubuntu-22.04
fae0c7b 
Lock the Ubuntu runner to ubuntu-22.04 to avoid accidental updates [1]
and increase reproducibility.

[1]: actions/runner-images#10636

(cherry picked from commit 2660dd1)
@trueNAHO @wolfgangwalther
workflows: update Ubuntu runner to ubuntu-24.04
1a7793d 
Link: actions/runner-images#10636
(cherry picked from commit fcfbe6f)
@trueNAHO @wolfgangwalther
workflows: lock macOS runner to macos-14
c2ab54c 
(cherry picked from commit dbd20ce)
@JohnRTitor @wolfgangwalther
workflows/eval: add swap to tackle recent borderline OOM
a6ea384 
Removed in #356023 (comment)

Due to #355847 (comment) #355847 (comment) #355847 (comment)

(cherry picked from commit 9ccdc41)
@wolfgangwalther
Copy link
Contributor

I added three more PRs (see description). Those are not directly related to review requests, but this makes it easier to backport other changes in the future.

I left out any changes to the periodic-merge workflows, because those run on a schedule and thus only from the master anyway. But I had to resolve some merge conflicts for those files, so the "check cherry-picks" should turn up red.

This was referenced Jan 11, 2025
Merged
Merged
Merged
@wolfgangwalther wolfgangwalther merged commit 1925ad6 into release-24.11 Jan 11, 2025
18 of 20 checks passed
@wolfgangwalther wolfgangwalther deleted the backport-366046-to-release-24.11 branch January 11, 2025 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@getchoo getchoo getchoo approved these changes

@wolfgangwalther wolfgangwalther wolfgangwalther approved these changes

@infinisil infinisil Awaiting requested review from infinisil

@LeSuisse LeSuisse Awaiting requested review from LeSuisse

@Mic92 Mic92 Awaiting requested review from Mic92

@mweinelt mweinelt Awaiting requested review from mweinelt

@philiptaron philiptaron Awaiting requested review from philiptaron

@risicle risicle Awaiting requested review from risicle

@zowoq zowoq Awaiting requested review from zowoq

Assignees
No one assigned
Labels
6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions 6.topic: policy discussion 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wolfgangwalther @getchoo @wegank @infinisil @JohnRTitor @trueNAHO