Return-Path: X-Original-To: apmail-cloudstack-dev-archive@www.apache.org Delivered-To: apmail-cloudstack-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 18FE618FA6 for ; Tue, 7 Jul 2015 07:39:40 +0000 (UTC) Received: (qmail 31673 invoked by uid 500); 7 Jul 2015 07:39:39 -0000 Delivered-To: apmail-cloudstack-dev-archive@cloudstack.apache.org Received: (qmail 31612 invoked by uid 500); 7 Jul 2015 07:39:39 -0000 Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@cloudstack.apache.org Delivered-To: mailing list dev@cloudstack.apache.org Received: (qmail 31589 invoked by uid 99); 7 Jul 2015 07:39:39 -0000 Received: from Unknown (HELO spamd4-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 07 Jul 2015 07:39:39 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd4-us-west.apache.org (ASF Mail Server at spamd4-us-west.apache.org) with ESMTP id EACE2C068C for ; Tue, 7 Jul 2015 07:39:38 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd4-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: -0.1 X-Spam-Level: X-Spam-Status: No, score=-0.1 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=disabled Authentication-Results: spamd4-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mx1-us-east.apache.org ([10.40.0.8]) by localhost (spamd4-us-west.apache.org [10.40.0.11]) (amavisd-new, port 10024) with ESMTP id 9XFFY8dsqSgG for ; Tue, 7 Jul 2015 07:39:24 +0000 (UTC) Received: from mail-wi0-f176.google.com (mail-wi0-f176.google.com [209.85.212.176]) by mx1-us-east.apache.org (ASF Mail Server at mx1-us-east.apache.org) with ESMTPS id 2E16042B0E for ; Tue, 7 Jul 2015 07:39:24 +0000 (UTC) Received: by wibdq8 with SMTP id dq8so174012274wib.1 for ; Tue, 07 Jul 2015 00:39:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=content-type:mime-version:subject:from:in-reply-to:date :content-transfer-encoding:message-id:references:to; bh=I6lLODN+LwCTwR96/hA8xMCVyF45gcLAUKZ1SSNQcM8=; b=cmGE3L8+wDr5Xj323zIuQ5qvIn4jAGA2KlBktnQBNUj/7pypIVPpPRp0Ltrv89dzSH yH2VwdAKP1rLpHdVXWyU3Y1zU/rRjYe6dhZOYbc0dEo4xm1SMKBx9vJBdW1GfdZO+cPU mwjmvVCPUfKtqLeikO5OOYZ7aa6X2YdrA/L4mIpuzWLUnrNMlHCq4ZEvIQwWKTtFToq8 ba21zrT+uikBtuymsLHGVSAJWg7RV/5TGQv8czyWW1j2o1a7Qi+u/sUeO1DFIf6I5V2Q RLrYHScDHk6cdZ0i+dnb0g/3hO7PRez4uDr6iKEGOT35a8tJu8dgyxBAG0aj7gVn6U83 NBug== X-Received: by 10.180.215.101 with SMTP id oh5mr61955038wic.6.1436254763393; Tue, 07 Jul 2015 00:39:23 -0700 (PDT) Received: from [10.0.0.9] (131.177.199.178.dynamic.wline.res.cust.swisscom.ch. [178.199.177.131]) by mx.google.com with ESMTPSA id q9sm51074413wiz.23.2015.07.07.00.39.21 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 07 Jul 2015 00:39:22 -0700 (PDT) Content-Type: text/plain; charset=windows-1252 Mime-Version: 1.0 (Mac OS X Mail 6.6 \(1510\)) Subject: Re: [PROPOSAL] Commit to master through PR only From: sebgoa In-Reply-To: Date: Tue, 7 Jul 2015 09:39:16 +0200 Content-Transfer-Encoding: quoted-printable Message-Id: References: <0F27DFBA-4207-4B6F-8427-6097B9AC2B37@gmail.com> <52058FBA-AFE4-4953-A13C-89BA752D994C@shapeblue.com> <8CCE9859D2CAFD45948DBF7145AFB98C29C51E75@SINPEX01CL01.citrite.net> <15270B5B-2676-4A33-B0B2-11DBF6BA536C@schubergphilis.com> To: dev@cloudstack.apache.org X-Mailer: Apple Mail (2.1510) On Jul 3, 2015, at 12:01 PM, Wilder Rodrigues = wrote: > Hi John, >=20 > If you look at the discrete operations wearing a hat of a Project = Manager, you won=92t care=85 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. >=20 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. >=20 > 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. >=20 > I=92m feeling inclined to send my next PR with squashed commits to see = if it will get reviewed properly and in an easy way. >=20 > Cheers, > Wilder=20 >=20 >=20 >> On 02 Jul 2015, at 20:35, John Burwell = wrote: >>=20 >> Wilder, >>=20 >> 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. >>=20 >> Thanks, >> -John >>=20 >> P.S. As an aside, if you encounter a 5000 class, I would encourage = you to decompose it rather than reformat the file. >>=20 >> [1]: = https://robots.thoughtbot.com/5-useful-tips-for-a-better-commit-message >>=20 >> --- >> John Burwell (@john_burwell) >> VP of Software Engineering, ShapeBlue >> (571) 403-2411 | +44 20 3603 0542 >> http://www.shapeblue.com >>=20 >>=20 >>=20 >>> On Jul 2, 2015, at 2:43 AM, Wilder Rodrigues = wrote: >>>=20 >>> Sateesh and Rajesh, >>>=20 >>> 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? >>>=20 >>> I wil give you the simplest example that could come to my mind to = encourage no squash: >>>=20 >>> * 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=92s say, a 3 lines = change, then I commit the change again. >>>=20 >>> Now, think about the PR. It will contain 2 commits: 1 with the = formatting changes only; and a second commit with 3 lines change. >>>=20 >>> Would you like to see it quashed and all messed up? It would be very = difficult to review. >>>=20 >>> That=92s just a simple example. >>>=20 >>> Cheers, >>> Wilder >>>=20 >>>> On 02 Jul 2015, at 07:22, Rajesh Battala = wrote: >>>>=20 >>>> +1 for squashing commit >>>>=20 >>>> -----Original Message----- >>>> From: John Burwell [mailto:john.burwell@shapeblue.com] >>>> Sent: Thursday, July 2, 2015 12:14 AM >>>> To: dev@cloudstack.apache.org >>>> Subject: Re: [PROPOSAL] Commit to master through PR only >>>>=20 >>>> All, >>>>=20 >>>> I think we should stick to 2 votes per PR. Defining types of PRs = becomes difficult bordering on the arbitrary =97 adding a process = complexity and the potential to start debating if a particular PR is one = type or another. >>>>=20 >>>> 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=92s. >>>>=20 >>>> 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. >>>>=20 >>>> Thanks, >>>> -John >>>>=20 >>>> [1]: https://github.com/graydon/bors >>>>=20 >>>> --- >>>> John Burwell (@john_burwell) >>>> VP of Software Engineering, ShapeBlue >>>> (571) 403-2411 | +44 20 3603 0542 >>>> http://www.shapeblue.com >>>>=20 >>>>=20 >>>>=20 >>>>> On Jul 1, 2015, at 1:48 PM, Rohit Yadav = wrote: >>>>>=20 >>>>> Hi, >>>>>=20 >>>>>> On 25-Jun-2015, at 4:38 pm, Sebastien Goasguen = wrote: >>>>>>=20 >>>>>> 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]. >>>>>>=20 >>>>>> However to get started on the right track. I would like to = propose the following easy step: >>>>>>=20 >>>>>> Starting Monday June 29th (next monday): >>>>>>=20 >>>>>> - 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. >>>>>=20 >>>>> +1 >>>>>=20 >>>>> I=92ve 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=92s = certainly do-able without investing a lot of effort to do it, perhaps = can done everyday or every few days in a week. >>>>>=20 >>>>> Some suggestions and comments to improve PR reviewing/merging: >>>>>=20 >>>>> - 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: >>>>>=20 >>>>> = http://blog.remibergsma.com/2015/05/24/accepting-pull-requests-the-easy-wa= y/ >>>>>=20 >>>>> - Let=92s try to send PR around on one issue or one broad issue, = or against a JIRA ticket; but avoid unrelated sub-systems etc >>>>>=20 >>>>> - 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. >>>>>=20 >>>>> - Certain PRs that are typographical fixes, doc fixes and tooling = related fixes - so let=92s review and merge them if we=92ve at least one = green review (=93LGTM=94), though changes to CloudStack mgmt server, = agent and plugins codebase IMO should have at least 2 green reviews = (=93LGTM=94). >>>>>=20 >>>>>> 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. >>>>>>=20 >>>>>> There is no tooling involved, just human agreement. >>>>>>=20 >>>>>> cheers, >>>>>=20 >>>>> Regards, >>>>> Rohit Yadav >>>>> Software Architect, ShapeBlue >>>>> M. +91 88 262 30892 | rohit.yadav@shapeblue.com >>>>> Blog: bhaisaab.org | Twitter: @_bhaisaab >>>>>=20 >>>>>=20 >>>>>=20 >>>>> Find out more about ShapeBlue and our range of CloudStack related = services >>>>>=20 >>>>> IaaS Cloud Design & = Build >>>>> CSForge =96 rapid IaaS deployment = framework >>>>> CloudStack = Consulting >>>>> CloudStack Software = Engineering >>>>> CloudStack Infrastructure = Support >>>>> CloudStack Bootcamp Training = Courses >>>>>=20 >>>>> 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. >>>>=20 >>>> Find out more about ShapeBlue and our range of CloudStack related = services >>>>=20 >>>> IaaS Cloud Design & = Build >>>> CSForge =96 rapid IaaS deployment = framework >>>> CloudStack Consulting >>>> CloudStack Software = Engineering >>>> CloudStack Infrastructure = Support >>>> CloudStack Bootcamp Training = Courses >>>>=20 >>>> 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. >>>=20 >>=20 >> Find out more about ShapeBlue and our range of CloudStack related = services >>=20 >> IaaS Cloud Design & = Build >> CSForge =96 rapid IaaS deployment = framework >> CloudStack Consulting >> CloudStack Software = Engineering >> CloudStack Infrastructure = Support >> CloudStack Bootcamp Training = Courses >>=20 >> 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. >=20