apex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Amol Kekre <a...@datatorrent.com>
Subject Re: PR merge policy
Date Sat, 29 Apr 2017 02:24:47 GMT
That makes sense. The committer should fix upon feedback. PR policy
violation is bad, I am not defending the violation.  think if the committer
takes the feedback and promptly works on a fix (new pr) it should be ok.

Thks,
Amol



E:amol@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*

www.datatorrent.com


On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.rozov@datatorrent.com> wrote:

> I think it is a good idea to make the committer responsible for fixing the
> situation by rolling back the commit and re-opening the PR for further
> review. IMO, committer right comes with the responsibility to respect the
> community and policies it established.
>
> I would disagree that rolling back should be used only in case of a
> disaster unless PR merge policy violation is a disaster :-) (and it
> actually is).
>
> Thank you,
>
> Vlad
>
> On 4/28/17 14:21, Amol Kekre wrote:
>
>> Strongly agree with Ilya. Lets take these events as learning opportunities
>> for folks to learn and improve. There can always be second commit to fix
>> in
>> case there is code issue. If it is a policy issue, we learn and improve.
>> Rolling back, should be used rarely and it does need to be a disaster. We
>> need to be cognizant of new contributors worrying about the cost to submit
>> code.
>>
>> I too do not think Apex is hurting from bad code getting in. We are doing
>> great with our current policies.
>>
>> Thks,
>> Amol
>>
>>
>> E:amol@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*
>>
>> www.datatorrent.com
>>
>>
>> On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya <
>> Ilya.Ganelin@capitalone.com>
>> wrote:
>>
>> Guess we can all go home then. Our work here is done:
>>>
>>>
>>>
>>>
>>> W.R.T the discussion below, I think rolling back an improperly reviewed
>>> PR
>>> could be considered disrespectful to the committer who merged it in the
>>> first place. I think that such situations, unless they trigger a
>>> disaster,
>>> should be handled by communicating the error to the responsible party and
>>> then allowing them to resolve it. E.g. I improperly commit an unreviewed
>>> PR, someone notices and sends me an email informing me of my error, and I
>>> then have the responsibility of unrolling the change and getting the
>>> appropriate review. I think we should start with the premise that we’re
>>> here in the spirit of collaboration and we should create opportunities
>>> for
>>> individuals to learn from their mistakes, recognize the importance of
>>> particular standards (e.g. good review process leads to stable projects),
>>> and ultimately internalize these ethics.
>>>
>>>
>>>
>>> Internally to our team, we’ve had great success with a policy requiring
>>> two PR approvals and not allowing the creator of a patch to be the one to
>>> merge their PR. While this might feel a little silly, it definitely helps
>>> to build collaboration, familiarity with the code base, and intrinsically
>>> avoids PRs being merged too quickly (without a sufficient period for
>>> review).
>>>
>>>
>>>
>>>
>>>
>>> - Ilya Ganelin
>>>
>>> [image: id:image001.png@01D1F7A4.F3D42980]
>>>
>>>
>>>
>>> *From: *Pramod Immaneni <pramod@datatorrent.com>
>>> *Reply-To: *"dev@apex.apache.org" <dev@apex.apache.org>
>>> *Date: *Friday, April 28, 2017 at 10:09 AM
>>> *To: *"dev@apex.apache.org" <dev@apex.apache.org>
>>> *Subject: *Re: PR merge policy
>>>
>>>
>>>
>>>
>>> On a lighter note, looks like the powers that be have been listening on
>>> this conversation and decided to force push an empty repo or maybe
>>> github just decided that this is the best proposal ;)
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> On Thu, Apr 27, 2017 at 10:47 PM, Vlad Rozov <v.rozov@datatorrent.com>
>>> wrote:
>>>
>>> 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
>>>
>>>
>>>
>>>
>>>
>>>
>>> ------------------------------
>>>
>>> The information contained in this e-mail is confidential and/or
>>> proprietary to Capital One and/or its affiliates and may only be used
>>> solely in performance of work or services for Capital One. The
>>> information
>>> transmitted herewith is intended only for use by the individual or entity
>>> to which it is addressed. If the reader of this message is not the
>>> intended
>>> recipient, you are hereby notified that any review, retransmission,
>>> dissemination, distribution, copying or other use of, or taking of any
>>> action in reliance upon this information is strictly prohibited. If you
>>> have received this communication in error, please contact the sender and
>>> delete the material from your computer.
>>>
>>>
>

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