Skip to content

Comments

maintainers: make email optional#209165

Merged
infinisil merged 4 commits intoNixOS:masterfrom
ncfavier:maintainers-email
Jan 27, 2023
Merged

maintainers: make email optional#209165
infinisil merged 4 commits intoNixOS:masterfrom
ncfavier:maintainers-email

Conversation

@ncfavier
Copy link
Member

@ncfavier ncfavier commented Jan 5, 2023

Not giving an email address is fine as long as the maintainer is reachable through other means, such as GitHub or Matrix. On the other hand, an @noreply.github.com/@users.noreply.github.com email is useless.

This does not break https://search.nixos.org because we already treat email as optional.

@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 Jan 5, 2023
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
'' ++ 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

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

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

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

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

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

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

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

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

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.

@Mindavi Mindavi added the 9.needs: community feedback This needs feedback from more community members. label Jan 6, 2023
@SuperSandro2000 SuperSandro2000 added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Jan 23, 2023
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/contribute-without-a-github-account/25007/1

@ncfavier
Copy link
Member Author

Rebased on master and updated the comment in maintainers-list.nix

Not giving an email address is fine as long as the maintainer is
reachable through other means, such as GitHub or Matrix.
If an email address is given, the maintainer should be reachable through it.
@infinisil infinisil merged commit 8a828fc into NixOS:master Jan 27, 2023
@ncfavier ncfavier deleted the maintainers-email branch January 27, 2023 18:57
@zowoq
Copy link
Contributor

zowoq commented Feb 1, 2023

On the other hand, an @noreply.github.com/@users.noreply.github.com email is useless.

It was useful if I wanted to set an obvious "I'm not contactable by email" email address.

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.

Any suggestions on what I should use now so I can restore my repology list and feed?

https://repology.org/projects/?maintainer=59103226%2Bzowoq%40users.noreply.github.com&inrepo=nix_unstable
https://repology.org/maintainer/59103226%2Bzowoq%40users.noreply.github.com/feed-for-repo/nix_unstable

ncfavier added a commit to ncfavier/repology-updater that referenced this pull request Feb 1, 2023
A recent change (NixOS/nixpkgs#209165) removed
`noreply.github.com` email addresses from the maintainer list, which
means that maintainers using only a GitHub account are not indexed by
repology any more, so we now need to extract GitHub accounts as well.
@ncfavier
Copy link
Member Author

ncfavier commented Feb 1, 2023

Oops, I didn't think of that, sorry. I've opened a PR at repology to also index GitHub accounts. In the meantime, feel free to revert the noreply check and re-add your email.

@ncfavier
Copy link
Member Author

ncfavier commented Feb 1, 2023

It was useful if I wanted to set an obvious "I'm not contactable by email" email address.

I'd say "no email address at all" is even more obvious.

@zowoq
Copy link
Contributor

zowoq commented Feb 1, 2023

I've opened a PR at repology to also index GitHub accounts.

Thanks.

In the meantime, feel free to revert the noreply check and re-add your email.

I can live without it for a bit, I'll give the repology maintainers some time to review at the PR and go from there.

It was useful if I wanted to set an obvious "I'm not contactable by email" email address.

I'd say "no email address at all" is even more obvious.

I don't understand? Email address was a required field prior to this PR?

@ncfavier
Copy link
Member Author

ncfavier commented Feb 1, 2023

I don't understand? Email address was a required field prior to this PR?

Yes, you're right: it was useful given that requirement, but isn't any more.

@SuperSandro2000
Copy link
Member

I can live without it for a bit, I'll give the repology maintainers some time to review at the PR and go from there.

Yeah, lets wait a few days and not hastily revert things. They need to get to nixos-unstable first anyway to be useful.

AMDmi3 pushed a commit to repology/repology-updater that referenced this pull request Feb 2, 2023
A recent change (NixOS/nixpkgs#209165) removed
`noreply.github.com` email addresses from the maintainer list, which
means that maintainers using only a GitHub account are not indexed by
repology any more, so we now need to extract GitHub accounts as well.
@ncfavier
Copy link
Member Author

ncfavier commented Feb 3, 2023

Here you go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

9.needs: community feedback This needs feedback from more community members. 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. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants