apex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Munagala Ramanath <...@datatorrent.com>
Subject Re: PR merge policy
Date Fri, 28 Apr 2017 03:11:26 GMT
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

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