On Jul 3, 2015, at 12:01 PM, Wilder Rodrigues <wrodrig...@schubergphilis.com> wrote:
> Hi John, > > If you look at the discrete operations wearing a hat of a Project Manager, > you won’t care… neither would I. However, from a Software Engineer > perspective, as much as the other people contributing with the Java code, I > do care and it really makes reviewing the PR easier. > The PR should not be squashed until it's reviewed and accepted. I am only arguing for squashing it when it is accepted and before merge. For now, I would love for us to focus on the 2 LGTM and green tests (as much as we can get them green). We can fine tune later. -sebastien > As a consumer of my change (project manager hat again), you should be looking > for Design Documents. Those will for sure show the motivation behind the > changes in a higher level. > > Concerning the 5k lines classes, I have found a few of them and they haven > been refactored accordingly. Have a look at the Virtual Router, > Citrix/LibVirt resource classes, those were cleaned as much as they could. > The example I gave was simple and should not be used in such a way, Think of > it as a 100 lines class instead, perhaps it will help. > > I’m feeling inclined to send my next PR with squashed commits to see if it > will get reviewed properly and in an easy way. > > Cheers, > Wilder > > >> On 02 Jul 2015, at 20:35, John Burwell <john.burw...@shapeblue.com> wrote: >> >> Wilder, >> >> In the grand scheme of the entire project history (e.g. reading git log), >> why do I care about these discrete operations? In six months (or long), I >> (as the consumer of your change) want to know what motivated this change >> which is completely lost in those two commits. I have found this advice [1] >> for commit messages combined with squashing commits to a topic (e.g. defect >> ticket, feature, enhancement ticket, etc) yields a git history that >> incredible value over the long term in a projects with a lot of developers >> making many changes. >> >> Thanks, >> -John >> >> P.S. As an aside, if you encounter a 5000 class, I would encourage you to >> decompose it rather than reformat the file. >> >> [1]: https://robots.thoughtbot.com/5-useful-tips-for-a-better-commit-message >> >> --- >> John Burwell (@john_burwell) >> VP of Software Engineering, ShapeBlue >> (571) 403-2411 | +44 20 3603 0542 >> http://www.shapeblue.com >> >> >> >>> On Jul 2, 2015, at 2: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. >>> >> >> 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. >
