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 05:47:53 GMT
In this case please propose how to deal with PR merge policy violations 
in the future. I will -1 proposal to commit an improvement on top of a 
commit.

Thank you,

Vlad

On 4/27/17 21:48, Pramod Immaneni wrote:
> I am sorry but I am -1 on the force push in this case.
>
>> On Apr 27, 2017, at 9:27 PM, Thomas Weise <thw@apache.org> wrote:
>>
>> +1 as measure of last resort.
>>
>>> On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov <v.rozov@datatorrent.com> wrote:
>>>
>>> IMO, force push will bring enough consequent embarrassment to avoid such
>>> behavior in the future.
>>>
>>> Thank you,
>>>
>>> Vlad
>>>
>>>> On 4/27/17 21:16, Munagala Ramanath wrote:
>>>>
>>>> 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
>>>>>>>
>>>>>>>
>>>>>>>


Mime
View raw message