I'm up for this if we deprecate the old way. Having two different processes seems like overkill. In general, I find the review interface of GitHub less expressive/clear but everything else is way better.
On Mon, Jun 22, 2015 at 6:59 PM, Steven Phillips <[email protected]> wrote: > +1 > > I am in favor of giving this a try. > > If I remember correctly, the reason we abandoned pull requests originally > was because we couldn't close the pull requests through Github. A solution > could be for whoever pushes the commit to the apache git repo to add the > Line "Closes <request number>". Github would then automatically close the > pull request. > > On Mon, Jun 22, 2015 at 1:02 PM, Jason Altekruse <[email protected] > > > wrote: > > > Hello Drill developers, > > > > I am writing this message today to propose allowing the use of github > pull > > requests to perform reviews in place of the apache reviewboard instance. > > > > Reviewboard has caused a number of headaches in the past few months, and > I > > think its time to evaluate the benefits of the apache infrastructure > > relative to the actual cost of using it in practice. > > > > For clarity of the discussion, we cannot use the complete github > workflow. > > Comitters will still need to use patch files, or check out the branch > used > > in the review request and push to apache master manually. I am not > > advocating for using a merging strategy with git, just for using the > github > > web UI for reviews. I expect anyone generating a chain of commits as > > described below to use the rebasing workflow we do today. Additionally > devs > > should only be breaking up work to make it easier to review, we will not > be > > reviewing branches that contain a bunch of useless WIP commits. > > > > A few examples of problems I have experienced with reviewboard include: > > corruption of patches when they are downloaded, the web interface showing > > inconsistent content from the raw diff, and random rejection of patches > > that are based directly on the head of apache master. > > > > These are all serious blockers for getting code reviewed and integrated > > into the master branch in a timely manner. > > > > In addition to serious bugs in reviewboard, there are a number of > > difficulties with the combination of our typical dev workflow and how > > reviewboard works with patches. As we are still adding features to Drill, > > we often have several weeks of work to submit in response to a JIRA or > > series of related JIRAs. Sometimes this work can be broken up into > > independent reviewable units, and other times it cannot. When a series of > > changes requires a mixture of refactoring and additions, the process is > > currently quite painful. Ether reviewers need to look through a giant > messy > > diff, or the submitters need to do a lot of extra work. This involves not > > only organizing their work into a reviewable series of commits, but also > > generating redundant squashed versions of the intermediate work to make > > reviewboard happy. > > > > For a relatively simple 3 part change, this involves creating 3 > reviewboard > > pages. The first will contain the first commit by itself. The second will > > have the first commits patch as a parent patch with the next change in > the > > series uploaded as the core change to review. For the third change, a > > squashed version of the first two commits must be generated to serve as a > > parent patch and then the third changeset uploaded as the reviewable > > change. Frequently a change to the first commit requires regenerating all > > of these patches and uploading them to the individual review pages. > > > > This gets even worse with larger chains of commits. > > > > It would be great if all of our changes could be small units of work, but > > very frequently we want to make sure we are ready to merge a complete > > feature before starting the review process. We need to have a better way > to > > manage these large review units, as I do not see the possibility of > > breaking up the work into smaller units as a likely solution. We still > have > > lots of features and system cleanup to work on. > > > > For anyone unfamiliar, github pull requests are based on a branch you > push > > to your personal fork. They give space for a general discussion, as well > as > > allow commenting inline on the diff. They give a clear reference to each > > commit in the branch, allowing reviewers to see each piece of work > > individually as well as provide a "squashed" view to see the overall > > differences. > > > > For the sake of keeping the project history connected to JIRA, we can see > > if there is enough automatic github integration or possibly upload patch > > files to JIRA each time we update a pull request. As an side note, if we > > don't need individual patches for reviewboard we could just put patch > files > > on JIRA that contain several commits. These are much easier to generate > an > > apply than a bunch of individual files for each change. This should > prevent > > JIRAs needing long lists of patches with names like > > DRILL-3000-part1-version3.patch > > > > > > -- > Steven Phillips > Software Engineer > > mapr.com >
