apex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vlad Rozov <v.ro...@datatorrent.com>
Subject Re: PR merge policy
Date Fri, 28 Apr 2017 04:12:28 GMT
I also was under impression that everyone agreed to the policy that 
gives everyone in the community a chance to raise a concern or to 
propose an improvement to a PR. Unfortunately, it is not the case, and 
we need to discuss it again. I hope that this discussion will lead to no 
future violations so we don't need to forcibly undo such commits, but it 
will be good for the community to agree on the policy that deals with 
violations.

Ram, committing an improvement on top of a commit should be discouraged, 
not encouraged as it eventually leads to the policy violation and lousy 
PR reviews.

Thank you,

Vlad

On 4/27/17 20:54, Thomas Weise wrote:
> I also thought that everybody was in agreement about that after the first
> round of discussion and as you say it would be hard to argue against it.
> And I think we should not have to be back to the same topic a few days
> later.
>
> While you seem to be focussed on the disagreement on policy violation, I'm
> more interested in a style of collaboration that does not require such
> discussion.
>
> Thomas
>
> On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <ram@datatorrent.com>
> wrote:
>
>> Everybody seems agreed on what the committers should do -- that waiting a
>> day or two for
>> others to have a chance to comment seems like an entirely reasonable thing.
>>
>> The disagreement is about what to do when that policy is violated.
>>
>> Ram
>>
>> On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <thw@apache.org> wrote:
>>
>>> Forced push should not be necessary if committers follow the development
>>> process.
>>>
>>> So why not focus on what the committer should do? Development process and
>>> guidelines are there for a reason, and the issue was raised before.
>>>
>>> In this specific case, there is a string of commits related to the plugin
>>> feature that IMO should be part of the original review. There wasn't any
>>> need to rush this, the change wasn't important for the release.
>>>
>>> Thomas
>>>
>>>
>>> On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <ram@datatorrent.com>
>>> wrote:
>>>
>>>> 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
>>>>
>>
>>
>> --
>>
>> _______________________________________________________
>>
>> Munagala V. Ramanath
>>
>> Software Engineer
>>
>> E: ram@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam
>>
>> www.datatorrent.com  |  apex.apache.org
>>


Mime
View raw message