apex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pramod Immaneni <pra...@datatorrent.com>
Subject Re: PR merge policy
Date Fri, 28 Apr 2017 02:36:38 GMT
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
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message