search

web-platform-tests / wpt-pr-bot

Home of the wpt-pr-bot
Apache License 2.0
29 stars 36 forks source link

WebKit export tool still asks people for reviews #164

Open annevk opened 3 years ago

annevk commented 3 years ago

Recent examples:

stephenmcgruer commented 3 years ago

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).

gsnedders commented 3 years ago

This is just #100 again, no?

shvaikalesh commented 3 years ago

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.

stephenmcgruer commented 3 years ago

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).

shvaikalesh commented 3 years ago

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
stephenmcgruer commented 3 years ago

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

gsnedders commented 1 year ago

@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 commented 6 months ago

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?

dbaron commented 6 months ago

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

gsnedders commented 6 months ago

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.