ci: request subsystem maintainers review

[0xda157]

Still WIP, waiting for move to nix-community

[danth]

When this is complete we can also remove the "Maintainer CC" section from the pull request template

[0xda157]

anyone know why this error is happening?

 error:
       … in the condition of the assert statement
         at /nix/store/g4ppspdl4fy7hnp4jgjl4ll03v7i08w3-source/pkgs/build-support/trivial-builders/default.nix:136:5:
          135|     # TODO: To fully deprecate, replace the assertion with `lib.isString` and remove the warning
          136|     assert lib.assertMsg (lib.strings.isConvertibleWithToString text) ''
             |     ^
          137|       pkgs.writeText ${lib.strings.escapeNixString name}: The second argument should be a string, but it's a ${builtins.typeOf text} instead.'';

       … in the left operand of the OR (||) operator
         at /nix/store/g4ppspdl4fy7hnp4jgjl4ll03v7i08w3-source/lib/asserts.nix:39:31:
           38|   # TODO(Profpatsch): add tests that check stderr
           39|   assertMsg = pred: msg: pred || builtins.throw msg;
             |                               ^
           40|

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: attribute 'name' missing

[trueNAHO]

The

nix build .#get-maintainers

command is failing with:

       error: expected a list but found a string: "<GITHUB_ID>"

[trueNAHO]

The code LGTM, although it is hard to say whether this actually works because GitHub Actions are fairly confusing sometimes.

@danth, do you think the current MAX_MAINTAINERS=5 approach is a reasonable heuristic to avoid accidental pings?

[trueNAHO]

LGTM.

Let's wait a bit longer on danth's opinion:

@danth, do you think the current MAX_MAINTAINERS=5 approach is a reasonable heuristic to avoid accidental pings?

[MattSturgeon]

FYI: I believe you can only request review for people who have some rights on the repo (triage I believe).

That's the main reason the NixOS org has a "NixOS/nixpkgs-maintainers" team, see RFC-39.

If maintainers don't join that team (or at least join the org), ofborg can't request their review on PRs.

The github docs hint at the requirements to request and/or be requested, but the phrasing is kinda vague in places.

[0xda157]

FYI: I believe you can only request review for people who have some rights on the repo (triage I believe).

That's the main reason the NixOS org has a "NixOS/nixpkgs-maintainers" team, see RFC-39.

If maintainers don't join that team (or at least join the org), ofborg can't request their review on PRs.

The github docs hint at the requirements to request and/or be requested, but the phrasing is kinda vague in places.

hmmm if/when when move to nix-community (me and danth have agreed on this, waiting for response from @trueNAHO) we could make a stylix maintainers team, not sure if it would make sense to add people as triagers on the current repo.

[MattSturgeon]

not sure if it would make sense to add people as triagers on the current repo

Triage only gives non-critical perms, like managing issues, assigning people, requesting reviews, being requestable, etc (OTTOMH).

Generally, if you trust someone to maintain a target module, you should trust them with that level of perm. If someone abuses triage perms then they shouldn't be a maintainer in the first place 🤠

You'd probably want two teams; core-maintainers and target-maintainers. Ideally core-maintainers and/or a bot would have perms to add people to the target-maintainers team, so the nix-comnunity admins don't need constant involvement.

That should be straightforward to set up.

[0xda157]

sounds like continuing with this requires @danth who has been busy recently, so this pr will await his return.

---

@trueNAHO if you could respond to that email from danth about the nix-communtiy that would be great

[danth]

do you think the current MAX_MAINTAINERS=5 approach is a reasonable heuristic to avoid accidental pings?

Yes, this seems reasonable. A single module having more than 5 active maintainers seems unlikely, and pinging people for many-module changes is what we want to avoid.

I believe you can only request review for people who have some rights on the repo

From the UI, I believe this is the case (it only gives me core maintainers and Copilot as possible choices).

We could either:

• Wait until we migrate to nix-community, and create a target maintainers team (if the nix-community admins agree to this)
• Or, change the script to just ping people in a comment rather than formally requesting a review

[danth]

Wait until we migrate to nix-community, and create a target maintainers team

@zowoq is working on automatically adding our maintainers to a team in nix-community, see nix-community/infra#1837

[zowoq]

sounds like @zowoq needs to approve the changes before it takes effect.

Done.

Would transferring ownership of the app to nix-community help with this? I can't see any other way to grant other people access to it.

I think transferring the app would allow access to other people but changes still need to be approved by by an org owner anyway.

[stylix-automation]

Successfully created backport PR for release-25.05:

• [release-25.05] ci: request subsystem maintainers review #1808