apex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thomas Weise <...@apache.org>
Subject Re: PR merge policy
Date Fri, 28 Apr 2017 04:26:29 GMT
That's an interesting thought also. Here is one more: Committer on the
project should require demonstrated responsible behavior in this area.
Proven ability to collaborate and doing the right things will be a criteria
for such nominations on my list.


On Thu, Apr 27, 2017 at 9:16 PM, Munagala Ramanath <ram@datatorrent.com>
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.rozov@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 <ram@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 <thw@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 <
> ram@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.rozov@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.rozov@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 <thw@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
<
> >>>>>>>>>> pramod@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 <thw@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 <
> >>>>>>>>>>>>
> >>>>>>>>>>>> pramod@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 <
> thw@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.rozov@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.rozov@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: ram@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam
> >>>>>
> >>>>> www.datatorrent.com  |  apex.apache.org
> >>>>>
> >>>>>
> >>>
> >>> --
> >>>
> >>> _______________________________________________________
> >>>
> >>> Munagala V. Ramanath
> >>>
> >>> Software Engineer
> >>>
> >>> E: ram@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam
> >>>
> >>> www.datatorrent.com  |  apex.apache.org
> >>>
> >>>
> >
>
>
> --
>
> _______________________________________________________
>
> Munagala V. Ramanath
>
> Software Engineer
>
> E: ram@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam
>
> www.datatorrent.com  |  apex.apache.org
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message