danth
ci: review request maintainers
When this is complete we can also remove the "Maintainer CC" section from the pull request template
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
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.
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.
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.
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
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
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 https://github.com/nix-community/infra/pull/1837
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 🤠
Generally, our module maintainers are supposed to be the first people we ask for help when working on their module, like for testing their module. Consequently, we are basically adding anyone as module maintainer that is willing to help us out. I don't think we should carelessly give permissions away.
Instead, the following might be better:
- Or, change the script to just ping people in a comment rather than formally requesting a review
Consequently, we are basically adding anyone as module maintainer that is willing to help us out.
This is no different from nixpkgs maintainers, which operate in the same way.
They have an order of magnitude more maintainers with triage permission (3'708 currently). Have they ran into (major) issues caused by it?
I don't think we should carelessly give permissions away.
I agree, you shouldn't carelessly dish out permissions. Instead, you should consider what damage these permissions can actually do in practice.
Triage: Recommended for contributors who need to proactively manage issues, discussions, and pull requests without write access
What they can do is close issues, PRs, and discussions; request reviews; submit reviews...
However, they cannot merge PRs; approve PRs; push to unprotected branches; "hide" comments; etc.
See Permissions for each role.
If someone abuses any of those permissions, it will be fairly obvious; issues closed when they shouldn't be; etc. In such a case it'll be easy to a) re-open the issue and b) tell the maintainer not to close issues in that particular scenario.
If someone continues to abuse the permission, they can be kicked from the team and/or removed as a maintainer.
On the flip side, empowering maintainers to manage issues may actually be helpful; you'll not need to spend as much time triaging target-specific issues, as you'll have "more hands" available to help out.
Consequently, we are basically adding anyone as module maintainer that is willing to help us out.
This is no different from nixpkgs maintainers, which operate in the same way.
They have an order of magnitude more maintainers with triage permission (3'708 currently). Have they ran into (major) issues caused by it?
I don't think we should carelessly give permissions away.
I agree, you shouldn't carelessly dish out permissions. Instead, you should consider what damage these permissions can actually do in practice.
Triage: Recommended for contributors who need to proactively manage issues, discussions, and pull requests without write access
What they can do is close issues, PRs, and discussions; request reviews; submit reviews...
However, they cannot merge PRs; approve PRs; push to unprotected branches; "hide" comments; etc.
See Permissions for each role.
If someone abuses any of those permissions, it will be fairly obvious; issues closed when they shouldn't be; etc. In such a case it'll be easy to a) re-open the issue and b) tell the maintainer not to close issues in that particular scenario.
If someone continues to abuse the permission, they can be kicked from the team and/or removed as a maintainer.
On the flip side, empowering maintainers to manage issues may actually be helpful; you'll not need to spend as much time triaging target-specific issues, as you'll have "more hands" available to help out.
Good point. In that case, are waiting on https://github.com/nix-community/infra/pull/1837:
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 https://github.com/nix-community/infra/pull/1837
Or should we
change the script to just ping people in a comment rather than formally requesting a review
in the meantime? I don't think this PR has a high priority, which means we can just wait on https://github.com/nix-community/infra/pull/1837.
@trueNAHO made some changes to the nix side but don't really understand jq, could you help me update the bash/jq part of this pr?
The configuration file (path: .github/labeler.yml) was not found locally, fetching via the api Error: HttpError: You do not have permission to create labels on this repository.: {"resource":"Repository","field":"label","code":"unauthorized"} Error: You do not have permission to create labels on this repository.: {"resource":"Repository","field":"label","code":"unauthorized"}This error seems unrelated to this PR and might have been somehow introduced by commit c4f87ef ("ci: add more top-level labels to provide better overview (#1755)"). Maybe
git rebase masterresolves the issue?Weird that the third CI attempt was successful. Maybe it has something to do with the fact that I added and adjusted the descriptions of the new labels in the meantime... Either way, LGTM.
The problem was I forgot to add the new labels after merging #1755. I did add them when I was working on testbed stuff last night, but the failing labeling ci needs to be manually rerun.
This shouldn't be merged until @danth has confirmed he made the required changes to https://github.com/apps/stylix-automation (or makes it possible for NAHO or me to make those changes ourself).
I've updated the permissions on the app, sounds like @zowoq needs to approve the changes before it takes effect.
or makes it possible for NAHO or me to make those changes ourself
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.
sounds like @zowoq needs to approve the changes before it takes effect.
Done.
Would transferring ownership of the app to
nix-communityhelp 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.
