Done: https://issues.apache.org/jira/browse/INFRA-7254
On 23 January 2014 14:12, Noah Slater <[email protected]> wrote: > Okay, I'll leave this a few days and then request a Review Board > instance so we can experiment with it. > > Will be entirely voluntary! If it starts to become useful, we can > codify how we expect it to be used. Until that point, we can just send > our patches to Review Board and ask for a review on the mailing list. > > On 23 January 2014 03:30, Mutton, James <[email protected]> wrote: >> On 1/22/14 3:07 PM, "Russell Branca" <[email protected]> wrote: >> >> >>>Huge +1 to more review. >>> >>>Let's also setup some code guidelines for Erlang/Javascript/C/HTML so we >>>have an authoritative list of rules to follow to ensure code consistency, >>>and similarly, let's get some guidelines for git commits messages as well, >>>Tim Pope's article comes to mind: >>>http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html. >>> >>>Whichever review tool we decide to use, we should use it universally. We >>>should figure out a way to do github integration so that PR's show up in >>>the tool, and then we update the PR with a link to the relevant review >>>page. We also need a good way to initiate the analogue of a PR in whatever >>>tool we use. One of the complications with the review process today, is >>>that we're all accustomed to doing review through github PR's, but we can >>>use github PR's for branches who exist solely on the ASF origin. I imagine >>>this won't be an issue with Hive or Gerrit, but we should verify that >>>we're >>>not required to initiate code review from third party origins. >>> >>>On a side note, one of my personal nits with github PR's, that I hope >>>might >>>be resolved with one of these tools, is losing comments when the code >>>changes. For instance, if I do a review and make 10 comments that each >>>represent a line item that needs to be addressed, I would like to see a >>>task list on the review indicating there are unfinished items to be >>>addressed, basically a way to make a set of checkboxes necessary to >>>complete the review. I find it odd that github doesn't do this, even more >>>so by the fact that changing code to address comments will often mess with >>>the commenting and hide the messages you posted, making it challenging to >>>decipher whether the review items have been addressed. This is by no means >>>a requirement to using any approach, just hoping that someone knows of a >>>good way to approach this problem in whatever tool we use. >>> >>> >>>-Russell >>> >>> >>>On Wed, Jan 22, 2014 at 6:06 AM, Benoit Chesneau >>><[email protected]>wrote: >>> >>>> On Wed, Jan 22, 2014 at 12:47 PM, Noah Slater <[email protected]> >>>>wrote: >>>> >>>> > My first comment is: if we want more reviews, let's have more >>>>committers. >>>> > >>>> > We double our committer base in 2013, and the results look like this: >>>> > >>>> > https://www.ohloh.net/p/couchdb/analyses/latest/languages_summary >>>> > >>>> > And I see comments like this on StackOverflow: >>>> > >>>> > "I've recently noticed that Couch DB is back in heavy >>>>development." >>>> >>>> >>>> > So I think we should continue to aggressively recruit more committers >>>> > to the project in 2014. Excelsior! >>>> > >>>> >>>> Welcoming new committers in the project is certainly a good thing but I >>>> think it's orthogonal to the need of more review and team work. We >>>>should >>>> find a good workflow now while we are still a limited number of >>>>committers >>>> because It will be harder to enforce anything on a larger team. We >>>>should >>>> settle on some guidelines now. It will also help us to attract more >>>>people >>>> IMO. >>>> >>>> >>>> > >>>> > However, as for the way we do reviews... >>>> > >>>> > Infra ticket about Gerrit >>>> > https://issues.apache.org/jira/browse/INFRA-2205 >>>> > >>>> > Effort seems to be mothballed. But I'm sure we could restart it if we >>>> > were serious. >>>> > >>>> > However, we could use this: >>>> > >>>> > https://reviews.apache.org/groups/hive/ >>>> > >>>> > It is well integrated with Apache infrastructure already, sends mails >>>> > to the mailing list, and so on. >>>> > >>>> > Happy to request an instance and we can experiment with it if we like. >>>> > If it doesn't work out, we stop using it. No harm. Could make it >>>> > entirely voluntary until we figure out a workflow that we all like. >>>> > >>>> > Should I do that? >>>> > >>>> >>>> I thought gerrit was already integrated but any tool that could help us >>>>to >>>> make the review is OK for me. How hive compares to gerrit? Could we also >>>> plug it with github like gerrit [1] ? >>>> >>>> - benoît >>>> >>>> [1] https://gerrit.googlesource.com/plugins/github/ (and some others) >>>> >>>> > >>>> > On 20 January 2014 15:30, Nick North <[email protected]> wrote: >>>> > > On 20 January 2014 12:26, Dale Harvey <[email protected]> wrote: >>>> > > >>>> > >> I fully agree, its something I mentioned at the couchdb conf in >>>> > vancouver, >>>> > >> a review first system encourages contributions and has multiple >>>> benefits >>>> > >> >>>> > >> * At least 2 people look at the code, less likely to push silly >>>> > mistakes >>>> > >> * Can codify and practice review rules >>>> > >> * Its much easier to view the current activity in the project >>>> > >> * Can bring up points of discussion before its too late >>>> > >> >>>> > >> But I think the most important thing is that it removes the burden >>>>of >>>> > >> responsibility from the committer to the project as a whole, also >>>> hugely >>>> > >> beneficial is that it makes it particularly obvious when a patch >>>>has >>>> > reach >>>> > >> a stalemate and forces someone to make the call. >>>> > >> >>>> > >> For reference on PouchDB every committer is trusted to push code, >>>> nobody >>>> > >> (including myself) pushes their own code to master, it goes in the >>>>PR >>>> > queue >>>> > >> and gets a +1 from any other committer (who will usually push it), >>>> thats >>>> > >> essentially the same process we use at mozilla and coming to think >>>>of >>>> it >>>> > >> any other project I have worked on, any commiter has the ability to >>>> -1 a >>>> > >> patch at which point they give a reason and usually some solution >>>>gets >>>> > >> agreed on >>>> > >> >>>> > > >>>> > > I like this system, with one small quibble about responsibility. I >>>> don't >>>> > > think it should be seen as removing the burden of responsibility >>>>from >>>> the >>>> > > committer to the project as a whole. It becomes easy then for >>>>everyone, >>>> > > including the committer, to think someone else will find bugs, and >>>> no-one >>>> > > puts in enough effort. I'd say it is still primarily the >>>>responsibility >>>> > of >>>> > > the committer to ensure that code is error free, but that at least >>>>one >>>> > > person who knows that area of the code base should sign off on it. >>>>Some >>>> > > spreading of responsibility is good, but too much can actually lead >>>>to >>>> a >>>> > > decline in quality. >>>> > > >>>> > > >>>> > > Nick >>>> > >>>> > >>>> > >>>> > -- >>>> > Noah Slater >>>> > https://twitter.com/nslater >>>> > >>>> >> >> >> +1 to reviews and getting code guidelines. Nice to see that Apache has a >> reviewboard instance available (https://reviews.apache.org/groups/ hive >> link). I've spent a lot of time using reviewboard, it's got a good UI and >> API. Reviews are submitted using a separate command line tool >> (post-review) to actually post the review, but it lets you make multiple >> updates from feedback and allows others to see the diffs on the same >> review so you can see how a change evolves. You can leave comments or put >> a "ship-it" tag to +1 the commit, then from a distance it's possible to >> see all the reviews outstanding and how many votes, if any, they've >> received. It's friendly for non-blocking reviews too, since the code >> review process is separate. It also lets you can work on reviews a little >> at a time and save them in between instead of having to devote a block of >> time to code reviews (I do this sometimes on big changes). >> >> Garrit wants to gate the source-tree if I remember correctly so is >> possibly less friendly with remote repositories. >> >> </JamesM> >> > > > > -- > Noah Slater > https://twitter.com/nslater -- Noah Slater https://twitter.com/nslater
