codereview: accept Github PRs #18517

Closed
bradfitz opened this Issue Jan 4, 2017 · 100 comments

Comments

@bradfitz
Member

bradfitz commented Jan 4, 2017

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.

@bradfitz bradfitz added the Proposal label Jan 4, 2017

@bradfitz bradfitz added this to the Proposal milestone Jan 4, 2017

@bradfitz bradfitz self-assigned this Jan 4, 2017

@randall77

This comment has been minimized.

Show comment
Hide comment
@randall77

randall77 Jan 4, 2017

Contributor

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?

Contributor

randall77 commented Jan 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 4, 2017

Member

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

Member

bradfitz commented Jan 4, 2017

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

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 4, 2017

Member

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

Member

bradfitz commented Jan 4, 2017

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

@fatih

This comment has been minimized.

Show comment
Hide comment
@fatih

fatih Jan 4, 2017

Member

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

Member

fatih commented Jan 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Jan 4, 2017

Member

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.

Member

mvdan commented Jan 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 4, 2017

Member

@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 grpc/grpc-go#546 for example. The bot is barely a prototype at this point and could be improved to leave better messages on Github.

Member

bradfitz commented Jan 4, 2017

@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 grpc/grpc-go#546 for example. The bot is barely a prototype at this point and could be improved to leave better messages on Github.

@velovix

This comment has been minimized.

Show comment
Hide comment
@velovix

velovix Jan 5, 2017

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.

velovix commented Jan 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 5, 2017

Member

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.

Member

bradfitz commented Jan 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rakyll

rakyll Jan 5, 2017

Member

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.

Member

rakyll commented Jan 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 5, 2017

Member

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

Member

bradfitz commented Jan 5, 2017

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

@rakyll rakyll closed this Jan 5, 2017

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Jan 5, 2017

( @rakyll did this get closed by accident? )

slimsag commented Jan 5, 2017

( @rakyll did this get closed by accident? )

@dsnet dsnet reopened this Jan 5, 2017

@fbettag

This comment has been minimized.

Show comment
Hide comment
@fbettag

fbettag Jan 5, 2017

YES YES YES YES!

fbettag commented Jan 5, 2017

YES YES YES YES!

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Jan 5, 2017

Contributor

👍 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 (#18517 (comment)) 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.

Contributor

nathany commented Jan 5, 2017

👍 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 (#18517 (comment)) 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

This comment has been minimized.

Show comment
Hide comment
@glasser

glasser Jan 5, 2017

Contributor

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.

Contributor

glasser commented Jan 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Jan 5, 2017

Contributor

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

Contributor

nathany commented Jan 5, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bamarni

bamarni Jan 5, 2017

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.

bamarni commented Jan 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rakyll

rakyll Jan 5, 2017

Member

( @rakyll did this get closed by accident? )

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

Member

rakyll commented Jan 5, 2017

( @rakyll did this get closed by accident? )

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

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 5, 2017

Member

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

Member

bradfitz commented Jan 5, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Jan 5, 2017

Contributor

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?

Contributor

nathany commented Jan 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 5, 2017

Member

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.

Member

bradfitz commented Jan 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@willnorris

willnorris Jan 5, 2017

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

(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

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 5, 2017

Member

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

Member

bradfitz commented Jan 5, 2017

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

@JustinAzoff

This comment has been minimized.

Show comment
Hide comment
@JustinAzoff

JustinAzoff Jan 5, 2017

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] golang/crypto@master...JustinAzoff:master

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] golang/crypto@master...JustinAzoff:master

@jadbox

This comment has been minimized.

Show comment
Hide comment
@jadbox

jadbox Jan 5, 2017

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

jadbox commented Jan 5, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@minux

minux Jan 5, 2017

Member
Member

minux commented Jan 5, 2017

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 5, 2017

Member

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

Member

bradfitz commented Jan 5, 2017

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

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Jan 6, 2017

Contributor

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.

Contributor

Manishearth commented Jan 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@leighmcculloch

leighmcculloch Jan 6, 2017

Contributor

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.

Contributor

leighmcculloch commented Jan 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 6, 2017

Member

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

Member

bradfitz commented Jan 6, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@dsnet

dsnet Jan 6, 2017

Member

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.

Member

dsnet commented Jan 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@fatih

fatih Jan 6, 2017

Member

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

Member

fatih commented Jan 6, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@dsnet

dsnet Jan 6, 2017

Member

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

Member

dsnet commented Jan 6, 2017

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

@rakyll

This comment has been minimized.

Show comment
Hide comment
@rakyll

rakyll Jan 6, 2017

Member

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

Member

rakyll commented Jan 6, 2017

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

@gedw99

This comment has been minimized.

Show comment
Hide comment
@gedw99

gedw99 Jan 6, 2017

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.

gedw99 commented Jan 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@driusan

driusan Jan 6, 2017

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

driusan commented Jan 6, 2017

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

gopherbot pushed a commit to golang/build that referenced this issue Jan 17, 2018

cmd/gerritbot: close GitHub PR when Gerrit change is closed
When a Gerrit change moves into a "closed" state (merged or abandoned),
close the linked GitHub Pull Request with the appropriate message.
If the change has been merged, there is no need to mention the commit
in the message on the PR because the commit message will be linked
from the PR by virtue of the GitHub-Pull-Request: git label. See
golang/scratch#2 (comment) for
an example.

Closed changes are also be cleaned up within b.pendingCLs.

Also removes a nil pointer dereference in the case where the
Gerrit CL does not exist yet, surmising its link from the output
of the push command.

Updates golang/go#18517

Change-Id: Ieb28c1b9d31216d48076b256bf6a65a099a38552
Reviewed-on: https://go-review.googlesource.com/87915
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Jan 19, 2018

Change https://golang.org/cl/88615 mentions this issue: cmd/gerritbot: properly import changes when running on k8s

Change https://golang.org/cl/88615 mentions this issue: cmd/gerritbot: properly import changes when running on k8s

gopherbot pushed a commit to golang/build that referenced this issue Jan 20, 2018

cmd/gerritbot: properly import changes when running on k8s
+ Switch to alpine linux as the base container for the
  Docker image so that git and curl can be easily installed
+ Remove ca-certificate copying since alpine has the cert
+ Although the git committer must remain GerritBot in order
  for the gitcookies auth to work, this will cause the original
  author to be CC’d on the Gerrit change and the proper owner
  will be attributed in the git history
+ Download the git http cookies file from GCE metadata to
  properly authorize GerritBot to upload changes

Update golang/go#18517

Change-Id: I3acc7323c8f1b35ed2145f610cb03fbcd29dcdf4
Reviewed-on: https://go-review.googlesource.com/88615
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Jan 31, 2018

Change https://golang.org/cl/91135 mentions this issue: cmd/gerritbot: allow user to toggle gerritbot comments on GitHub

Change https://golang.org/cl/91135 mentions this issue: cmd/gerritbot: allow user to toggle gerritbot comments on GitHub

gopherbot pushed a commit to golang/build that referenced this issue Jan 31, 2018

cmd/gerritbot: allow user to toggle gerritbot comments on GitHub
Also updates the base image of Dockerfile.0 to 1.9.3 and alters
the Makefile to be more explicit of where to push/deploy.

Updates golang/go#18517

Change-Id: I5a63a2c6b954da7417fd0832767e023077b3d8a3
Reviewed-on: https://go-review.googlesource.com/91135
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@andybons

This comment has been minimized.

Show comment
Hide comment
@andybons

andybons Feb 5, 2018

Member

OK. GerritBot is in a good state to accept PRs, but the scratch repo is the only one whitelisted for use.

In order to launch this to the broader Go community we need to whitelist the repos where we wish to support this. Maintner is tracking the following projects. We should check which ones we want to whitelist:

List of projects to whitelist
  • benchmarks
  • blog
  • arch
  • crypto
  • build
  • debug
  • example
  • exp
  • gddo
  • go
  • image
  • mobile
  • net
  • oauth2
  • perf
  • playground
  • proposal
  • review
  • scratch
  • sublime-build
  • sublime-config
  • sync
  • sys
  • talks
  • term
  • text
  • time
  • tools
  • tour

Also:

  • Remove the pushback GitHub hook that automatically closes PRs
  • Find out why maintner isn’t triggering when a new PR issue is being created
  • Write an FAQ for new users of the system
  • Remove GitHub Pull Request templates from repos
  • Update documentation to say we accept PRs
Member

andybons commented Feb 5, 2018

OK. GerritBot is in a good state to accept PRs, but the scratch repo is the only one whitelisted for use.

In order to launch this to the broader Go community we need to whitelist the repos where we wish to support this. Maintner is tracking the following projects. We should check which ones we want to whitelist:

List of projects to whitelist
  • benchmarks
  • blog
  • arch
  • crypto
  • build
  • debug
  • example
  • exp
  • gddo
  • go
  • image
  • mobile
  • net
  • oauth2
  • perf
  • playground
  • proposal
  • review
  • scratch
  • sublime-build
  • sublime-config
  • sync
  • sys
  • talks
  • term
  • text
  • time
  • tools
  • tour

Also:

  • Remove the pushback GitHub hook that automatically closes PRs
  • Find out why maintner isn’t triggering when a new PR issue is being created
  • Write an FAQ for new users of the system
  • Remove GitHub Pull Request templates from repos
  • Update documentation to say we accept PRs
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Feb 5, 2018

Member

Let's ignore code.googlesource.com for now. Too many differences.

And ignore grpc-review and gofrontend and gollvm. They're special.

But otherwise I think we should enable it for all of the others.

Member

bradfitz commented Feb 5, 2018

Let's ignore code.googlesource.com for now. Too many differences.

And ignore grpc-review and gofrontend and gollvm. They're special.

But otherwise I think we should enable it for all of the others.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Feb 5, 2018

Change https://golang.org/cl/92135 mentions this issue: cmd/pubsubhelper: don’t ignore Pull Request webhook events

Change https://golang.org/cl/92135 mentions this issue: cmd/pubsubhelper: don’t ignore Pull Request webhook events

gopherbot pushed a commit to golang/build that referenced this issue Feb 6, 2018

cmd/pubsubhelper: don’t ignore GitHub Pull Request webhook events
Update golang/go#18517

Change-Id: Idcd7813a36cc7491d2c2b03adcdfbe71df383f8e
Reviewed-on: https://go-review.googlesource.com/92135
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Feb 8, 2018

Change https://golang.org/cl/92936 mentions this issue: cmd/gerritbot: include link to wiki in import messages

Change https://golang.org/cl/92936 mentions this issue: cmd/gerritbot: include link to wiki in import messages

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Feb 8, 2018

Change https://golang.org/cl/92937 mentions this issue: cmd/gerritbot: add all Gerrit projects to whitelist

Change https://golang.org/cl/92937 mentions this issue: cmd/gerritbot: add all Gerrit projects to whitelist

gopherbot pushed a commit to golang/build that referenced this issue Feb 8, 2018

cmd/gerritbot: include link to wiki in import messages
Update golang/go#18517

Change-Id: I5c7980d07db368c90c2e76610ee4c854cec27b60
Reviewed-on: https://go-review.googlesource.com/92936
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

gopherbot pushed a commit to golang/build that referenced this issue Feb 8, 2018

cmd/gerritbot: add all Gerrit projects to whitelist
Update golang/go#18517

Change-Id: I5ea227896265d5612b5efa60850f9e03b9025db6
Reviewed-on: https://go-review.googlesource.com/92937
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Feb 8, 2018

Change https://golang.org/cl/93015 mentions this issue: cmd/gopherbot: ignore GitHub PRs when updating issues

Change https://golang.org/cl/93015 mentions this issue: cmd/gopherbot: ignore GitHub PRs when updating issues

gopherbot pushed a commit to golang/build that referenced this issue Feb 9, 2018

cmd/gopherbot: ignore GitHub PRs when updating issues
Otherwise you get something like
golang/go#23752 (comment)

Update golang/go#18517

Change-Id: I5298f1e34738a4f6faa8b829546bf880a237043d
Reviewed-on: https://go-review.googlesource.com/93015
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Feb 9, 2018

Change https://golang.org/cl/93035 mentions this issue: all: remove PULL_REQUEST_TEMPLATE from .github

Change https://golang.org/cl/93035 mentions this issue: all: remove PULL_REQUEST_TEMPLATE from .github

gopherbot pushed a commit that referenced this issue Feb 9, 2018

all: remove PULL_REQUEST_TEMPLATE from .github
Update golang/go#18517

Change-Id: I76d928d5fcc5ed22beaffb86f0fa8fbf6d4ac3d7
Reviewed-on: https://go-review.googlesource.com/93035
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Feb 9, 2018

Change https://golang.org/cl/93135 mentions this issue: maintner/maintnerd: add additional GitHub projects to "go" config

Change https://golang.org/cl/93135 mentions this issue: maintner/maintnerd: add additional GitHub projects to "go" config

gopherbot pushed a commit to golang/build that referenced this issue Feb 9, 2018

maintner/maintnerd: add additional GitHub projects to "go" config
Update golang/go#18517

Change-Id: Icdc131236092f15c51effafa71329e280979a0ef
Reviewed-on: https://go-review.googlesource.com/93135
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Feb 11, 2018

Change https://golang.org/cl/93335 mentions this issue: CONTRIBUTING: remove Pull Request bit

Change https://golang.org/cl/93335 mentions this issue: CONTRIBUTING: remove Pull Request bit

@andlabs

This comment has been minimized.

Show comment
Hide comment
@andlabs

andlabs Feb 11, 2018

Contributor

Would it still be possible to submit changes directly to Gerrit?

Contributor

andlabs commented Feb 11, 2018

Would it still be possible to submit changes directly to Gerrit?

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney Feb 11, 2018

Contributor
Contributor

davecheney commented Feb 11, 2018

@andlabs

This comment has been minimized.

Show comment
Hide comment
@andlabs

andlabs Feb 11, 2018

Contributor

Good; thanks. (I'm not opposed to GitHub PRs at all — my own projects use the PR system and GitHub's code review software; I am a bit uncomfortable with having to have a forked repository linger in your repo list, though, but that may just be me not knowing everything about git or GitHub to know why the current system they have is better than just sending over a patch file. That being said, I plan on patching Go from my work computer(s) instead of from my home computer, and was going to just git clone directly instead of going through GitHub, which is why I was asking.)

Contributor

andlabs commented Feb 11, 2018

Good; thanks. (I'm not opposed to GitHub PRs at all — my own projects use the PR system and GitHub's code review software; I am a bit uncomfortable with having to have a forked repository linger in your repo list, though, but that may just be me not knowing everything about git or GitHub to know why the current system they have is better than just sending over a patch file. That being said, I plan on patching Go from my work computer(s) instead of from my home computer, and was going to just git clone directly instead of going through GitHub, which is why I was asking.)

gopherbot pushed a commit that referenced this issue Feb 11, 2018

CONTRIBUTING: remove Pull Request bit
Also remove the "Also, please do not post patches on the issue
tracker" part, since that didn't seem to reduce the number of patches
inlined into bug reports. And now that we accept PRs, people will
probably try that first. We'll see.

Fixes #23779
Updates #18517

Change-Id: I449e0afd7292718e57d9d428494799c78296a0d2
Reviewed-on: https://go-review.googlesource.com/93335
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Feb 11, 2018

Change https://golang.org/cl/93355 mentions this issue: cmd/gerritbot: remind people to publish their draft comments

Change https://golang.org/cl/93355 mentions this issue: cmd/gerritbot: remind people to publish their draft comments

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Feb 11, 2018

Change https://golang.org/cl/93356 mentions this issue: cmd/gerritbot: reword message for when a Gerrit message is posted

Change https://golang.org/cl/93356 mentions this issue: cmd/gerritbot: reword message for when a Gerrit message is posted

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Feb 11, 2018

Change https://golang.org/cl/93357 mentions this issue: cmd/gerritbot: don’t post messages to GitHub over an hour old

Change https://golang.org/cl/93357 mentions this issue: cmd/gerritbot: don’t post messages to GitHub over an hour old

@andybons

This comment has been minimized.

Show comment
Hide comment
@andybons

andybons Feb 11, 2018

Member

Filed golang/go#23783 to further update documentation.

Member

andybons commented Feb 11, 2018

Filed golang/go#23783 to further update documentation.

gopherbot pushed a commit to golang/build that referenced this issue Feb 12, 2018

cmd/gerritbot: remind people to publish their draft comments
Update golang/go#18517

Change-Id: Ia970d648b85cd75831ee0c6969762752e9b49289
Reviewed-on: https://go-review.googlesource.com/93355
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

gopherbot pushed a commit to golang/build that referenced this issue Feb 12, 2018

cmd/gerritbot: reword message for when a Gerrit message is posted
Update golang/go#18517

Change-Id: Iccb7b5c61d8fc161b877d0a39f4ee1c523a4cd75
Reviewed-on: https://go-review.googlesource.com/93356
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Feb 12, 2018

Change https://golang.org/cl/93435 mentions this issue: cmd/gerritbot: don't query GitHub as much

Change https://golang.org/cl/93435 mentions this issue: cmd/gerritbot: don't query GitHub as much

@andybons

This comment has been minimized.

Show comment
Hide comment
@andybons

andybons Feb 12, 2018

Member

This is live. We now accept Pull Requests in all our repos so I’m going to close this as fixed.

🎉

Member

andybons commented Feb 12, 2018

This is live. We now accept Pull Requests in all our repos so I’m going to close this as fixed.

🎉

@andybons andybons closed this Feb 12, 2018

gopherbot pushed a commit to golang/build that referenced this issue Feb 13, 2018

cmd/gerritbot: don't query GitHub as much
Use maintner as a fast-path to determine whether comments are
duplicates or not.

Update golang/go#18517

Change-Id: I3ddc380e6e202371903ce62ae9e4c07e9805c1ed
Reviewed-on: https://go-review.googlesource.com/93435
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Feb 13, 2018

Member

For more information, see https://golang.org/wiki/GerritBot

Member

bradfitz commented Feb 13, 2018

For more information, see https://golang.org/wiki/GerritBot

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 13, 2018

Awesome work!

Awesome work!

@golang golang locked as resolved and limited conversation to collaborators Feb 13, 2018

@golang golang deleted a comment from JoaoGFarias Feb 13, 2018

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Apr 3, 2018

Change https://golang.org/cl/104577 mentions this issue: cmd/pushback: delete

Change https://golang.org/cl/104577 mentions this issue: cmd/pushback: delete

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.