cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebgoa <run...@gmail.com>
Subject Re: [PROPOSAL] Commit to master through PR only
Date Tue, 07 Jul 2015 07:39:16 GMT

On Jul 3, 2015, at 12:01 PM, Wilder Rodrigues <wrodrigues@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.burwell@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 <wrodrigues@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.battala@citrix.com>
wrote:
>>>> 
>>>> +1 for squashing commit
>>>> 
>>>> -----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
>>>> 
>>>> 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.yadav@shapeblue.com>
wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>>> On 25-Jun-2015, at 4:38 pm, Sebastien Goasguen <runseb@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.yadav@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.
> 


Mime
View raw message