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 >
