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

Review review tools #3708

Closed
stuhood opened this issue Jul 21, 2016 · 17 comments
Closed

Review review tools #3708

stuhood opened this issue Jul 21, 2016 · 17 comments
Assignees

Comments

@stuhood
Copy link
Sponsor Member

stuhood commented Jul 21, 2016

Switching to phabricator was essentially agreed upon as of the last summit, and the need to break away from the rbcommons.com/s/twitter/ url to an independent org will force the issue in the next few months.

On the other hand, there are a large handful of hosted, github-integrated review tools that we should do due-diligence on first. A few from @patricklaw's googling:
https://reviewable.io/
http://gerrithub.io/

Additionally, it would be worth evaluating whether we really need a UI independent of github's own PR UI, given that it now supports squash merges.

@stuhood stuhood added the infra label Jul 21, 2016
@stuhood stuhood self-assigned this Jul 21, 2016
@jsirois
Copy link
Member

jsirois commented Jul 21, 2016

I have historically been one of if not the only github objectionist. The way the github merge button worked was never an issue I had at least with github code review - I just curl the .patch suffix url into git am.

The issues I've always had with github PRs are, in order of severity:

  1. The chattiness of commenting - 1 email per, no batching
  2. No reasonable way to do interdiffs
  3. No useful keyboard control.

They've since added support for side-by-side diff which was also something I had hoped for.

@stuhood
Copy link
Sponsor Member Author

stuhood commented Jul 21, 2016

Very good points. Will make sure to include those considerations when looking at these.

@kwlzn
Copy link
Member

kwlzn commented Jul 21, 2016

note there's also a hosted phab offering from the folks that are actively developing it: https://www.phacility.com/

@kwlzn
Copy link
Member

kwlzn commented Jul 21, 2016

..and a +1 to @jsirois's github PR gripes + a 4) No multi-line comments.

@patricklaw
Copy link
Member

The pricing on hosted Phacility is a bit steep--I suspect we'd do fine self-hosting since we're already paying for the server.

I'm +1 for Phabricator, hosted or otherwise, and I'm happy to help with the prod/maintenance components. These days I'm not doing so much direct OSS contribution, but I enjoy the server config and maintenance.

I have all of the same grievances with GH PR + review, but I will say this: it's dirt simple to use and everyone else already knows how to use it. And it's free. I wouldn't complain if we decided to use it, but I'm also not such a big consumer of the CR tools these days, so I'm not affected as much by a bad UX.

If we decide to go the Phab route, we should decide precisely which pieces of Phab we want to use, and what our ideal workflow looks like.

@benjyw
Copy link
Sponsor Contributor

benjyw commented Jul 21, 2016

@jsirois is not the only GitHub objector. I, too, very much detest their code review facilities, for the same reasons, particularly the fact that it's all just comments that get sent as soon as you type them, with no batching. In fact, they don't have the concept of a "code review", just free-floating comments.

@benjyw
Copy link
Sponsor Contributor

benjyw commented Jul 21, 2016

Don't we have a working instance of Phab? Maybe we can try and play with that and see if we like it, before committing to it?

@patricklaw
Copy link
Member

We had one for a while @benjyw, but we kind of waffled on actually switching over to it. I'm sure we can get one back up, but the question will immediately become

  1. What's the new process?
  2. How do we beta test that process?
  3. How do we do the final switch-over?

@lenucksi
Copy link
Contributor

lenucksi commented Jul 21, 2016

As far as my 2cents go: I can recommend Gerrit and thus GerritHub - if it offers the stability and painlessness of standard self-hosted Gerrit. Basically Gerrit should address most of the pain points the GitHub PR interface was criticized for.
However, Gerrit provides a different workflow than the workflow of standard Git+GitHub.

Update: Please opt for something that has proper OAuth integration with GitHub. And RBCommons is really cringeworthy...Even compared to the f...ine UI of current Gerrit.

@ericzundel
Copy link
Member

Let me just put in my $.02 and say that I also don't like the review part of Github code review. I've used the predecessors of Gerrit and though they were great.

@patricklaw
Copy link
Member

So this issue has floated here for a while now. It seems like the consensus so far is that we should try Phabricator again. I can go ahead and get it running again on our dedicated ec2 machine if there's interest, and then we can poke around at it to evaluate workflows.

Any objections?

@stuhood
Copy link
Sponsor Member Author

stuhood commented Aug 8, 2016

@patricklaw : If you wouldn't mind getting it set up, that would be helpful. I will follow up on steps (2) and (3) this week.

@stuhood
Copy link
Sponsor Member Author

stuhood commented Oct 13, 2016

@benjyw : What are you thoughts on github review at this point? Potentially a feasible replacement?

If you and @jsirois are both not strongly opposed to github PR reviews at this point, I will probably draft a proposal for the workflow that would result from that switch to be reviewed at the summit.

@benjyw
Copy link
Sponsor Contributor

benjyw commented Oct 13, 2016

It's about 100x better than it was a month ago, that's for sure.

As far as I can tell it's now functionally equivalent to RBCommons, so I have no objection to using it. In fact, it would be nice to be better integrated, instead of having to link travis, github and RBCommons together by hand as we do now.

@stuhood
Copy link
Sponsor Member Author

stuhood commented Oct 17, 2016

@benjyw : Ok, thanks. I will draft a proposal for a new workflow before the summit.

@stuhood
Copy link
Sponsor Member Author

stuhood commented Oct 23, 2016

Posted https://rbcommons.com/s/twitter/r/4333/ with a proposal for what github reviews would look like.

Also reviewable on #3995

@stuhood
Copy link
Sponsor Member Author

stuhood commented Nov 19, 2016

It is done. e35dc2e

@stuhood stuhood closed this as completed Nov 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants