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.

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

workflows/eval: Request reviews from changed package maintainers #366046

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

infinisil
Copy link
Member

Request reviews from maintainers of changed packages, mirroring what ofborg does, but without having to wait for it anymore. Part of #355847

Things done


This work is funded by Tweag and Antithesis

Add a 👍 reaction to pull requests you find important.

@infinisil infinisil added the backport release-24.11 Backport PR automatically label Dec 18, 2024
@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 Dec 18, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 18, 2024
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.
…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.
@infinisil
Copy link
Member Author

infinisil commented Dec 18, 2024

Feedback addressed, still works: Infinisil-s-Test-Organization#37 (comment)

Copy link
Contributor

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eval "Process" seems to be failing.

@infinisil
Copy link
Member Author

infinisil commented Dec 19, 2024

@JohnRTitor A bit unfortunate but unfixable here, because the workflow for this PR itself uses the version from the base branch, but fetches the code from the PR, which now takes an extra argument not passed in the workflow

@Mic92
Copy link
Member

Mic92 commented Dec 19, 2024

@JohnRTitor A bit unfortunate but unfixable here, because the workflow for this PR itself uses the version from the base branch, but fetches the code from the PR, which now takes an extra argument not passed in the workflow

This is the reference to look at: Infinisil-s-Test-Organization#37 (comment) instead

Comment on lines +272 to +275
jq -r 'keys[]' comparison/maintainers.json \
| while read -r id; do gh api /user/"$id"; done \
| jq -s '{ reviewers: map(.login) }' \
> reviewers.json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
jq -r 'keys[]' comparison/maintainers.json \
| while read -r id; do gh api /user/"$id"; done \
| jq -s '{ reviewers: map(.login) }' \
> reviewers.json
jq -r 'keys[]' comparison/maintainers.json \
| head -n15 \
| while read -r id; do gh api /user/"$id"; done \
| jq -s '{ reviewers: map(.login) }' \
> reviewers.json

GitHub apps are limited to 15 reviewer requests anyway, and this way treewide changes are handled better. Adding a head -n15 here should work, but I did not test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea. Should be the list sorted, so it's always the same 15 reviewers?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see two other options to handle treewides / PRs with more than 15 reviewers:

  • Don't request reviewers in this case.
  • Instead of requesting review, post a comment with all maintainers mentioned for this case.

I actually prefer the first option: When you make a treewide change and you get some random 15 reviewers, chances are high that those reviewers:

  • are not responding anyway
  • don't have much to say about that treewide change

So why notify many random people?

(Note that for a treewide, we very likely have already requested reviews from a bunch of people via OWNERS!)

For treewide changes it makes much more sense to pick your reviewers individually, specifically for that change.

Comment on lines +227 to +228
# See ./codeowners-v2.yml, reuse the same App because we need the same permissions
# Can't use the token received from permissions above, because it can't get enough permissions
Copy link
Contributor

@wolfgangwalther wolfgangwalther Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #366046 (comment) @FliegendeWurst mentioned that a single app can only request review from 15 reviewers.

How does this then interact with the codeowners reviewer request?

We should make sure that codeowners are always requested - and are not silently ignored because of the reviewer limit when the maintainers had been requested before.

We probably need two apps here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The limit only applies per run, but I'm not sure. And codeowners is another actions run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The limit only applies per run, but I'm not sure. And codeowners is another actions run.

In #361878 the github-actions bot requested review from more than 15 reviewers across multiple runs - that seems to support what you say.

Comment on lines +56 to +83
relevantFilenames =
drv:
(lib.lists.unique (
builtins.map (pos: lib.strings.removePrefix (toString ../..) pos.file) (
builtins.filter (x: x != null) [
(builtins.unsafeGetAttrPos "maintainers" (drv.meta or { }))
(builtins.unsafeGetAttrPos "src" drv)
# broken because name is always set by stdenv:
# # A hack to make `nix-env -qa` and `nix search` ignore broken packages.
# # TODO(@oxij): remove this assert when something like NixOS/nix#1771 gets merged into nix.
# name = assert validity.handled; name + lib.optionalString
#(builtins.unsafeGetAttrPos "name" drv)
(builtins.unsafeGetAttrPos "pname" drv)
(builtins.unsafeGetAttrPos "version" drv)

# Use ".meta.position" for cases when most of the package is
# defined in a "common" section and the only place where
# reference to the file with a derivation the "pos"
# attribute.
#
# ".meta.position" has the following form:
# "pkgs/tools/package-management/nix/default.nix:155"
# We transform it to the following:
# { file = "pkgs/tools/package-management/nix/default.nix"; }
{ file = lib.head (lib.splitString ":" (drv.meta.position or "")); }
]
)
));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is an extension on top of the current ofborg behavior, but maybe a good time to discuss it:

Can we extend the "relevant file names" for a derivation to include some dependent files like patches or setup hooks?

I remember having to look up maintainers from the derivation manually when changing the setup-hook.sh.

I think some basic rules could be:

  • if the path ends in /default.nix or the path ends in /package.nix and is in /by-name/, then
  • add all files in the same directory (recursively) to the list of relevant files.

A random example that would benefit from it:

pkgs/by-name/me/meson/package.nix would include changes to all the patch and .sh files in the same folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions 6.topic: policy discussion 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux backport release-24.11 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants