That is not the main point I was making. I think your main concern is that when a commit gets added like this, the principle of putting community first is being violated. That is well taken but what I am trying to say is that your remedy is going the same route because you are focusing only on a specific way of undoing the change and not considering how it is going to affect the community. There is no silver bullet here but I think a few extra commits are acceptable.
On Sat, Apr 29, 2017 at 9:51 AM, Vlad Rozov <v.ro...@datatorrent.com> wrote: > I don't think that anyone proposed shaming of individuals who violated the > policy especially in a public e-mail. Understanding of inconvenience caused > to the community is sufficient to avoid policy violations. > > I would propose using PR to communicate to a committer and a contributor > request to undo the commit unless a commit was done without PR even being > open that will be a larger violation. In the later case, e-mail to dev@apex > is required. > > I am strongly against commit on top of a commit. Undo needs to be done > quickly and reverting changes in extra commit still require review to be > sure that "undo" is complete. In addition, after PR review is done, we will > end up with 3 commits instead of one. > > Thank you, > > Vlad > > > On 4/29/17 08:29, Pramod Immaneni wrote: > >> I assume we are still referring to force push and removing the commit from >> the upstream git commit tree as rollback and not to applying a new commit >> that reverts the changes. When a violation happens, why should everyone >> (who synced with the repo) suffer the inconvenience of their local git >> repo/branch ending up with a bad git commit state for a problem they did >> not cause, which would happen when rollback is employed, when this can be >> easily fixed by a new commit that reverts the changes. Second, a violation >> while bad, should the default policy be, to immediately revert the changes >> with a new PR/commit without looking at what they are? That seems >> unnecessarily draconian too, considering that violations are not the norm >> nor flagrant that drastic measures need to be taken now. If the changes >> are >> questionable or even simply require more time for review, yes, by all >> means, send an email to dev list with the reason for reverting the PR, >> revert the changes with a new commit and redo the PR process. The email >> serves two purposes, first it serves as a courtesy notification both to >> the >> committer and contributor as to why the commit is being reverted and >> second it also ends up being a reminder to the committer that their action >> affected the broader community, that they need to be cognizant of it and >> be >> more careful in the future. Also, I am against any public shaming of >> individuals in the emails. >> >> Thanks >> >> 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. >>>>> >>>>> >>>>> >
