+1
The key part is the fact that a reviewer (different from the author) has
to put a comment LGTM (Looks Good To Me) in the PR.
With this, the reviewer or the author can push the change.
Thanks Frances !
Regards
JB
On 03/21/2016 05:53 PM, Frances Perry wrote:
I'd like to suggest a slightly stronger review process. In the draft
guidelines
<https://docs.google.com/document/d/1syFyfqIsGOYDE_Hn3ZkRd8a6ylcc64Kud9YtrGHgU0E/>
it currently says all PRs should get reviewed, regardless of author. The
only real exception to that is something that is an emergency fix, like
rolling back a bad PR to fix a build break. Even then, use a PR for test
coverage and communication -- just merge immediately.
Given that we're currently merging multiple disparate code bases, I think
we'll benefit by increasing the number of folks familiar with the code. And
in my experience, good quality code reviews are the best way to do that.
And even when I own a chunk of code and think I know it inside out, code
reviews still turn up subtle issues I missed.
It's not a matter of trust, it's about building a strong community and
spreading our knowledge to each other. I, for one, would love to learn more
about Flink and Beam ;-)
Frances
On Mon, Mar 21, 2016 at 3:06 AM, Jean-Baptiste Onofré <[email protected]>
wrote:
Hi Max,
fully agree, my previous e-mail was more my own mea culpa as I pushed a
change without PR (of course it was minor change as it concerned the legal
NOTICE and DISCLAINER files ;)).
So, my intend was just a global reminder, including myself ;)
Regards
JB
On 03/21/2016 10:50 AM, Maximilian Michels wrote:
Hi JB,
If I remember correctly from the past discussions, we agreed that we
want a PR-review process for all code changes which are
important/major or break the API in some way. I wholeheartedly agree
with this process.
In addition, committers preserve the right to provide small fixes
which do not change important logic of the system. Ideally, this
should only be changes that are part of the component which the
committer is responsible for. In this way, we can focus on important
pull requests and still provide meaningful fixes. It is the
committer's responsibility to make sure those fixes don't hinder other
committers' work. If necessary, committers should sync with each other
before pushing fixes. In rare cases, commits can be reverted.
Pull requests are a very important tool in the open source development
process. Whenever possible, let's use them but please let's not
enforce them. Community is also about trusting each other. Direct
pushes should be used with care and shouldn't be the normal
development process. On the other side, pull requests which get merged
away immediately are not meaningful either.
So far I think we are doing a good job. Please correct me if you feel
differently.
Best,
Max
On Sun, Mar 20, 2016 at 8:27 PM, Jean-Baptiste Onofré <[email protected]>
wrote:
Hi all,
As a reminder, we agreed that everyone in the project should use the same
workflow: prepare a PR, submit the PR, give some time to review, apply
the
PR.
Right now, we had some pushes directly without a PR.
It would be great to we *all* use the same workflow.
Thanks !
Regards
JB
--
Jean-Baptiste Onofré
[email protected]
http://blog.nanthrax.net
Talend - http://www.talend.com
--
Jean-Baptiste Onofré
[email protected]
http://blog.nanthrax.net
Talend - http://www.talend.com
--
Jean-Baptiste Onofré
[email protected]
http://blog.nanthrax.net
Talend - http://www.talend.com
