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 04:16:48 GMT
My thought was that leaving the bad commit would be a permanent reminder to
the committer
(and others) that a policy violation occurred and the consequent
embarrassment would be an
adequate deterrent.

Ram

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

> 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
>>>
>>>
>


-- 

_______________________________________________________

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