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 <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.
