search

web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
5k stars 3.1k forks source link

chromium-wpt-export-bot needs to give GitHub handles of those involved in the patch #8434

Open annevk opened 6 years ago

annevk commented 6 years ago

I don't need them pinged, but I do want the ability to ping them without lots of work on my part. E.g., https://github.com/w3c/web-platform-tests/pull/8431 is wrong and I've said as much in https://chromium-review.googlesource.com/c/chromium/src/+/784653, but it'd be nice if I could convey that in GitHub instead.

It would be good if we made it easier to appeal upstreamed changes somehow.

annevk commented 6 years ago

(Perhaps not pinging those specific people, but a "this needs to be reverted" command that does the appropriate thing would also work. Not sure what works best for people.)

jgraham commented 6 years ago

Does reverting the patch in GH not have the right effect? I think they will pick up the revert next time they do a landing. Of course that doesn't solve the communication problem.

annevk commented 6 years ago

Yeah, it doesn't matter where it's done, but I do think it would be good for those who made the mistake to correct it and so being able to notify the people responsible (both author(s) and reviewer(s)) seems useful.

jgraham commented 6 years ago

From a Mozilla pov, trying to work out GitHub handles for an unbounded number of developers doesn't seem likely to work. Of course it's possible to have some kind of map so that common cases are well handled, but commenting on the original bug might be more effective than assuming people pay a lot of attention to their GH email (in particular I think that the world is approximately divided into people who live in their GH notifications and people who regard most GH email as spam).

foolip commented 6 years ago

There's no guarantee that the original authors and reviewers are on GitHub, but I agree it should be possible to give feedback without a lot of manual effort, so I'm calling this roadmap to either figure something out, or come back here and explain why we didn't.

foolip commented 6 years ago

@Hexcles, WDYT?

Hexcles commented 6 years ago

Actually, there is a perhaps easy solution to this: encourage people to add their work emails (mozilla or chromium) to GitHub, and GitHub will automatically figure out the user from the email of the author of a patch. A random example like #8404. Then if you reply in that PR, the person will get be notified by GitHub (provided that they did not mute all the notifications).

foolip commented 6 years ago

@Hexcles I'm not sure that actually works, because @chromium-wpt-export-bot is the one creating the PR and the committer. Can you try commenting a bit on https://github.com/w3c/web-platform-tests/pull/9107 so I can see what kind of email I get? The reason I think it doesn't work is that I haven't been getting any spam from @w3c-bots, which would have greatly annoyed me :)

Because of @w3c-bots, I'm also a bit skeptical that we can make just commenting on the PR notify the author. That'd require being able to post GitHub comments without causing notification mail, and using that by all bots.

@annevk, what is the workflow you'd want here? To be able to just comment? A list of handles that aren't notified but you can copy to a comment? Doesn't matter much?

annevk commented 6 years ago

Ideally I could just complain in a merged/open PR and not have to track it myself afterwards to make sure something actually gets done. I'd be open to opening a new issue, but I'm not sure we do good enough triage on those.

foolip commented 6 years ago

My experience is that most people miss GitHub email very easily for a variety of reasons, so anything that just notifies people using the normal way probably won't have a high success rate.

This sounds like overkill, but maybe we could automatically add a label to all issues that get comments by humans, and have a triage for that label? @Hexcles, other ideas?

Hexcles commented 6 years ago

@foolip Just commented. Did you get any email?

The reason I think it works is that the author is also listed as a participant of the PR on the right, along with the bots. And I do get emails from @w3c-bots (once per PR, as the bot updates the comment instead of creating new comments).

One point I forgot is that even if a Chromium engineer adds their email to their GitHub account, they may not route the notification correctly, and emails end up in their primary (personal) address.

foolip commented 6 years ago

Nope, I didn't get any email, as expected. And I do redirected GitHub email so that repos in the w3c org end up with my work email.

alijuma commented 6 years ago

Should this still have priority:roadmap? Who's a good owner for figuring out if something can be done here?

foolip commented 6 years ago

https://github.com/w3c/web-platform-tests/pulls?utf8=%E2%9C%93&q=is%3Amerged+is%3Apr+label%3Achromium-export+comments%3A%3E2 is the list of issues where there was maybe some non-bot activity, made hard to tell because of inconsistent number of comments by @w3c-bots.

Seems like most are us struggling to get stuff exported where we don't want to bother the original CL authors, so I think any solution for this would have to be listing the handles in a way where they aren't pinged by default.

I don't know where that leaves except without a clear path forward, so I'm going to downgrade the priority. (Is that backwards? Maybe.)

If people keep having this problem then I think we should revisit. @annevk, has it continued to be a problem for you?

annevk commented 6 years ago

I haven't been actively reviewing new commits.

foolip commented 6 years ago

The volume of changes across your specs is probably too high for that, yeah.

It would be useful for someone sometime, but let's wait to see more requests then.