wpt-pr-bot icon indicating copy to clipboard operation
wpt-pr-bot copied to clipboard
verified publisher icon web-platform-tests

WebKit export tool still asks people for reviews

Open annevk opened this issue 5 years ago • 10 comments

Recent examples:

  • https://github.com/web-platform-tests/wpt/pull/26762
  • https://github.com/web-platform-tests/wpt/pull/26765

annevk avatar Dec 07 '20 08:12 annevk

Yikes, that's definitely not meant to happen! Thanks for the report annevk.

cc @gsnedders who may be interested in supporting WebKit exports, otherwise I will try to find time to look at this over the next few weeks (but sadly cannot promise).

stephenmcgruer avatar Dec 07 '20 15:12 stephenmcgruer

This is just #100 again, no?

gsnedders avatar Dec 07 '20 17:12 gsnedders

These PRs were opened before corresponding patches were reviewed on the Bugzilla. WebKit bot has some delay, but it marked them as reviewed after r+.

I wonder if there is a way to become a collaborator on the WPT repo so I can land reviewed PRs? According to the website, I am a WebKit reviewer.

shvaikalesh avatar Dec 07 '20 19:12 shvaikalesh

This is just #100 again, no?

That code was removed in https://github.com/web-platform-tests/wpt-pr-bot/commit/e49022f32919e079616cf25255ff2bdf0eb5e739#diff-6858ab67caf6ac5fd0757dba33dfdcc84b285a36b92b03d00b9029ca8b0929b5 .

The current logic is pretty shaky, but is meant to:

  1. Comment on a PR saying its from WebKit, add labels, but not add any reviewers
  2. Approve the PR once the downstream bug is r+'d.
  3. Land the PR once the downstream bug is cq+'d (but in practice this logic has never run, I think, either because its broken or because WebKit folks land the PR manually before wpt-pr-bot has a chance).

I wonder if there is a way to become a collaborator on the WPT repo so I can land reviewed PRs?

Sorry for the delay here. I have sent you an invite to become a member of the org, which should let you land reviewed PRs. Please let me know if that doesn't grant enough permissions (we also have a reviewers team, but my vague memory is that it's actually unnecessary :D).

As to this bug in general; I will not be able to commit time to fix it, I am afraid (or to fix much of wpt-pr-bot in the short/medium term).

stephenmcgruer avatar Dec 21 '20 16:12 stephenmcgruer

Thank you @stephenmcgruer!

However, org membership doesn't grant write access (GitHub's default now I suppose), which is required to merge reviewed PRs:

Screenshot 2020-12-21 at 18 46 13

According to description of reviewers team, write access is granted to its members:

Screenshot 2020-12-21 at 18 48 18

shvaikalesh avatar Dec 21 '20 16:12 shvaikalesh

Ahah, so I remembered wrong then. Updated your invitation to be in reviewers then :)

stephenmcgruer avatar Dec 21 '20 17:12 stephenmcgruer

@dbaron mentioned this in Matrix a few days ago; discussion following that led to my conclusion that:

I think the conflict might be various systems racing; if:

  1. export-w3c-test-changes creates the PR;
  2. wpt-pr-bot runs on that PR;
  3. export-w3c-test-changes adds the WPT URL to the See Also field in Bugzilla,

then you'll get this behaviour.

gsnedders avatar Jun 23 '23 13:06 gsnedders

An obvious fix here is to drop the requirement to have the PR linked from the WebKit bug; given the number of people who can add things to that field, I don't think that is overly opening things up to abuse?

gsnedders avatar Mar 01 '24 14:03 gsnedders

I haven't seen this happen for a long time. Maybe something already fixed it?

dbaron avatar Mar 01 '24 19:03 dbaron

I haven't seen this happen for a long time. Maybe something already fixed it?

The race very much should still be there; it could just be that GitHub is slightly slower at dispatching the hook or something thus wpt-pr-bot never wins the race between steps 2 and 3.

gsnedders avatar Mar 06 '24 12:03 gsnedders