Opened 8 years ago
Closed 8 years ago
#52826 closed defect (fixed)
commit notification emails don't allow replies to the committer
| Reported by: | ryandesign (Ryan Carsten Schmidt) | Owned by: | admin@… |
|---|---|---|---|
| Priority: | Normal | Milestone: | |
| Component: | server/hosting | Version: | |
| Keywords: | Cc: | raimue (Rainer Müller) | |
| Port: |
Description
Our new commit notification emails are sent from <github_username>@users.noreply.github.com, which presumably means that when I write to that address, my email will be discarded and will not be delivered to the user. This defeats the purpose of the macports-changes list, which is that we should be able to review contributions and reply to them to provide feedback. Can the notification emails be changed to come from the committer's <macports_username>@macports.org address, as they used to with Subversion?
Change History (15)
comment:1 Changed 8 years ago by raimue (Rainer Müller)
| Cc: | raimue added |
|---|
comment:2 Changed 8 years ago by ryandesign (Ryan Carsten Schmidt)
Don't we get that information when a committer logs in to Trac? We somehow manage to replace occurrences of their email addresses in Trac with their GitHub username, so we must be able to make that association already.
comment:3 Changed 8 years ago by mojca (Mojca Miklavec)
Yes, we desperately need the mapping. At the moment we have no clue whether some non-committer who submits a pull request is actually the maintainer or not. And when someone opens a ticket on github trying to mention the maintainer, it's nearly impossible to know what the maintainer's username is.
Please note that we might want to send the email to two people: author and committer of the patch. When someone approves the patch that's suboptimal, the one who pushes the changes should be notified as well.
comment:4 Changed 8 years ago by mojca (Mojca Miklavec)
I would actually say that we need that for the next macports release, so that port info would show that info as well.
comment:5 Changed 8 years ago by mojca (Mojca Miklavec)
One more thing though. Each commit message contains both the email of author and committer and those should should normally be the correct addresses. I have no idea how and by which service emails are sent; if they are sent by GitHub, we might be stuck, but if we send them with our own script, it should be straightforward to fix this problem without implementing any additional mapping. All the information is available in the commit log already.
comment:6 Changed 8 years ago by neverpanic (Clemens Lang)
The idea of having the data available in Trac initially was to only use it for the conversion, not keep it around indefinitely for other purposes. I don't like the idea of requesting users' private data, storing it, and later re-using it for different purposes, like sending their email address to a mailing list even if it is otherwise private to GitHub.
As Mojca mentioned, we could try using the committer and author addresses from the commit itself. Note that those are not necessarily valid email addresses (though in practice, they usually are). The change mails are sent from a hook called by GitHub's webhook mechanism. See https://github.com/macports/trac.macports.org/tree/master/plugins/hooks for the source.
As for the problem of knowing whether a non-committer is the maintainer of a port and assigning tickets to their maintainers in trac and on github, we should just allow GitHub usernames in the maintainer field.
comment:7 Changed 8 years ago by raimue (Rainer Müller)
GitHub will not use your @macports.org address as your committer email, but your primary email when you merge on the web interface. If we use that as sender email or in Reply-To, replies to this address and macports-dev will lead to duplicates for the recipient, as they are probably not subscribed to the list with this address.
If we want GitHub usernames in the maintainer field, we will also need this mapping for port(1). We could store it in the ports tree, either next to .mailmap or somewhere in _resources. Could even be .mailmap itself with entries mapping <cal@macports.org> <neverpanic@users.noreply.github.com>.
In any case, this will not help to map non-members.
comment:8 follow-up: 9 Changed 8 years ago by neverpanic (Clemens Lang)
Yes, it's not ideal, but it's as good as we can get without publishing data that we don't have a mandate to publish.
I disagree that we will need this mapping in port(1) to allow GitHub usernames in the maintainer field. While you can't send the maintainer an email (unless they have a public email address on GitHub or one in their commit), you can open a ticket in trac or mention them on GitHub with this information, something you can not do with the email addresses at the moment.
comment:9 follow-up: 10 Changed 8 years ago by raimue (Rainer Müller)
Replying to mojca:
Please note that we might want to send the email to two people: author and committer of the patch. When someone approves the patch that's suboptimal, the one who pushes the changes should be notified as well.
We can only have one sender address. I would not want to put people on CC and send them unnecessary notifications and Reply-To is already taken by the macports-dev address.
Replying to neverpanic:
Yes, it's not ideal, but it's as good as we can get without publishing data that we don't have a mandate to publish.
If that person's domain has a DMARC record that requires DKIM signatures, recipients could reject the email as being spoofed. This was no problem previously as we always used @macports.org where we have control over its policies. The dmarc_moderation_action on mailman would probably transform the From automatically in this case, but it needs testing.
We should continue the separate discussion whether port(1) needs this mapping on the mailing list.
comment:10 Changed 8 years ago by neverpanic (Clemens Lang)
Replying to raimue:
Replying to mojca:
Please note that we might want to send the email to two people: author and committer of the patch. When someone approves the patch that's suboptimal, the one who pushes the changes should be notified as well.
We can only have one sender address. I would not want to put people on CC and send them unnecessary notifications and Reply-To is already taken by the
macports-devaddress.
Reply-To can contain multiple addresses.
comment:11 follow-ups: 12 13 Changed 8 years ago by ryandesign (Ryan Carsten Schmidt)
If the problem is that we don't ask the user's permission to store their GitHub email address for other purposes when they log in to Trac, then let's ask for that permission.
It should come as no surprise to any MacPorts committer that when we issue them an @macports.org email address, we will use that email address to communicate with them, and that since we're an open source project and conduct our communications on public mailing lists, that email address will become public knowledge. I don't know of any committer who's complained about that before.
If the commit already contains the email address of the author and the committer, let's use that. Our policy is already that that should be the @macports.org email address. I very much want to be able to reply to the email and reach both the committer and the author. Previously, I've often had to go to the ticket to find the author's email address and manually add it to the email.
If GitHub won't use the @macports.org email address when clicking the merge button on the web site, then that sucks and I don't know what to do about it.
comment:12 Changed 8 years ago by neverpanic (Clemens Lang)
Replying to ryandesign:
If the problem is that we don't ask the user's permission to store their GitHub email address for other purposes when they log in to Trac, then let's ask for that permission.
It should come as no surprise to any MacPorts committer that when we issue them an @macports.org email address, we will use that email address to communicate with them, and that since we're an open source project and conduct our communications on public mailing lists, that email address will become public knowledge. I don't know of any committer who's complained about that before.
That's absolutely true for MacPorts commiters (and reading through the ticket it seems you're only interested in the committer of a patch, not necessarily the author?). It's a different story for the occasional contributor that sends a PR on GitHub and then sees one of their email addresses, possibly one marked private on GitHub, appear on our mailing list.
If the commit already contains the email address of the author and the committer, let's use that. Our policy is already that that should be the @macports.org email address. I very much want to be able to reply to the email and reach both the committer and the author. Previously, I've often had to go to the ticket to find the author's email address and manually add it to the email.
I agree. That data is already public with the commit and should be fine to use in virtually all cases.
If GitHub won't use the @macports.org email address when clicking the merge button on the web site, then that sucks and I don't know what to do about it.
We should take to GitHub support.
comment:13 Changed 8 years ago by raimue (Rainer Müller)
Replying to ryandesign:
It should come as no surprise to any MacPorts committer that when we issue them an @macports.org email address, we will use that email address to communicate with them, and that since we're an open source project and conduct our communications on public mailing lists, that email address will become public knowledge. I don't know of any committer who's complained about that before.
The problem is not that I do not want to use the @macports.org address, just that we do not have the pusher/committer/author's @macports.org address, the payload in the WebHook event contains *some* email address and the GitHub username, if available.
We do have the author/committer which should be the @macports.org address, but GitHub will always use the primary email address or a noreply-dummy for certain actions. Yes, this sucks, but we cannot change it.
Proposal:
- When GitHub tells us the GitHub username of an pusher/author/committer, look up their email address in the Trac database. These addresses are already published on the web interface and in Cc field of ticket notification mails. This would have the side-effect that we consistently use the same email address across all notification mails. Project members should configure their @macports.org address in Trac.
- When the author/committer does not have a GitHub username, we use whatever is stored in the commit as author/committer.
Based on this proposal, I would work on an updated trac-github-update with the following settings:
- refchange mails (those with just a summary):
From: $pusher Reply-To: macports-dev@
- commit mails (those with inline patches):
From: $author Reply-To: $committer, macports-dev@
- combined mails (a single commit was pushed):
(same as commit mails)
comment:14 Changed 8 years ago by raimue (Rainer Müller)
comment:15 Changed 8 years ago by raimue (Rainer Müller)
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
GitHub will not tell us the right email address of the pusher in the hook payload. This will either be the primary email address, or this
@users.noreply.github.comwhen the user configured their email addresses to be private on GitHub.As explained here: https://github.com/macports/trac.macports.org/blob/2cfaff7/plugins/hooks/trac-github-update.py#L86-L88
We would need a public machine-readable mapping of GitHub username to MacPorts handle to lookup the sender email address to use. So far we do not have anything like that.