I'm afraid I don't agree on some of points here, Rohit. On Wed, Jul 1, 2015 at 7:48 PM, Rohit Yadav <rohit.ya...@shapeblue.com> wrote: ...
> Some suggestions and comments to improve PR reviewing/merging: > > - Let's merge the PR commits in a fast forward way instead of doing a branch > merge that introduces frivolous merge commits. This is one approach to do > quickly and painlessly: > > http://blog.remibergsma.com/2015/05/24/accepting-pull-requests-the-easy-way/ Most specifically I am a great proponent of merge commits. The make very clear that a bunch of work belongs together and was merged at a certain point in time. > > - Let’s try to send PR around on one issue or one broad issue, or against a > JIRA ticket; but avoid unrelated sub-systems etc Now this I strongly agree on with you. > > - If there are not many changes (say less than 100-200 lines were changed), > let's have the changes melded into one commit. This can be done either by the > PR author or by the committer. The immediate benefit is that all the changes > will be much easy to port across other branches, easy to view and follow > git-log, and easy to revert-able. Only if the change is rally doing only one thing. It is the merge commit that should make it easily revertible. > > - Certain PRs that are typographical fixes, doc fixes and tooling related > fixes - so let’s review and merge them if we’ve at least one green review > (“LGTM”), though changes to CloudStack mgmt server, agent and plugins > codebase IMO should have at least 2 green reviews (“LGTM”). this is one I agree on again :) (score for those keeping: 2-2) -- Daan
