Mailing-List: contact dev-help@apex.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@apex.apache.org
MIME-Version: 1.0
In-Reply-To: <bf33a792-4119-bdce-303f-4326a60292e3@datatorrent.com>
References: <b67201e3-ca8b-1526-f65c-5847004b4ccc@datatorrent.com>
 <78fa9363-ad2e-63b8-8912-c158808d4796@datatorrent.com> <CAA7LveF2MqpDj5=FCo5-v3ntnnLT765zD9+LeZCYtUxTLnUnZg@mail.gmail.com>
 <cc29e176-d9b5-6dd1-aea4-58745a49fb22@datatorrent.com> <CA+5xAo2Qj0g2GL1+GoT-WV_g4B7V+t39k9EDBXS-TaL96mEo5Q@mail.gmail.com>
 <CAA7LveFLSOe9RFa6GeTzi=jm+DH2FpgVOWG5sZ3FkhnFXZJVTA@mail.gmail.com>
 <CA+5xAo05hRVV=H1s1LgvfoiFJY7ro3oH1=pAWEv0+fFzNch7Yw@mail.gmail.com>
 <CAA7LveGJs07kMaDRAp9+V7X2vSxu0X_VdBfSjih0i7rYeUykNg@mail.gmail.com>
 <CA+5xAo1tNpbHVxEXZFKrW-1U5r+UnY3y3yVa2oV_96zqaXogew@mail.gmail.com>
 <CAA7LveHhfwe39RVeqZUcgthRPb+aqCSYAFTov+WormTDUP1p5Q@mail.gmail.com>
 <7e8917b4-e211-632a-a7ba-6e4e902cf0c3@datatorrent.com> <CAA7LveG8QJJuo2+ap4YnbpQtO5uMPMSOM6WyqKGaEKx7SbcXzQ@mail.gmail.com>
 <bf33a792-4119-bdce-303f-4326a60292e3@datatorrent.com>
From: Munagala Ramanath <ram@datatorrent.com>
Date: Thu, 27 Apr 2017 20:11:26 -0700
Message-ID: <CAJ484Bicn2-0Pb=8t+SWkxaQAP=_ss94_i1adh_4UqMGM6sP7w@mail.gmail.com>
Subject: Re: PR merge policy
To: dev@apex.apache.org
Content-Type: multipart/alternative; boundary=001a1144e054dcf126054e316ddc
archived-at: Fri, 28 Apr 2017 03:11:43 -0000

--001a1144e054dcf126054e316ddc
Content-Type: text/plain; charset=UTF-8

I agree with Pramod that force pushing should be a rare event done only
when there is an immediate
need to undo something serious. Doing it just for a policy violation should
itself be codified in our
policies as a policy violation.

Why not just commit an improvement on top ?

Ram

On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.rozov@datatorrent.com> wrote:

> Violation of the PR merge policy is a sufficient reason to forcibly undo
> the commit and such violations affect everyone in the community.
>
> Thank you,
>
> Vlad
>
> On 4/27/17 19:36, Pramod Immaneni wrote:
>
>> I agree that PRs should not be merged without a chance for others to
>> review. I don't agree to force push and altering the commit tree unless it
>> is absolutely needed, as it affects everyone. This case doesn't warrant
>> this step, one scenario where a force push might be needed is if somebody
>> pushed some copyrighted code.
>>
>> Thanks
>>
>> On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov <v.rozov@datatorrent.com>
>> wrote:
>>
>> I am open to both approaches - two commits or a join commit. Both have
>>> pros and cons that we may discuss. What I am strongly against are PRs
>>> that
>>> are merged without a chance for other contributors/committers to review.
>>> There should be a way to forcibly undo such commits.
>>>
>>> Thank you,
>>>
>>> Vlad
>>>
>>>
>>> On 4/27/17 12:41, Pramod Immaneni wrote:
>>>
>>> My comments inline..
>>>>
>>>> On Thu, Apr 27, 2017 at 12:01 PM, Thomas Weise <thw@apache.org> wrote:
>>>>
>>>> I'm -1 on using the author field like this in Apex for the reason stated
>>>>
>>>>> (it is also odd to see something like this showing up without prior
>>>>> discussion).
>>>>>
>>>>>
>>>>> I am not set on this for future commits but would like to say, do we
>>>>>
>>>> really
>>>> verify the author field and treat it with importance. For example, I
>>>> don't
>>>> think we ever check if the author is the person they say they are, check
>>>> name, email etc. If I were to use slightly different variations of my
>>>> name
>>>> or email (not that I would do that) would reviewers really verify that.
>>>> I
>>>> also have checked that tools don't fail on reading a commit because
>>>> author
>>>> needs to be in a certain format. I guess contribution stats are the ones
>>>> that will be affected but if used rarely I dont see that being a big
>>>> problem. I can understand if we wanted to have strict requirements for
>>>> the
>>>> committer field.
>>>>
>>>> Thanks
>>>>
>>>>
>>>> Consider adding the additional author information to the commit message.
>>>>
>>>>> Thomas
>>>>>
>>>>> On Thu, Apr 27, 2017 at 11:55 AM, Pramod Immaneni <
>>>>> pramod@datatorrent.com>
>>>>> wrote:
>>>>>
>>>>> Agreed it is not regular and should only be used in special
>>>>> circumstances.
>>>>>
>>>>> One example of this is pair programming. It has been done before in
>>>>>> other
>>>>>> projects and searching on google or stackoverflow you can see how
>>>>>> other
>>>>>> people have tried to handle it
>>>>>>
>>>>>> https://bugs.eclipse.org/bugs/show_bug.cgi?id=375536
>>>>>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=451880
>>>>>> http://stackoverflow.com/questions/7442112/attributing-
>>>>>> a-single-commit-to-multiple-developers
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> On Thu, Apr 27, 2017 at 9:57 AM, Thomas Weise <thw@apache.org> wrote:
>>>>>>
>>>>>> commit 9856080ede62a4529d730bcb6724c757f5010990
>>>>>>
>>>>>>> Author: Pramod Immaneni & Vlad Rozov <pramod+v.rozov@datatorrent.com
>>>>>>> >
>>>>>>> Date:   Tue Apr 18 09:37:22 2017 -0700
>>>>>>>
>>>>>>> Please don't use the author field in such a way, it leads to
>>>>>>> incorrect
>>>>>>> tracking of contributions.
>>>>>>>
>>>>>>> Either have separate commits or have one author.
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Apr 27, 2017 at 9:31 AM, Pramod Immaneni <
>>>>>>>
>>>>>>> pramod@datatorrent.com
>>>>>> wrote:
>>>>>>
>>>>>>> The issue was two different plugin models were developed, one for
>>>>>>>
>>>>>>>> pre-launch and other for post-launch. I felt that the one built
>>>>>>>>
>>>>>>>> latter
>>>>>>>
>>>>>> was
>>>>>>
>>>>>>> better and it would be better to have a uniform interface for the
>>>>>>>>
>>>>>>>> users
>>>>>>>
>>>>>> and
>>>>>>
>>>>>>> hence asked for the changes.
>>>>>>>>
>>>>>>>> On Thu, Apr 27, 2017 at 9:05 AM, Thomas Weise <thw@apache.org>
>>>>>>>>
>>>>>>>> wrote:
>>>>>>>
>>>>>> I think the plugins feature could have benefited from better
>>>>>>
>>>>>>> original
>>>>>>>>
>>>>>>> review, which would have eliminated much of the back and forth
>>>>>>
>>>>>>> after
>>>>>>>>
>>>>>>> the
>>>>>>
>>>>>>> fact.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Thu, Apr 27, 2017 at 8:20 AM, Vlad Rozov <
>>>>>>>>>
>>>>>>>>> v.rozov@datatorrent.com
>>>>>>>>
>>>>>>> wrote:
>>>>>>
>>>>>>> Pramod,
>>>>>>>>>
>>>>>>>>>> No, it is not a request to update the apex.apache.org (to do
>>>>>>>>>>
>>>>>>>>>> that
>>>>>>>>>
>>>>>>>> we
>>>>>>
>>>>>> need
>>>>>>>
>>>>>>>> to file JIRA). It is a reminder that it is against Apex policy to
>>>>>>>>>>
>>>>>>>>>> merge
>>>>>>>>>
>>>>>>>> PR
>>>>>>>>
>>>>>>>>> without giving enough time for others to review it (few hours
>>>>>>>>>>
>>>>>>>>>> after
>>>>>>>>>
>>>>>>>> PR
>>>>>>
>>>>>>> was
>>>>>>>>
>>>>>>>>> open).
>>>>>>>>>>
>>>>>>>>>> Thank you,
>>>>>>>>>>
>>>>>>>>>> Vlad
>>>>>>>>>>
>>>>>>>>>> On 4/27/17 08:05, Pramod Immaneni wrote:
>>>>>>>>>>
>>>>>>>>>> Vlad, are you asking for a consensus on the policy to make it
>>>>>>>>>> official
>>>>>>>>>>
>>>>>>>>> (publish on website etc). I believe we have one. However, I did
>>>>>>>>
>>>>>>>>> not
>>>>>>>>>>
>>>>>>>>> see
>>>>>>>
>>>>>>>> much difference between what you said on Mar 26th to what I
>>>>>>>>>
>>>>>>>>>> proposed
>>>>>>>>>>
>>>>>>>>> on
>>>>>>>
>>>>>>>> Mar
>>>>>>>>>
>>>>>>>>>> 24th. Is the main difference any committer can merge (not just
>>>>>>>>>>>
>>>>>>>>>>> the
>>>>>>>>>>
>>>>>>>>> main
>>>>>>
>>>>>>> reviewer) as long as there are no objections from others. In
>>>>>>>>>
>>>>>>>>>> that
>>>>>>>>>>
>>>>>>>>> case,
>>>>>>
>>>>>>> I
>>>>>>>>>
>>>>>>>>> am fine with it.
>>>>>>>>>>
>>>>>>>>>>> On Thu, Apr 27, 2017 at 7:44 AM, Vlad Rozov <
>>>>>>>>>>>
>>>>>>>>>>> v.rozov@datatorrent.com>
>>>>>>>>>>
>>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> This is a friendly reminder regarding PR merge policy.
>>>>>>>>>>>
>>>>>>>>>>> Thank you,
>>>>>>>>>>>>
>>>>>>>>>>>> Vlad
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 3/23/17 12:58, Vlad Rozov wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Lately there were few instances where PR open against apex-core
>>>>>>>>>>>>
>>>>>>>>>>>> and
>>>>>>>>>>>
>>>>>>>>>> apex-malhar were merged just few hours after it being open and
>>>>>>>
>>>>>>>> JIRA
>>>>>>>>>>>>
>>>>>>>>>>> being
>>>>>>>>
>>>>>>>>> raised without giving chance for other contributors to review
>>>>>>>>>>>>>
>>>>>>>>>>>>> and
>>>>>>>>>>>>
>>>>>>>>>>> comment.
>>>>>>>
>>>>>>>> I'd suggest that we stop such practice no matter how trivial
>>>>>>>>>>>>>
>>>>>>>>>>>>> those
>>>>>>>>>>>>
>>>>>>>>>>> changes
>>>>>>>
>>>>>>>> are. This equally applies to documentation. In a rear cases
>>>>>>>>>>>>>
>>>>>>>>>>>>> where
>>>>>>>>>>>>
>>>>>>>>>>> PR
>>>>>>>
>>>>>>> is
>>>>>>>>
>>>>>>>>> urgent (for example one that fixes compilation error), I'd
>>>>>>>>>>
>>>>>>>>>>> suggest
>>>>>>>>>>>>
>>>>>>>>>>> that
>>>>>>>
>>>>>>>> a
>>>>>>>>>>
>>>>>>>>>>> committer who plans to merge the PR sends an explicit
>>>>>>>>>>>>>
>>>>>>>>>>>>> notification
>>>>>>>>>>>>
>>>>>>>>>>> to
>>>>>>>
>>>>>>>> dev@apex and gives others a reasonable time to respond.
>>>>>>>>>
>>>>>>>>>> Thank you,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Vlad
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>


-- 

_______________________________________________________

Munagala V. Ramanath

Software Engineer

E: ram@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam

www.datatorrent.com  |  apex.apache.org

--001a1144e054dcf126054e316ddc--