Re: PR merge policy

[Vlad Rozov]

I agree that there is no silver bullet here. My view on the force push 
is that it is not so disruptive to the community as it looks to be. It 
is very easy to reset master to apache/github remote master and rebase 
branches if necessary assuming that it is not done frequently. I view 
multiple commits as a bad practice and IMO they will cause more 
confusion in a longer run. In a few years it will be hard to understand 
why one commit was followed by an undo commit, followed by another 
commit without doing a lot of research (especially that those commits 
may not necessarily be sequential).

Thank you,
Vlad

On 4/29/17 10:46, Pramod Immaneni wrote:
That is not the main point I was making. I think your main concern is that
when a commit gets added like this, the principle of putting community
first is being violated. That is well taken but what I am trying to say is
that your remedy is going the same route because you are focusing only on a
specific way of undoing the change and not considering how it is going to
affect the community. There is no silver bullet here but I think a few
extra commits are acceptable.

On Sat, Apr 29, 2017 at 9:51 AM, Vlad Rozov <v.ro...@datatorrent.com> wrote:

I don't think that anyone proposed shaming of individuals who violated the
policy especially in a public e-mail. Understanding of inconvenience caused
to the community is sufficient to avoid policy violations.

I would propose using PR to communicate to a committer and a contributor
request to undo the commit unless a commit was done without PR even being
open that will be a larger violation. In the later case, e-mail to dev@apex
is required.

I am strongly against commit on top of a commit. Undo needs to be done
quickly and reverting changes in extra commit still require review to be
sure that "undo" is complete. In addition, after PR review is done, we will
end up with 3 commits instead of one.

Thank you,

Vlad


On 4/29/17 08:29, Pramod Immaneni wrote:

I assume we are still referring to force push and removing the commit from
the upstream git commit tree as rollback and not to applying a new commit
that reverts the changes. When a violation happens, why should everyone
(who synced with the repo) suffer the inconvenience of their local git
repo/branch ending up with a bad git commit state for a problem they did
not cause, which would happen when rollback is employed, when this can be
easily fixed by a new commit that reverts the changes. Second, a violation
while bad, should the default policy be, to immediately revert the changes
with a new PR/commit without looking at what they are? That seems
unnecessarily draconian too, considering that violations are not the norm
nor flagrant that drastic measures need to be taken now. If the changes
are
questionable or even simply require more time for review, yes, by all
means, send an email to dev list with the reason for reverting the PR,
revert the changes with a new commit and redo the PR process. The email
serves two purposes, first it serves as a courtesy notification both to
the
committer and contributor as to why the commit is being reverted and
second it also ends up being a reminder to the committer that their action
affected the broader community, that they need to be cognizant of it and
be
more careful in the future. Also, I am against any public shaming of
individuals in the emails.

Thanks

On Fri, Apr 28, 2017 at 7:10 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:

I think it is a good idea to make the committer responsible for fixing the
situation by rolling back the commit and re-opening the PR for further
review. IMO, committer right comes with the responsibility to respect the
community and policies it established.

I would disagree that rolling back should be used only in case of a
disaster unless PR merge policy violation is a disaster :-) (and it
actually is).

Thank you,

Vlad


On 4/28/17 14:21, Amol Kekre wrote:

Strongly agree with Ilya. Lets take these events as learning
opportunities
for folks to learn and improve. There can always be second commit to fix
in
case there is code issue. If it is a policy issue, we learn and improve.
Rolling back, should be used rarely and it does need to be a disaster.
We
need to be cognizant of new contributors worrying about the cost to
submit
code.

I too do not think Apex is hurting from bad code getting in. We are
doing
great with our current policies.

Thks,
Amol


E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*

www.datatorrent.com


On Fri, Apr 28, 2017 at 1:35 PM, Ganelin, Ilya <
ilya.gane...@capitalone.com>
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

[image: 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>
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> wrote:

+1 as measure of last resort.

On Thu, Apr 27, 2017 at 9:25 PM, Vlad Rozov <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>
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
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






------------------------------

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.