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