Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebDriver BiDi: Expand list of people with admin merge permissions #40860

Closed
thiagowfx opened this issue Jul 3, 2023 · 13 comments
Closed

WebDriver BiDi: Expand list of people with admin merge permissions #40860

thiagowfx opened this issue Jul 3, 2023 · 13 comments

Comments

@thiagowfx
Copy link
Member

thiagowfx commented Jul 3, 2023

Currently, due to some ongoing CI infra issues (e.g. Chromium, for Firefox I am not sure which Issue to link to), most PRs that touch files under /webdriver/tests/bidi or /webdriver/tests/support fail, and thus cannot be automatically merged.

The problematic CI jobs are often the following two:

The workflow is such that one always needs to ask one of the current admins to admin merge the PRs. Example (you'll find many more similar examples in recently merged PRs).

You can find more data points here.

This is an abnormal workflow
In fact, an RFC was event sent to make those jobs non-required (c.f. web-platform-tests/rfcs#131) -- well, at least the Chromium one, not sure about Firefox. However, the RFC wasn't yet implemented.
Ideally the CI jobs should be stable / non-flaky, so that all checks would be green, so that e.g. auto-merge would work once at least one approval is attained, without the requirement that the approver needs to be an admin.

Given that this flaky state is the current status quo for a while, I would like to ask that more people be added as admins.

Currently, amongst the current admins (https://github.com/orgs/web-platform-tests/teams/admins/members && https://github.com/orgs/web-platform-tests/teams/wpt-core-team) within the subset of people that actually work on webdriver bidi, there's at least one person from Apple (@gsnedders) and one person from Mozilla (@jgraham). There's also one person from Google (@foolip) who's currently on extended leave.

My ask is: Can we add someone else from Google (who actively works on BiDi) as admin? I am not sure what the bar is to add more admins. I don't mind volunteering myself to become one. However, if the bar is high, we could add someone else who has been around for a longer time (e.g. @mathiasbynens or @OrKoN).

In the meantime I've been asking @DanielRyanSmith to merge some of my PRs so that we reduce our bus factor, but this is not ideal and doesn't scale because although Daniel is from Google, he is not part of the BiDi team.

It's just that the current process to submit PRs from Google to WPT is quite cumbersome / slow and often takes O(days) when ideally it should take O(hours) for PRs that are not controversial, as we always need to wait for at least two serial reviews: one from a domain reviewer, and one from an admin (which only happens after the domain review is approved).
Fixing this gap would unlock the possibility to allow us to become more productive and iterate on more PRs more quickly.

@jrandolf-2
Copy link
Contributor

+1 This is a major bottleneck for development.

@mathiasbynens
Copy link
Contributor

My ask is: Can we add someone else from Google (who actively works on BiDi) as admin? I am not sure what the bar is to add more admins. I don't mind volunteering myself to become one. However, if the bar is high, we could add someone else who has been around for a longer time (e.g. @mathiasbynens or @OrKoN).

FWIW, I would be happy to take on this role if deemed worthy. I’m colocated with the team at Google working on BiDi and already-domain-approved-PRs could be sent to me where appropriate.

@gsnedders
Copy link
Member

Currently, amongst the current admins (https://github.com/orgs/web-platform-tests/teams/admins/members && https://github.com/orgs/web-platform-tests/teams/wpt-core-team) within the subset of people that actually work on webdriver bidi, there's at least one person from Apple (@gsnedders) and one person from Mozilla (@jgraham). There's also one person from Google (@foolip) who's currently on extended leave.

I don't see why we should be limiting ourselves to people who are working on WebDriver BiDi? After all—for most areas of the web platform, no admin is a direct contributor.

The reality is we have 16 admins already, which I'd hope gave us a decent number with decent timezone coverage.

@thiagowfx
Copy link
Member Author

thiagowfx commented Jul 4, 2023

I don't see why we should be limiting ourselves to people who are working on WebDriver BiDi

It's merely because the current bottleneck this issue is for is WebDriver BiDi. If an admin has to force merge anyway, it is better that the admin has at least a bit of domain knowledge to know what they are doing.

The reality is we have 16 admins already, which I'd hope gave us a decent number with decent timezone coverage.

True, but here I am assuming that not every admin is equal. If I assume I can arbitrarily ping any admin (e.g. including yourself, who happens to be familiar with BiDi, but possibly also other admins who are not as familiar) to merge my pending issues (e.g. #40814) then I can try that from now on.

But having to consistently wait for many hours/days just for the sake of having PRs merged (the aforementioned issue is one example, it was ready since yesterday night, but it wasn't yet merged - and it's not the first time) is really not ideal. We should have the ability to unblock ourselves assuming domain review is done, and we can do that by having more admins ready to review - instead of relying on a single person.

Again -- there are many ways to fix this issue. I am proposing the one that is the most practical w.r.t. speed which is to have someone in our context be admin.

Another possibility is to ping, say, 3 admins at once for every PR that is ready. But it is noisy, right? I also do not feel comfortable pinging the same person all the time, it's annoying. If they did not review, I assume they are busy or OOO, and would rather have someone else review it.

Another possibility is to introduce a SLO for admins to approve things (e.g. 24h?).

Another possibility is to make me admin (yay).

I am open to suggestions. If your suggestion is to add more admins to approved PRs (w.r.t. domain review), then I am happy to start to do that from now on. I am just hoping it won't be very noisy to ping multiple people for every PR. I am also hoping that it won't take forever.

@thiagowfx
Copy link
Member Author

@jgraham Is there a firefox bug we can follow regarding the current firefox stability job failure?

Is https://bugzilla.mozilla.org/show_bug.cgi?id=1832541 the correct bug?

@OrKoN
Copy link
Contributor

OrKoN commented Jul 12, 2023

I believe the stability bug is https://bugzilla.mozilla.org/show_bug.cgi?id=1841792

@whimboo
Copy link
Contributor

whimboo commented Aug 2, 2023

I believe the stability bug is https://bugzilla.mozilla.org/show_bug.cgi?id=1841792

And the Firefox stability failure is fixed now on this repo via #41243.

@whimboo
Copy link
Contributor

whimboo commented Aug 2, 2023

Shall we close this issue given that expanding the list of admins is not what people want?

@thiagowfx
Copy link
Member Author

Yes, fine to close it for the time being. We can reopen it in the future if needed.

@whimboo
Copy link
Contributor

whimboo commented Aug 31, 2023

The reality is we have 16 admins already, which I'd hope gave us a decent number with decent timezone coverage.

@gsnedders this doesn't seem to really work out. Beside jgraham (who is out atm), I tagged you and one other admin on a lot of PRs where admin merge is needed. Sadly so far no reply / action from anyone. Ok, I'm not sure if one of you two are off as well.

As such is there a mechanic for automatically tagging all the admins at once so that the one who sees it first can actually do the merge? I don't think it's helpful to have to CC each and every admin manually in case an admin merge is needed. Thanks.

@whimboo
Copy link
Contributor

whimboo commented Aug 31, 2023

FYI over on #41661 I tried to CC the @web-platform-tests/admins team. Not sure if that is working, but maybe that's a solution?

@thiagowfx
Copy link
Member Author

Someone merged your PR, so it seems that the alias works? Maybe we could use it from now on, whenever the usual approvers are OOO.

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

No branches or pull requests

6 participants