You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Jean-Baptiste Onofré <jb...@nanthrax.net> on 2016/03/20 20:27:38 UTC
Committer workflow
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é
jbonofre@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com
Re: Committer workflow
Posted by Amit Sela <am...@gmail.com>.
+1 for knowledge-sharing and getting to know other pieces of our code base.
I would definitely be interested in getting to know Beam better, apart from
the parts a runner developer deals with.
On Mon, Mar 21, 2016, 20:19 Kenneth Knowles <kl...@google.com.invalid> wrote:
> +1 for emphasizing the knowledge-sharing aspect of review.
>
> I think it is the most important for project health, and the most fun too!
> I love the chance to learn about a new piece of code (or learn how I messed
> up in my own code :-)
>
Re: Committer workflow
Posted by Kenneth Knowles <kl...@google.com.INVALID>.
+1 for emphasizing the knowledge-sharing aspect of review.
I think it is the most important for project health, and the most fun too!
I love the chance to learn about a new piece of code (or learn how I messed
up in my own code :-)
Re: Committer workflow
Posted by Jean-Baptiste Onofré <jb...@nanthrax.net>.
+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é <jb...@nanthrax.net>
> 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é <jb...@nanthrax.net>
>>> 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é
>>>> jbonofre@apache.org
>>>> http://blog.nanthrax.net
>>>> Talend - http://www.talend.com
>>>>
>>>
>> --
>> Jean-Baptiste Onofré
>> jbonofre@apache.org
>> http://blog.nanthrax.net
>> Talend - http://www.talend.com
>>
>
--
Jean-Baptiste Onofré
jbonofre@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com
Re: Committer workflow
Posted by Frances Perry <fj...@google.com.INVALID>.
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é <jb...@nanthrax.net>
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é <jb...@nanthrax.net>
>> 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é
>>> jbonofre@apache.org
>>> http://blog.nanthrax.net
>>> Talend - http://www.talend.com
>>>
>>
> --
> Jean-Baptiste Onofré
> jbonofre@apache.org
> http://blog.nanthrax.net
> Talend - http://www.talend.com
>
Re: Committer workflow
Posted by Jean-Baptiste Onofré <jb...@nanthrax.net>.
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é <jb...@nanthrax.net> 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é
>> jbonofre@apache.org
>> http://blog.nanthrax.net
>> Talend - http://www.talend.com
--
Jean-Baptiste Onofré
jbonofre@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com
Re: Committer workflow
Posted by Maximilian Michels <mx...@apache.org>.
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é <jb...@nanthrax.net> 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é
> jbonofre@apache.org
> http://blog.nanthrax.net
> Talend - http://www.talend.com