Skip to content

maintainers: make email optional #209165

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

Merged
infinisil merged 4 commits into from
Jan 27, 2023
Merged

maintainers: make email optional #209165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e39849
maintainers: make `email` optional
ncfavier Jan 5, 2023
c165d8d
maintainers: remove "noreply" email addresses
ncfavier Jan 5, 2023
93dc2de
maintainers: disallow `noreply.github.com` emails
ncfavier Jan 5, 2023
0288ee5
maintainers: clarify what fields are required
ncfavier Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/tests/maintainer-module.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ in {
type = types.str;
};
email = lib.mkOption {
type = types.str;
type = types.nullOr types.str;
default = null;
};
matrix = lib.mkOption {
type = types.nullOr types.str;
Expand Down
9 changes: 7 additions & 2 deletions lib/tests/maintainers.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# to run these tests (and the others)
# nix-build nixpkgs/lib/tests/release.nix
# These tests should stay in sync with the comment in maintainers/maintainers-list.nix
{ # The pkgs used for dependencies for the testing itself
pkgs ? import ../.. {}
, lib ? pkgs.lib
Expand All @@ -20,16 +21,20 @@ let
];
}).config;

checkGithubId = lib.optional (checkedAttrs.github != null && checkedAttrs.githubId == null) ''
checks = lib.optional (checkedAttrs.github != null && checkedAttrs.githubId == null) ''
echo ${lib.escapeShellArg (lib.showOption prefix)}': If `github` is specified, `githubId` must be too.'
# Calling this too often would hit non-authenticated API limits, but this
# shouldn't happen since such errors will get fixed rather quickly
info=$(curl -sS https://api.github.com/users/${checkedAttrs.github})
id=$(jq -r '.id' <<< "$info")
echo "The GitHub ID for GitHub user ${checkedAttrs.github} is $id:"
echo -e " githubId = $id;\n"
'' ++ lib.optional (checkedAttrs.email == null && checkedAttrs.github == null && checkedAttrs.matrix == null) ''
Copy link
Member

@infinisil infinisil Jan 5, 2023

Choose a reason for hiding this comment

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

Suggested change
'' ++ lib.optional (checkedAttrs.email == null && checkedAttrs.github == null && checkedAttrs.matrix == null) ''
'' ++ lib.optional (checkedAttrs.email == null && checkedAttrs.matrix == null) ''

There's no way to reach out to arbitrary GitHub users

Copy link
Member Author

@ncfavier ncfavier Jan 5, 2023

Choose a reason for hiding this comment

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

Can't you just @ them?

Copy link
Member Author

@ncfavier ncfavier Jan 5, 2023

Choose a reason for hiding this comment

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

Note that this change would make 62 maintainers invalid.

Copy link
Member

@infinisil infinisil Jan 5, 2023

Choose a reason for hiding this comment

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

You can't reach out to GitHub users privately though (unless I guess you create a private repository, invite the desired users to it, then create an issue and ping them). Also when you see a GitHub users profile and they have no public email address this might be their way of saying that they don't want to be contacted privately.

I guess the question is: Do we want nixpkgs maintainers to be contactable privately? While that would be nice, I don't think it's actually needed. How about just requiring them to have a GitHub account instead, ideally with the requirement that that very GitHub user makes the PR that adds them as a maintainer. Since nixpkgs is entirely public, there shouldn't be a reason to contact people privately. If you have a question regarding a package you can file an issue and ping the maintainers.

Copy link
Member Author

@ncfavier ncfavier Jan 5, 2023

Choose a reason for hiding this comment

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

Do we want nixpkgs maintainers to be contactable privately?

Hell no. We already have a security team for disclosures.

How about just requiring them to have a GitHub account instead

I'm kinda :| on that. Ideally you should be able to submit patches to nixpkgs by email. I guess there's little sense in having only a matrix ID specified though.

Copy link
Member

@infinisil infinisil Jan 27, 2023

Choose a reason for hiding this comment

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

Turns out there's only one person without an associated GitHub account:

nix-repl> :p builtins.filter (attr: ! attr ? github) (builtins.attrValues (import maintainers/maintainer-list.nix))                      
[
  { email = "davide.peressoni@tuta.io"; githubId = 3186857; matrix = "@dpd-:matrix.org"; name = "Davide Peressoni"; }
  { email = "loveisgrief@tuta.io"; keys = [ { fingerprint = "9847 4F48 18C6 4E0A F0C5  3529 E96D 1EDF A053 45EB"; } ]; name = "LoveIsGrief"; }
]

And that looks like it might be a mistake when @LoveIsGrief added themselves in #209769 (there is an obviously associated GitHub account)

While this makes this check here more strict, it wouldn't be a problem because of that

Copy link

@ghost ghost Jan 27, 2023

Choose a reason for hiding this comment

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

I'm not sure I understand the mistake. The file header says that email and name are required https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix#L4 . I followed what it says.

IMO, this PR isn't complete if maintainers-list.nix keep the header with the email+name requirements, if the email is rendered optional.

Copy link
Member Author

@ncfavier ncfavier Jan 27, 2023

Choose a reason for hiding this comment

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

Thanks for pointing that out, that comment should indeed be updated to stay in sync with whatever check we decide to settle on.

Regardless, we should also mention that setting a GitHub ID allows ofborg to request your review automatically when a package you maintain is modified.

Copy link
Member

@infinisil infinisil Jan 27, 2023

Choose a reason for hiding this comment

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

Regardless of the other ideas here, I think this check in this PR is good as is. The only thing to do is to update the comment in the maintainer listing

Copy link
Contributor

@rnhmjoj rnhmjoj Jan 27, 2023

Choose a reason for hiding this comment

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

Turns out there's only one person without an associated GitHub account:

That's because they have all been removed in #178656

Famously, there was @volth that received a full damnatio memoriae by github but contributed patches using the discourse forum, I think.

echo ${lib.escapeShellArg (lib.showOption prefix)}': At least one of `email`, `github` or `matrix` must be specified, so that users know how to reach you.'
'' ++ lib.optional (checkedAttrs.email != null && lib.hasSuffix "noreply.github.com" checkedAttrs.email) ''
echo ${lib.escapeShellArg (lib.showOption prefix)}': If an email address is given, it should allow people to reach you. If you do not want that, you can just provide `github` or `matrix` instead.'
'';
in lib.deepSeq checkedAttrs checkGithubId;
in lib.deepSeq checkedAttrs checks;

missingGithubIds = lib.concatLists (lib.mapAttrsToList checkMaintainer lib.maintainers);

Expand Down
Loading