codereview: accept Github PRs

[bradfitz]

I propose we start accepting Github PRs (Pull Requests).

Currently we have a bot auto-close them with a message telling them we don't use PRs and instead use Gerrit.

When we moved to Github, @robpike said:

Most members of the Go community use Git and host their work on GitHub, and we should join them.

While that's true, we're still not using Github like Github users use Github.

I believe that our current pushback bot dissuades many potential contributors.

I propose we start accepting pull requests by automatically converting them into Gerrit CLs ("change lists", same as a PR but different terminology). Reviews would still happen on Gerrit and the bot would update the PR of activity on Gerrit. Gerrit is still where we'd run trybots and push the "Merge" button. We would never merge on Github. Gerrit would remain the upstream source-of truth.

I prototyped this syncing in https://github.com/LetsUseGerrit/gerritbot/ and used it a bit while working on gRPC-go (examples), but in the opposite direction: my Gerrit CLs were abandoned after gRPC-go accepted them on Github.

In any case, the point is that this can be automated with a bit of work and rejecting Github PRs or not is a policy decision more than anything. I propose we change our policy.

Some will say that the quality of PRs will decrease, as many the PRs that arrive and are auto-closed by the pushback bot are pretty bad. But so are many of the Gerrit CLs. I believe the Gerrit CLs are only better on average because that means it's more likely people have read the contributing instructions, or have contributed in the past. But if you only look at first-time contributors on Github PRs vs Gerrit CLs, the quality doesn't look too different. People improve over time as they learn the project and its policies.

[randall77]

How would the CLA happen? My understanding is that currently you can't make a Gerrit CL without having already jumped through the CLA hoop. If we auto-make a Gerrit CL, we'd have un-CLAd CLs floating around. Mark them in big red letters, maybe?

[bradfitz]

@randall77, Google has an official bot for CLA checking already. We just have to flip a bit to turn it on.

[bradfitz]

(The CLA check would happen on the PR before the Gerrit CL is created.)

[fatih]

Is Gerrit a requirement? I'm asking because Github recently released improved versions of reviewing Code at Github. Some parts (such as commenting on the code) is now similar to Gerrit. Also say we opened a PR from Github, is the Contributor required to do anything in Gerrit or is only used for merging/testing? I'm curious about the boundary (I just contributed once via Gerrit so don't remember quite the details anymore

[mvdan]

What about feedback? If someone files a PR and it gets converted to a Gerrit CL, they might not know how or be willing to reply to any comments or update the code.

[bradfitz]

@fatih, Gerrit is still a requirement. GitHub has made improvements, but it's still not as powerful as Gerrit. Moving off Gerrit would be way too big of a change to be palatable.

@mvdan, as I wrote above:

Reviews would still happen on Gerrit and the bot would update the PR of activity on Gerrit.

You can see this in action at https://github.com/grpc/grpc-go/pull/546 for example. The bot is barely a prototype at this point and could be improved to leave better messages on Github.

[velovix]

Taking a look at the grpc issue, I'm a bit skeptical as to the value this strategy adds. Users get to contribute changes in a more familiar way, but as soon as a code review is posted, we're in unfamiliar territory again. I suspect that, as a result of this change, we'll see a lot of abandoned contributions proposed by people who didn't know what they were getting into and don't have an interest in figuring this all out. Learning Gerrit is a pill contributors still need to swallow, regardless.

That said, I appreciate the effort the Go team is making to improve this process.

[bradfitz]

but as soon as a code review is posted, we're in unfamiliar territory again.

It's something to learn, yes, but it's not as much of a mountain as our current process.

Also, the new Gerrit UI is coming along nicely and will probably be the default at about the same time we're done with this, lowering the bar as well.

[rakyll]

I would also love to see Trybots integration to push the CI results back to the PR. If no one is working on that, I can.

[bradfitz]

@rakyll, that'd be nice, but it's a bit soon for that. Let's get Hello World working first, and then we can do that fanciness.

[slimsag]

( @rakyll did this get closed by accident? )

[fbettag]

YES YES YES YES!

[nathany]

👍 This would be nice.

As someone who rarely uses Gerrit or the gocontribute tool, I've found it necessary to review the rather lengthy Contribution Guidelines #17802 every time. Honestly, that is a deterrent when wanting to make a small change, such as fixing a typo in the release notes.

I do have the same question (https://github.com/golang/go/issues/18517#issuecomment-270517566) as @fatih and I'm not convinced that Gerrit is still more powerful. It seems far simpler to join language communities like Microsoft TypeScript, Mozilla Rust, and Apple Swift in using GitHub for development.

But I can understand that a lot of tooling has been built up around Gerrit, that it could be the personal preference of all or most regular contributors, and switching tools yet again would be a big undertaking.

Which is to say, I think this proposal would be a very nice way to meet the GitHub-using community in the middle. Using Gerrit for review isn't bad -- a few icons are a mystery (e.g. it took me a minute to figure out how to switch to a unified view), but that will likely improve with the new UI.

I think the big open question is what happens when somebody pushes a followup commit to GitHub or amends their local commit and force pushes it? Can gerritbot be expected to handle that?

Thanks Brad.

[glasser]

Would contributors be able to submit additional versions of their patch set by updating their PR (branch) too, or would they have to switch to Gerrit?

As a very occasional Go contributor I find I have to relearn the semantics around creating and publishing patch sets each time, vs GitHub where it's just "push to a branch". (As an open source maintainer I do recognize that Gerrit leads to better patch sets vs PRs where contributors tend to tack more commits on the end during review instead of rewriting, though.) Whereas the discussion part on Gerrit is no trouble.

[nathany]

@glasser Check out GitHub squash commits https://github.com/blog/2141-squash-your-commits It's long been possible for maintainers to squash things first via tools like hub, or for contributors to amend and force push. In the past year, such functionality is just a button on GitHub. It's not identical to the Gerrit cherry-pick system, but accomplishes the same goal (tidy git history).

[bamarni]

Would it make sense that the gopherbot also syncronizes it the other way around (Gerrit -> Github)? Because apart from contributing Github PRs also allow people to watch open-source projects development, either through the timeline or the notifications overview panel.

That could be great to also have access to the contribution activity through Github, eg. to see what CLs are currently pending, what is merged or refused, possibly what is discussed too.

[rakyll]

( @rakyll did this get closed by accident? )

It is closed by my issue triaging tool, waffle.io, by mistake :(

[bradfitz]

@bamarni, that might be nice later once we get the basics working. Or maybe https://dev.golang.org/ will be fleshed out more by then and we'd prefer that dashboard. We'll see.

[nathany]

If gerritbot is just to transfer the initial PR to a CL, what would the transition look like for followup changes based on the review?

[bradfitz]

If gerritbot is just to transfer the initial PR to a CL

That is not true. Even in the prototype already implemented, it updates Gerrit when a new version of the Github PR arrives. It can also reply and tell users to squash if they're stacking commits on top of each other.

[willnorris]

(The CLA check would happen on the PR before the Gerrit CL is created.)

This should certainly work fine, but let me know if/when you enable this so I can keep a close eye on any CLA issues.

There are a few cases I can think of that might cause confusion, depending on how you are creating the Gerrit CL. CLA checking on both GitHub and Gerrit support looking contributors up by email address, but Gerrit has its own internal email alias concept that doesn't apply to GitHub PRs. Similarly, CLA checks on GitHub PRs can be done based solely on GitHub username, even if the git author email is something completely different than what was used to sign the CLA. Gerrit obviously doesn't know or care about GitHub usernames. So I can imagine a case where CLA checking on the GitHub PR succeeds, but subsequently fails when moved over to Gerrit.

Anyway, lots of moving pieces, but I'm happy to do what we can on the CLA side to make this easier.

[bradfitz]

@willnorris, thanks! Yes, I planned on bugging you, @spearce, et al when the time came.

[JustinAzoff]

This would be very nice. A week ago I forked x/crypto to add a missing feature to the ssh library[1]. https://golang.org/doc/contribute.html is daunting and I ended up tabling the whole thing until I have a few hours to set aside to setup everything required to submit or even discuss my 10 line patch.

[1] https://github.com/golang/crypto/compare/master...JustinAzoff:master

[jadbox]

@bradfitz "It can also reply and tell users to squash if they're stacking commits on top of each other."

When you merge PRs, you have the option to squash the merge via Github's UI. I've thought it's odd that users are forced to keep squashing commits on their branches... it's especially annoying when the PR author makes several small changes to adhere to feedback and then they need to (continually) 're-squash' them.

[minux]

Yeah, perhaps the gerritbot should squash the commits by itself, which makes the live easier for PR authors.

I tried to squash and push -f for github PRs, but github would then think the different squashed commits are different, and will add separate notifications to the mentioned issues.

One problem for gerritbot to squash the commit is to figure out the proper commit message to use. And I don't have good solution for this problem...

[bradfitz]

We'll start with requiring users to squash them themselves. That isn't an unreasonable amount of effort.

[Manishearth]

FWIW Servo uses http://reviewable.io/ which is able to track review history much better. The workflow is integrated into github, so a reviewer can choose to use Reviewable if they think PR is complex enough to need it. Smaller PRs just go through the git review interface.

[leighmcculloch]

Like @glasser and @nathany I'm someone who'd like to contribute to Go, but when I find a way to contribute, the time required to read the lengthy contribution instructions and augment my dev environment is an unproportional overhead.

GitHub has made improvements, but it's still not as powerful as Gerrit.

@bradfitz: I share the same question as @fatih and challenge the idea that Gerrit is required because it provides more features.

Go has set itself apart by shedding features for simplicity and speed, but as @nathany pointed out it has a heavy and time consuming contribution workflow compared to TypeScript, Rust, Swift. For those of us that contribute to many projects, the need to relearn this project's semantics is an obstacle.

Ironically, contributing to go projects is straightforward because of the consistency in build tool usage, and contributing to most OSS on GitHub is too because of a consistent usage of GitHub PRs, but this is not the case with the Go project because of it's unique contribution workflow.

If GitHub PRs were used entirely, CLAs could still tracked. There are several GitHub integrations that solve this problem (cla-assistance, clahub) by blocking PRs from merging until the GitHub user signs the CLA, and Google's CLA checker could prevent merging in the same way.

Allowing us to open PRs via GitHub without using Gerrit is a great first step.

[bradfitz]

@leighmcculloch, noted. I understand that many people are happy with Github code reviews and totally agree that more people are used to them. That's too big of a technical change and too controversial to even pursue. I can only propose and support this incremental change at this time. Maybe Github will improve enough later. But maybe Gerrit will too. Let's wait and see. For now, I want to try the syncing thing.

[dsnet]

When I first started contributing to Go, I was also intimidated by the code review process. While it took time to setup git-codereview and learn how to use Gerrit (which certainly has its quirks), I believe it is still the better review tool. Compared to GitHub PR reviews, Gerrit allows you to track user changes better as the review goes on and does a better job keeping a history of the code review discussion. When I do reviews on GitHub (for other repos) I often find it difficult figuring what the latest changes were and whether they addressed my comments from earlier.

While I'm not attached to Gerrit, I do hope we stay with it until Github reviews are better.

[fatih]

@bradfitz thanks for the clarification. I agree that it's hard to switch directly to Github on a single step. It's not feasible, and even if we decide to switch, it should be do gradually as there are many years of experience and tooling build around Gerrit. However I think we should probably outline a document on what we miss from Github, that Gerrit provides but Github still doesn't have. Maybe at least that would be helpful for others joining the conversation later and also would be helpful to track the situation if we want to switch later in the future.

[dsnet]

\cc @connors (who works at GitHub, and may be interested in the discussion regarding GitHub reviews)

[rakyll]

@fatih There has been a summary of reasons why the Go project initially preferred Gerrit over PRs at https://talks.golang.org/2015/state-of-go.slide#7. A few items have been improved by GitHub regarding the ability to draft comments, but there are still missing pieces:

• Can only view diffs on a single page (can be very slow).
• Cannot compare differences between patch sets.
• To create a patch one must fork the repository publicly (weird and unnecessary).

Not listed on the slides but most significantly as @dsnet mentions, there is no way to keep track of which comments have been addressed.

Also, I don't how Github handles dependencies between PRs. This is a common requirement for Go. Most people send out code in multiple CLs in order to keep the CLs at at reasonable size.

[ghost]

I also found it off putting to have to use Gerrit to submit patches to Shiny for example.

what if the golang team could add an offically supported tool that helps golang programmers to do PR/s mergers with github ? I dont knwo enough about the underlying architecture to comment further though.

[driusan]

@minux If autosquashing, the PR description seems like the obvious choice to use for the commit message. GitHub even automagically populates it with the message from the commit when you send a PR with only one commit.

[leighmcculloch]

I don't how Github handles dependencies between PRs.

@rakyll: It may not be as elegant, but GitHub allows you to open a PR against another PR. GitHub allows you to change the base branch to master which I do after the earlier PR is merged. It might not be as elegant but I use that in my workflow to nest multiple PRs that are dependent on each other.

there is no way to keep track of which comments have been addressed.

@rakyll: If a reviewer requests changes when creating their review, the GitHub merge box shows a large red X until all reviewers who requested changes return, review the changes made since they last reviewed (that's a feature displayed to reviewers after changes are made), and click "Approve Changes". It is mostly up to the reviewer to check that their comment has been addressed unfortunately, but the tools are there to help the reviewer do so.

For now, I want to try the syncing thing.

@bradfitz: I agree, this is a great first step that I hope leads to a standard contribution workflow in the future.

[nathany]

"Can only view diffs on a single page (can be very slow)." - @rakyll

The issue with slow diffs has been addressed fairly well IMO http://githubengineering.com/how-we-made-diff-pages-3x-faster/

It's still a single page instead of individual diffs like Gerrit, but at least it is usable now. Here's an example of a large pull pull request using gvt to vendor a bunch of dependencies: https://github.com/nathany/gvt-sample/pull/1

The benefit of GitHub pull requests is familiarity for a lot of people. There are disadvantages too, and probably always will be.

I'm really grateful for this step towards making it easier for people to get involved, and I like @fatih's suggestion to keep tabs on the situation.

[j7b]

It would be great to take advantage of pull requests and incorporate them into the project's workflow and would probably encourage contribution.

@bradfitz any possibility of using https://github.com/google/git-appraise metadata to integrate pull requests and Gerrit activity, with the advantage of Gerrit content existing in the repo as well? PR to CR appears already implemented in https://github.com/google/git-pull-request-mirror.

[bradfitz]

@j7b, I don't follow. Are you proposing an implementation strategy, or a tactic for that tactic's sake? I don't understand "with the advantage of Gerrit content existing in the repo as well". We don't want the Gerrit code review comment history in the main Go repo's git history.

[rakyll]

I'm really grateful for this step towards making it easier for people to get involved, and I like @fatih's suggestion to keep tabs on the situation.

Maybe someone needs to write a followup proposal to open a discussion and the project can keep evaluating the situation.

I assume Github-Gerrit bridge will keep the discussion alive as the current and future contributors see PRs around on a daily basis. This bridge is a good incremental step in the right direction.

Additionally in the back of my mind, I was wondering if it is possible to default to GitHub PRs and fallback to Gerrit CLs for advanced users. But, such a model would complicate everything given Gerrit is opinionated that it should own the master. I am convinced that the bridge is the best best best option to enable the vast majority to contribute.

[j7b]

@bradfitz basically my train of thought was that an integration of Github and Gerrit represents operations based on common metadata, and storing that metadata in the repo is probably a reasonable approach, and using the appraise serialization and ref conventions allows the metadata to be consumed by other tools as well.

[jadbox]

any thoughts about Reviewable vs Gerrit? Can we use one over the other.. or are they not comparable?

https://reviewable.io/ https://www.gerritcodereview.com/

[bradfitz]

@jadbox, switching away from Gerrit is not up for discussion (to Github, to Reviewable, to anything), at least not in this bug. This bug is only about accepting Github PRs and automatically syncing them with Gerrit reviews.

[rsc]

From a policy standpoint, this sounds great. I'm sure we'll work through the details as they arise but the current sketch is good.

-rsc for @golang/proposal-review

[AlekSi]

Can someone update #9220 with a link to this one? Automatic Github linking doesn't work for locked issues.

[bradfitz]

@AlekSi, done.

[nathany]

Fyi, GitHub is looking to survey open source communities that are not on GitHub. https://github.com/github/open-source-survey/issues/83 via @bkeepers.

[VojtechVitek]

@bradfitz Sorry to bug you - but I'm not quite clear on the state of this issue.

The proposal has been accepted. Does that mean that someone will start implementing the syncing bot? Or is this on hold until scheduled? Or do you guys expect someone from the community to step up and take over this? Thanks!

[bradfitz]

@VojtechVitek, yes, I will do it sometime. It's moving up my priority list as I finish other things. I've done part of it already.

[AlmogBaku]

Maybe we can take a look how the guys from Kubernetes works.. they also use Github, and it's also a large scale project from google..

In k8s we use github for code-review and in some projects we also have "reviewable" which is more advanced tool (k8s also have a bot for that.. dunno exactly what it does though 😆 )