apex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vlad Rozov <v.ro...@datatorrent.com>
Subject Re: PR merge policy
Date Sat, 29 Apr 2017 02:37:05 GMT
I don't think we should formalize the requirement to have at least 2 
sign-off on a PR before it is merged, but at the same time I would 
encourage more contributors and committers to review PRs. This will 
bring more collaboration and familiarity with how Apex works that Ilya 
mentioned and also will speedup PR review and improve quality of commits.

On a side note, we do allow committers to commit their own PRs after 
other committers sign-off on it. It is done to avoid empty merge 
commits. When reviewer and committer are part of the same organization 
and can coordinate proper rebase, committing own PR should be avoided.

Thank you,

Vlad

On 4/28/17 13:35, Ganelin, Ilya 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
>
> id:image001.png@01D1F7A4.F3D42980
>
> *From: *Pramod Immaneni <pramod@datatorrent.com>
> *Reply-To: *"dev@apex.apache.org" <dev@apex.apache.org>
> *Date: *Friday, April 28, 2017 at 10:09 AM
> *To: *"dev@apex.apache.org" <dev@apex.apache.org>
> *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 <v.rozov@datatorrent.com 
> <mailto:v.rozov@datatorrent.com>> 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 <thw@apache.org
>             <mailto:thw@apache.org>> wrote:
>
>             +1 as measure of last resort.
>
>                 On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov
>                 <v.rozov@datatorrent.com
>                 <mailto:v.rozov@datatorrent.com>> 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
>                     <v.rozov@datatorrent.com
>                     <mailto: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
>                             <mailto: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
>                                 <mailto: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
>                                     <mailto: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
>                                         <mailto: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
>                                                 <mailto: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
>                                                     <mailto: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
>                                                             <mailto: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
>                                                                 <mailto: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
>                                                                     <mailto: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
>                                                                     <mailto: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
>                                                                             <mailto:v.rozov@datatorrent.com>
>
>                                                                             wrote:
>
>                                                                     Pramod,
>
>                                                                     No,
>                                                                     it
>                                                                     is
>                                                                     not
>                                                                     a
>                                                                     request
>                                                                     to
>                                                                     update
>                                                                     the
>                                                                     apex.apache.org
>                                                                     <http://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
>                                                                                     <mailto: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
>                                         <mailto:ram@datatorrent.com> |
>                                         M: (408) 331-5034
>                                         <tel:%28408%29%20331-5034> |
>                                         Twitter: @UnknownRam
>
>                                         www.datatorrent.com
>                                         <http://www.datatorrent.com> |
>                                         apex.apache.org
>                                         <http://apex.apache.org>
>
>
>                                         --
>
>                                 _______________________________________________________
>
>                                 Munagala V. Ramanath
>
>                                 Software Engineer
>
>                                 E: ram@datatorrent.com
>                                 <mailto:ram@datatorrent.com> | M:
>                                 (408) 331-5034
>                                 <tel:%28408%29%20331-5034> | Twitter:
>                                 @UnknownRam
>
>                                 www.datatorrent.com
>                                 <http://www.datatorrent.com> |
>                                 apex.apache.org <http://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.
>


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