Guess we can all go home then. Our work here is done:

[cid:image001.png@01D2C024.5F2A5090]

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 <pra...@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.ro...@datatorrent.com<mailto:v.ro...@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 
<t...@apache.org<mailto:t...@apache.org>> wrote:

+1 as measure of last resort.
On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov 
<v.ro...@datatorrent.com<mailto:v.ro...@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.ro...@datatorrent.com<mailto:v.ro...@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 
<r...@datatorrent.com<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto:v.ro...@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.ro...@datatorrent.com<mailto: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<mailto:r...@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: r...@datatorrent.com<mailto:r...@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.

Reply via email to