Vlad, Your point regarding not merging any PR without allowing the community to review is well taken. I agree that the committer should be responsible for rolling back the change and I think we should establish the way in which it should be done.
There can be a delay for the committer to notice that a commit should not have been merged and needs to be reverted. But it might be a while before the committer realizes this and a force push might create problems for the community. Perhaps a new PR could be the way to go. Note that I agree that undoing a PR may not even be needed given that the community has had a chance to review it, but there might still be cases where such undo commits need to be done. ~ Bhupesh _______________________________________________________ Bhupesh Chawda E: [email protected] | Twitter: @bhupeshsc www.datatorrent.com | apex.apache.org On Sat, Apr 29, 2017 at 11:07 AM, Vlad Rozov <[email protected]> wrote: > If a committer is fast to pull the trigger to merge and slow to rollback > when the policy is violated, the case can be sent to PMC. It will be a > clear indication that a committer does not respect the community, so I > disagree that this is "just kicking the can down the road" as committer > right may be eventually revoked (hope we don't need to go that far ever). > > Not all policies are immediately reflected at > http://apex.apache.org/contributing.html. The vote clearly happened a > week ago when I initially proposed that a PR can be merged only once the > community has a chance to review it . http://apex.apache.org/contrib > uting.html is for new contributors and new committers, existing > committers should be fully aware of the PR merge policy and if in doubt, > raise a question here (dev@apex). > > We are all humans and may overlook problems in PRs that we review. The > concern is not that Travis build may fail and a commit needs to be > reverted. It will not be a valid reason to undo the commit (note that it is > still committer responsibility to thoroughly review changes/new code and > ensure that license is in place, build is free from compilation and other > errors, code is optimal and readable, and basically put his/her name next > to the contributor name). The concern is when a committer does not give > community a chance for review. It is against "community before code" > behavior that we want to prohibit. I am strongly for requesting a > contributor to undo a commit in such cases and disallowing additional "fix" > commits. > > Thank you, > > Vlad > > On 4/28/17 20:17, Munagala Ramanath wrote: > >> That's just kicking the can down the road: What if the committer is not >> inclined >> to perform the rollback ? Other reviewers can provide feedback on the >> closed PR >> but the committer may choose to add a new commit on top. >> >> Firstly, the point about waiting a day or two is not even a formal policy >> requirement >> outlined at http://apex.apache.org/contributing.html in the "Merging a >> Pull >> Request" >> section, so it only has an advisory status currently in my view. >> >> Secondly, I think a variety of so called "violations" from overlooking >> Travis build >> failures, to not detecting missing unit tests, to not enforcing JavaDoc >> requirements >> are not fatal in most cases. Reverting commits in such cases is contrary >> to >> the >> Apache injunction to "Put community before code" ( >> http://community.apache.org/newbiefaq.html) >> >> So I am firmly against reverting except in the most dire circumstances and >> in favor >> of adding new commits to fix issues -- we do that for bugs, no reason to >> treat these >> other cases differently. >> >> Ram >> >> On Fri, Apr 28, 2017 at 7:24 PM, Amol Kekre <[email protected]> wrote: >> >> 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:[email protected] | M: 510-449-2606 | Twitter: @*amolhkekre* >>> >>> www.datatorrent.com >>> >>> >>> 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 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:[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 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:[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 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 <[email protected] >>>>>> > >>>>>> 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 <[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 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 < >>>>>> >>>>> [email protected] >>> >>>> 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 <[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 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. >>>>>> >>>>>> >>>>>> >> >> >
