I will add one more question:How many contributors/committers can tell how many force pushes were done to apex repositories since graduation without looking into commits@apex.
Thank you, Vlad On 4/29/17 11:48, Munagala Ramanath wrote:
Looks like we have clarity now that we are at an impasse: -1 on forced revert and -1 on the only other alternative with is additional commits. We need a continuing resolution ( https://federalnewsradio.com/federal-headlines/2017/04/short-term-spending-measure-introduced-to-keep-government-open/ ) to keep going :-) Does anybody know if: 1. Any other Apache project has a similar forced-revert policy; and 2. Any Apache project has actually force-reverted a commit for any reason in recent years, regardless of whether they have a policy about it or not. These additional data points will help in arriving at a resolution. Ram On Sat, Apr 29, 2017 at 10:46 AM, Pramod Immaneni <[email protected]> wrote: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 <[email protected]> wrote:I don't think that anyone proposed shaming of individuals who violatedthepolicy especially in a public e-mail. Understanding of inconveniencecausedto 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 todev@apexis 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, wewillend 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 commitfromthe upstream git commit tree as rollback and not to applying a newcommitthat 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 canbeeasily fixed by a new commit that reverts the changes. Second, aviolationwhile bad, should the default policy be, to immediately revert thechangeswith a new PR/commit without looking at what they are? That seems unnecessarily draconian too, considering that violations are not thenormnor 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 theiractionaffected 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 <[email protected]> wrote: I think it is a good idea to make the committer responsible for fixingthesituation by rolling back the commit and re-opening the PR for further review. IMO, committer right comes with the responsibility to respectthecommunity 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 learningopportunities for folks to learn and improve. There can always be second commit tofixin case there is code issue. If it is a policy issue, we learn andimprove.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:[email protected] | M: 510-449-2606 | Twitter: @*amolhkekre* www.datatorrent.com On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya < [email protected]> wrote: Guess we can all go home then. Our work here is done:W.R.T the discussion below, I think rolling back an improperlyreviewedPR could be considered disrespectful to the committer who merged it inthefirst 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 thatwe’rehere in the spirit of collaboration and we should createopportunitiesfor 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 policyrequiringtwo PR approvals and not allowing the creator of a patch to be theoneto 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:[email protected]] *From: *Pramod Immaneni <[email protected]> *Reply-To: *"[email protected]" <[email protected]> *Date: *Friday, April 28, 2017 at 10:09 AM *To: *"[email protected]" <[email protected]> *Subject: *Re: PR merge policy On a lighter note, looks like the powers that be have been listeningonthis 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 <[email protected]>wrote: In this case please propose how to deal with PR merge policyviolationsin 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 <[email protected]> wrote: +1 as measure of last resort. On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov <[email protected] 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 <[email protected] 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 proposeanimprovement to a PR. Unfortunately, it is not the case, and we needtodiscuss it again. I hope that this discussion will lead to no future violations so we don't need to forcibly undo such commits, but itwillbe 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 andlousyPR 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 fewdayslater. While you seem to be focussed on the disagreement on policyviolation,I'm more interested in a style of collaboration that does not requiresuchdiscussion. Thomas On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath < [email protected] wrote: Everybody seems agreed on what the committers should do -- thatwaitinga 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 <[email protected]>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 < [email protected] 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 <[email protected] 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 < [email protected]> 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 <[email protected]> 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 < [email protected]> 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 <[email protected]> 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 < [email protected] 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 <[email protected] 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 < [email protected] 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 < [email protected]> 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: [email protected] | M: (408) 331-5034 | Twitter: @UnknownRam www.datatorrent.com | apex.apache.org -- _______________________________________________________ Munagala V. Ramanath Software Engineer E: [email protected] | 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 ofanyaction in reliance upon this information is strictly prohibited. Ifyouhave received this communication in error, please contact the sender and delete the material from your computer.
