Improved Gerrit access/experience for non-Googler SDK contributors

[parlough]

The dart-lang/sdk Gerrit review and submission process is one that (understandably) has limitations for external contributors. It has however recently gotten harder with the requirement for approvals from two googlers. Enforcing the OWNERS file has made this even harder.

A few common scenarios/issues, both old and new:

1. You are new to contributing to the SDK and open a CL. You might not know labeling practices or contribution standards, so you don't assign a reviewer and no one ever sees it in their triage process. So it sits there awkwardly.
   • Even if you know to assign reviewers. Who do you assign it to? One, two, all of the OWNERS for your changes? Non-googlers can't easily determine who works on what or if that person is on leave or whatever. It also feels bad to assign a reviewer, like you're adding to their work, so you may not want to.
2. Open a CL. You can't run (or rerun after fail or changes) the try bots, and need to ask someone else to do so.
3. CL is approved but reviewers don't submit (CQ+2) the change since they think you can do it. Now you need to ping and distract them again to submit the change or else it will sit there. If there's an issue or merge conflict, you then need to do the whole asking and waiting process again.
4. CL is approved by one Googler, even if simple, but you now need to find a second Googler.

These types of issues (and more) exist for all external contributors. Perhaps there's no perfect solution for all of these depending on Google's requirements, but a special group like Flutter hackers could maybe help for the most common cases. It could contain contractors, nominated long-term contributors, past Google employees that continue contribution, etc.

I imagine this group would benefit from some or all of the following, depending on what is permissible by Google's policies:

• Can run their desired try bots
• Can submit their own changes that have the necessary approvals
• Only need one Googler approval
• Not as necessary: Ability to LGTM isn't as important, especially since OWNERs are used now.

Other potential/alternative improvements, that would help all external contributors:

If a group like that is not allowed/possible, some additions to the Gerrit UI for (Google) reviewers of non-Google CLs could be helpful. The changes would also help external contributors not part of a trusted group:

• When leaving a review on a CL, if there isn't another reviewer added, it should prompt to add one. This is especially important for external contributors who may not know who to request a review from. Autoassigning reviewers based on OWNERS when a non-Google CL is opened is also an option.
• Could notify the viewer/reviewer to run the related try bots, since the author can't.
• After the second approval, it could ask that approver if they want to submit the change (since the author can't).
• Edit (Added): Relating to @Hixie's mentions below, more standard and frequent CL triage processes by teams could help some of these issues.

[parlough]

\cc @Hixie In case you have anything to suggest, since I know you've raised similar issues before, and have experience with the Flutter hackers group.

[Hixie]

This is exacerbated by other issues, for example, there's no discussion forum (e.g. IRC, Discord, whatever) where you can reach out to team members to get reviews, and there's no CL triage process to ensure that CLs get looked at. So CLs just sit there unreviewed (e.g. I submitted a CL back in August to see what would happen, and nobody's seen it as far as I know), and there's no real process that I can see to do anything about it.

[athomas]

I've started to address some of these issues in https://dart-review.googlesource.com/c/sdk/+/330420.

The dart-lang/sdk Gerrit review and submission process is one that (understandably) has limitations for external contributors. It has however recently gotten harder with the requirement for approvals from two googlers. Enforcing the OWNERS file has made this even harder.

I believe the OWNERS files actually make this easier rather than harder. The OWNERS file can help Gerrit to suggest relevant reviewers using the ADD OWNERS feature and allow git cl upload --r-owners. The two Googlers requirement is part of an ongoing effort to strengthen supply chain security across Google open source project and is unfortunately here to stay. That being said, our instructions CONTRIBUTING.md instructions need to be updated.

A few common scenarios/issues, both old and new:

1. You are new to contributing to the SDK and open a CL. You might not know labeling practices or contribution standards, so you don't assign a reviewer and no one ever sees it in their triage process. So it sits there awkwardly.

   • Even if you know to assign reviewers. Who do you assign it to? One, two, all of the OWNERS for your changes? Non-googlers can't easily determine who works on what or if that person is on leave or whatever. It also feels bad to assign a reviewer, like you're adding to their work, so you may not want to.

Two OWNERS should be assigned. OWNERS files are how non-googlers can determine who works on what. If someone is an OWNER, it is their responsibility to review incoming CLs in that area. They may remove themselves and add another reviewer if they feel they are better suited to review a particular CL. I realize this is different from your average GitHub project but this is also how the Dart team works with each other so there's no reason to feel bad about it.

2. Open a CL. You can't run (or rerun after fail or changes) the try bots, and need to ask someone else to do so.
3. CL is approved but reviewers don't submit (CQ+2) the change since they think you can do it. Now you need to ping and distract them again to submit the change or else it will sit there. If there's an issue or merge conflict, you then need to do the whole asking and waiting process again.

This is an unfortunate limitation for security reasons on our current distributed build system. We're in the process of moving to a new build system in Q4 which will allow us to revisit this limitation in Q1/Q2 next year.

Until then, it's the OWNERS responsibility to run the presubmit tests and submit the CL on your behalf.

4. CL is approved by one Googler, even if simple, but you now need to find a second Googler.

These types of issues (and more) exist for all external contributors. Perhaps there's no perfect solution for all of these depending on Google's requirements, but a special group like Flutter hackers could maybe help for the most common cases. It could contain contractors, nominated long-term contributors, past Google employees that continue contribution, etc.

I imagine this group would benefit from some or all of the following, depending on what is permissible by Google's policies:

• Can run their desired try bots

• Can submit their own changes that have the necessary approvals These two need to revisited once we're on the new distributed build system. Possibly there's an opportunity to open this up earlier if Googler approvals are already present. I think initially we could allow all users to run presubmits. If we get abuse we might need to lock it down to a smaller group of trusted contributors. So far, we've been able to handle abuse on Gerrit with manual admin actions.

• Only need one Googler approval

The policy explicitly is about two Googlers. This may evolve over time but for now we're stuck with this.

• Not as necessary: Ability to LGTM isn't as important, especially since OWNERs are used now.

LGTM might actually be easier to open up now that there are other controls (2 Googler enforcement+OWNERS checks+self approvement limitation). I think that might be nice because it allows externals to signal their approval of a change.

@sortie WDYT? Could we just allow anyone to vote CR+1?

Other potential/alternative improvements, that would help all external contributors:

If a group like that is not allowed/possible, some additions to the Gerrit UI for (Google) reviewers of non-Google CLs could be helpful. The changes would also help external contributors not part of a trusted group:

• When leaving a review on a CL, if there isn't another reviewer added, it should prompt to add one. This is especially important for external contributors who may not know who to request a review from. Autoassigning reviewers based on OWNERS when a non-Google CL is opened is also an option.

I've proposed updates CONTRIBUTING.md in the CL mentioned above to make it more likely that a reviewer is added. This is kind-of incompatible with the Dart team's workflow, where devs frequently upload a CL, and iterate on it for a while before adding a reviewer. I'm not sure we have the ability/cycles to customize Gerrit enough automate this. There may be ways to insert a reminder/link to the CONTRIBUTING.md.

• Could notify the viewer/reviewer to run the related try bots, since the author can't.

Not sure how to do that from the top of my head. But yes, that would be useful. I do hope that we can address that limitation early next year.

• After the second approval, it could ask that approver if they want to submit the change (since the author can't).

Normally, Dart team members use Gerrit's auto submit feature to signal to each other that a CL may be landed. I'll add that to CONTRIBUTING.md as well. That way, when all submit requirements are fulfilled Gerrit should trigger the submit. This doesn't cover all cases, because auto submit mainly triggers when a reviewer leaves an LGTM, but at least makes this less likely. I think it will also trigger when comments are addressed by the change author, but fail and leave a comment which hopefully nudges the owner into submitting without further action needed by the author.

@parlough Would you mind trying the proposed workflow and let me know if it improves things for you?

• Edit (Added): Relating to @Hixie's mentions below, more standard and frequent CL triage processes by teams could help some of these issues.

I've attempted to find such CLs in Gerrit to see how widespread this problem is. The best search I could come up with is: https://dart-review.googlesource.com/q/is:open+-ownerin:mdb/dart-team+-reviewerin:mdb/dart-team+-is:wip

This does reveal @Hixie's CL: https://dart-review.googlesource.com/c/sdk/+/319871

There's also a lot of false positives turned up in the search (CLs that are clearly not meant to land, not ready review or from former dart team members). Not adding reviewers in the Dart team means "I don't want this reviewed". So my preferred option is to document how to add reviewers instead before adding meetings and processes.

[athomas]

Re chat: it does seem that some people are using hackers-dart on the Flutter discord for that purpose: https://discord.com/channels/608014603317936148/608022273152122881

[sortie]

@athomas I am positive towards allowing +1 from externals since we have Review-Enforcement. Maybe we should ask Gerrit for official guidance here. I'm not entirely sure we can trust Review-Enforcement to be around and reliable enough though, I recall seeing one of our repos not have it and I didn't understand why. Maybe once these features have proven themselves a bit more. Let's have a chat about this next time we talk :)

[mraleph]

Related to this: can we make Copybara put a comment on a pull request whenever corresponding Gerrit review has changes? (e.g. reviewer has submitted comments, reviewer has +1/+2ed the change)?

[sortie]

Off the top of my head, we don't have the tech for that, at least with what I know I can do with Copybara.I think sending messages back would considerably improve the workflow and we could explore if Copybara has the functionality, or we may need to build a custom bot ala the label notifier.

[athomas]

@parlough did you have a chance to look at the changes proposed in my CL?

[leafpetersen]

A couple of thoughts/comments.

First, the message that the go team sends to github (example) is much clearer than ours (including links, and detailed callouts of how to proceed. We should consider adopting something similar.

Second, it does seem like they have some kind of updating mechanism in place to push messages back to the PR.

Third, I wonder whether it might not be useful to include in CONTRIBUTING.md (or somewhere else) a short guide to "how to review an external contribution" to help our internal reviewers understand how the different challenges to navigating an external PR. @jacob314 and I could potentially contribute to that.

Fourth, would it be possible to have copybara automatically add somebody specific to the gerrit reviews that external PRs open up? Again, assuming the volume is relatively low (which it appears to be) we could probably do the triage with just a couple of volunteers (me, @jacob314 ?).

[sortie]

Thank you for the exciting new information @leafpetersen!

Go is our new sister team, so we may be able to share some infrastructure like this. It looks like they're using this gopherbot instead of copybara. Maybe it could fit our needs. Although we do like copybara, it also does have issues. We should talk to them about it. Maybe we just need the message pushback. I really do like their messaging on the initial message.

It's no problem making copybara add reviewers on import. I'll send you a CL :)

[Hixie]

One thing that sounds trivial but I think helps — I would caution against using the word "external" to refer to people who are not at Google. It tends to be exclusionary and leads to an "us vs them" rift in the community. "Googler" vs "Non-Googler" is more precise and avoids the issue.

[athomas]

One thing that sounds trivial but I think helps — I would caution against using the word "external" to refer to people who are not at Google. It tends to be exclusionary and leads to an "us vs them" rift in the community. "Googler" vs "Non-Googler" is more precise and avoids the issue.

I've updated the issue title. In fact, even Googlers may face some of these issues because some ACLs are limited to Dart team members.

[athomas]

Third, I wonder whether it might not be useful to include in CONTRIBUTING.md (or somewhere else) a short guide to "how to review an external contribution" to help our internal reviewers understand how the different challenges to navigating an external PR. @jacob314 and I could potentially contribute to that.

These are the sections in the CONTRIBUTING.md currently describing the committer part of the workflow: https://github.com/dart-lang/sdk/blob/main/CONTRIBUTING.md#for-committers-submitting-a-patch https://github.com/dart-lang/sdk/blob/main/CONTRIBUTING.md#for-committers-merging-contributions-from-non-members

[jacob314]

I hear that @Hixie might soon have a publicly available doc on how to review PRs from new contributors. Would be great to link to that doc both for Flutter and Dart. It goes into a lot of details that would be helpful for new contributors to understand as they get their first contributions reviewed.

[Hixie]

https://github.com/flutter/flutter/wiki/Tree-hygiene#how describes how to review PRs for Flutter.

[Hixie]

Re: the two-googler review policy, I understand moving from git-on-borg to GitHub as the source of truth could alleviate that problem.

[parlough]

I just want to say thanks everyone for the continued discussion by and various ideas discussed (and some already implemented)! It's excited to see all the considerations about how the contribution experience can be improved, through different aspects like documentation, bots, infra changes, etc.

Go is our new sister team, so we may be able to share some infrastructure like this.

Taking advantage of or adapting some of the Go team's Gerrit and other bot workflows sounds great. I know they considered moving back to Gerrit only earlier this year but are(?) trying some adaptations to avoid that, so sounds like the teams could benefit from sharing experiences and improve those workflows together.

Third, I wonder whether it might not be useful to include in CONTRIBUTING.md (or somewhere else) a short guide to "how to review an external contribution"

I think this would be great, as long as it's somewhere public and linked to. Understanding the expected process helps those being reviewed by it as well. Hixie's wiki guidelines and presentation definitely have some good guidelines and advice to start with. It would of course need to be modified for the Dart team and SDK's preferences and situation.

[parlough]

@athomas said in response to try bots discussion:

This is an unfortunate limitation for security reasons on our current distributed build system. We're in the process of moving to a new build system in Q4 which will allow us to revisit this limitation in Q1/Q2 next year.

Thanks for the explanation, that makes complete sense and is understandable! It's exciting to hear developments could change the situation though. While I do think a generalized 2 (nominated) committer policy like Flutter/Chromium is great for Dart's openness, having a smaller role similar to AOSP verifiers or Chromium try-bot access is much more valuable, especially for the state of SDK contributions as of now. To clarify, would this work make a similar role more of a possibility?

I'm really not at all familiar with this area so I may be using terminology completely wrong, but would that mean switching over to something standardized like Bazel's Remote Execution API rather than Goma? That way other groups who contribute to or fork the SDK could more easily use their own backend of choice for distributed builds?

[athomas]

Thanks for the explanation, that makes complete sense and is understandable! It's exciting to hear developments could change the situation though. While I do think a generalized 2 (nominated) committer policy like Flutter/Chromium is great for Dart's openness, having a smaller role similar to AOSP verifiers or Chromium try-bot access is much more valuable, especially for the state of SDK contributions as of now. To clarify, would this work make a similar role more of a possibility?

I would hope we can make that access generally available to all contributors. But if there's abuse or higher-than-expected load resulting from this we may need a more restrictive model.

I'm really not at all familiar with this area so I may be using terminology completely wrong, but would that mean switching over to something standardized like Bazel's Remote Execution API rather than Goma? That way other groups who contribute to or fork the SDK could more easily use their own backend of choice for distributed builds?

It isn't really about the specific technology but that we need separate environments that are isolated from each other and it's not worth investing in that before migrating to the new technology (that effort would be wasted). We're migrating to https://github.com/bazelbuild/reclient which uses the Remote Execution API under the hood but we're already using that today on the backend as well.