Wilder, you are being to friendly IMHO here. I would actually -1 a PR that contains a commit that tries to do more then one thing. It is obfuscating and makes it harder to discuss implementations and fixes.
<psuedo-quote>in commit abcdef, in the part that fixes the NPE related to the network implementation of sdn-xyz in hypervisor-123, a problem arises that had not been there if in commit fedcba for the TO of the vm we had... </psuedo-quote> such argumentations should be "commit abcdef wasn't necessary, just revert commit fedcba". On Thu, Jul 2, 2015 at 8:43 AM, Wilder Rodrigues <wrodrig...@schubergphilis.com> wrote: > Sateesh and Rajesh, > > It seems you were the only guys who +1 the squash idea. Could you please > share with us what benefits you think squashing commits will bring? > > I wil give you the simplest example that could come to my mind to encourage > no squash: > > * I open a Java class with 5 thousand lines. The first thing I do is format > the code and commit the change. > * I go back to the class and apply the fix, let’s say, a 3 lines change, then > I commit the change again. > > Now, think about the PR. It will contain 2 commits: 1 with the formatting > changes only; and a second commit with 3 lines change. > > Would you like to see it quashed and all messed up? It would be very > difficult to review. > > That’s just a simple example. > > Cheers, > Wilder > >> On 02 Jul 2015, at 07:22, Rajesh Battala <rajesh.batt...@citrix.com> wrote: >> >> +1 for squashing commit >> >> -----Original Message----- >> From: John Burwell [mailto:john.burw...@shapeblue.com] >> Sent: Thursday, July 2, 2015 12:14 AM >> To: dev@cloudstack.apache.org >> Subject: Re: [PROPOSAL] Commit to master through PR only >> >> All, >> >> I think we should stick to 2 votes per PR. Defining types of PRs becomes >> difficult bordering on the arbitrary — adding a process complexity and the >> potential to start debating if a particular PR is one type or another. >> >> I agree regarding the fast forward, and feel that all PRs should squashed >> down to one commit. Ultimately, intermediate commits that seem informative >> in a feature branch become noise in a history as large as CloudStack’s. >> >> To enforce the policy and ensure that PRs are merged in an orderly and >> correct manner (i.e. one at time), I think we should consider adopting a >> tool such as bors [1] to verify that the merge passes all tests and then >> performs the merge. It would some minor modification to require two votes, >> but I doubt that would take much effort to implement. If there is interest, >> I would happy to make those changes for the project. >> >> Thanks, >> -John >> >> [1]: https://github.com/graydon/bors >> >> --- >> John Burwell (@john_burwell) >> VP of Software Engineering, ShapeBlue >> (571) 403-2411 | +44 20 3603 0542 >> http://www.shapeblue.com >> >> >> >>> On Jul 1, 2015, at 1:48 PM, Rohit Yadav <rohit.ya...@shapeblue.com> wrote: >>> >>> Hi, >>> >>>> On 25-Jun-2015, at 4:38 pm, Sebastien Goasguen <run...@gmail.com> wrote: >>>> >>>> A few of us are in Amsterdam at DevOps days. We are chatting about release >>>> management procedure. >>>> Remi is working on a set of principles that he will put on the wiki to >>>> start a [DISCUSS]. >>>> >>>> However to get started on the right track. I would like to propose the >>>> following easy step: >>>> >>>> Starting Monday June 29th (next monday): >>>> >>>> - Only commit through PR will land on master (after a minimum of 2 LGTM >>>> and green Travis results) >>>> - Direct commit will be reverted >>>> - Any committer can merge the PR. >>> >>> +1 >>> >>> I’ve been trying to help close PRs, it was difficult at first but then I >>> found some tooling to help me do that. I think it’s certainly do-able >>> without investing a lot of effort to do it, perhaps can done everyday or >>> every few days in a week. >>> >>> 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/ >>> >>> - 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 >>> >>> - 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. >>> >>> - 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”). >>> >>>> Goal being to start having a new practice -everything through PR for >>>> everyone- which is an easy way to gate our own commits building up to a PR. >>>> >>>> There is no tooling involved, just human agreement. >>>> >>>> cheers, >>> >>> Regards, >>> Rohit Yadav >>> Software Architect, ShapeBlue >>> M. +91 88 262 30892 | rohit.ya...@shapeblue.com >>> Blog: bhaisaab.org | Twitter: @_bhaisaab >>> >>> >>> >>> Find out more about ShapeBlue and our range of CloudStack related services >>> >>> IaaS Cloud Design & >>> Build<http://shapeblue.com/iaas-cloud-design-and-build//> >>> CSForge – rapid IaaS deployment framework<http://shapeblue.com/csforge/> >>> CloudStack Consulting<http://shapeblue.com/cloudstack-consultancy/> >>> CloudStack Software >>> Engineering<http://shapeblue.com/cloudstack-software-engineering/> >>> CloudStack Infrastructure >>> Support<http://shapeblue.com/cloudstack-infrastructure-support/> >>> CloudStack Bootcamp Training >>> Courses<http://shapeblue.com/cloudstack-training/> >>> >>> This email and any attachments to it may be confidential and are intended >>> solely for the use of the individual to whom it is addressed. Any views or >>> opinions expressed are solely those of the author and do not necessarily >>> represent those of Shape Blue Ltd or related companies. If you are not the >>> intended recipient of this email, you must neither take any action based >>> upon its contents, nor copy or show it to anyone. Please contact the sender >>> if you believe you have received this email in error. Shape Blue Ltd is a >>> company incorporated in England & Wales. ShapeBlue Services India LLP is a >>> company incorporated in India and is operated under license from Shape Blue >>> Ltd. Shape Blue Brasil Consultoria Ltda is a company incorporated in Brasil >>> and is operated under license from Shape Blue Ltd. ShapeBlue SA Pty Ltd is >>> a company registered by The Republic of South Africa and is traded under >>> license from Shape Blue Ltd. ShapeBlue is a registered trademark. >> >> Find out more about ShapeBlue and our range of CloudStack related services >> >> IaaS Cloud Design & Build<http://shapeblue.com/iaas-cloud-design-and-build//> >> CSForge – rapid IaaS deployment framework<http://shapeblue.com/csforge/> >> CloudStack Consulting<http://shapeblue.com/cloudstack-consultancy/> >> CloudStack Software >> Engineering<http://shapeblue.com/cloudstack-software-engineering/> >> CloudStack Infrastructure >> Support<http://shapeblue.com/cloudstack-infrastructure-support/> >> CloudStack Bootcamp Training >> Courses<http://shapeblue.com/cloudstack-training/> >> >> This email and any attachments to it may be confidential and are intended >> solely for the use of the individual to whom it is addressed. Any views or >> opinions expressed are solely those of the author and do not necessarily >> represent those of Shape Blue Ltd or related companies. If you are not the >> intended recipient of this email, you must neither take any action based >> upon its contents, nor copy or show it to anyone. Please contact the sender >> if you believe you have received this email in error. Shape Blue Ltd is a >> company incorporated in England & Wales. ShapeBlue Services India LLP is a >> company incorporated in India and is operated under license from Shape Blue >> Ltd. Shape Blue Brasil Consultoria Ltda is a company incorporated in Brasil >> and is operated under license from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a >> company registered by The Republic of South Africa and is traded under >> license from Shape Blue Ltd. ShapeBlue is a registered trademark. > -- Daan
