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 >
