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:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre* www.datatorrent.com On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@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:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre* >> >> www.datatorrent.com >> >> >> On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya < >> ilya.gane...@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 <pra...@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.ro...@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 <t...@apache.org> wrote: >>> >>> +1 as measure of last resort. >>> >>> On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov <v.ro...@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.ro...@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 <r...@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 <t...@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 < >>> r...@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.ro...@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.ro...@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 <t...@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 < >>> >>> pra...@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 <t...@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 < >>> >>> pra...@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 <t...@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.ro...@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.ro...@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: r...@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam >>> >>> www.datatorrent.com | apex.apache.org >>> >>> >>> -- >>> >>> _______________________________________________________ >>> >>> Munagala V. Ramanath >>> >>> Software Engineer >>> >>> E: r...@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. >>> >>> >
