Hi all,
The volume of contributions to Foreman, Foreman proxy, and the installer
has risen dramatically over the last few months - at present we have ~60
open PRs across our codebase. That's absolutely amazing, and I am
constantly blown away by the contributions you guys make.
Some of those contributions are relatively complex, and require time, good
reviews, and some back-and-forth discussion about best practices, test
structures and so on. That's all fine. However, some contributions are
relatively quick, but might get backed up behind some of the bigger
discussions. That's not fine - simple stuff should be merged as soon as we
have a consensus that thinks it should be.
So, I wanted to open a discussion about how people think things should be
done. I think the open questions are:
- Do we agree the current system needs changing (or is it just me)?
- How do we handle building consensus?
- When do we merge?
- Who gets merge rights to the lead branch?
For myself, I envisage a process something like:
- A simple PR comes in
- Passes the Jenkins tests
- Gets an ACK from some (2?) committers (or perhaps other knowledgable
people?)
- Gets merged the next day.
4a) Author's shouldn't merge their own work
I suspect whether a PR is deemed simple or complex will be decided by
those who feel experienced enough to review, test and ACK it 
We also need to decide who has commit rights. I did a grep though the git
logs for the last year, and came up with the top 6 authors (for
foreman-core)
311 Author: Ohad Levy
129 Author: Amos Benari
61 Author: Joseph Mitchell Magen
38 Author: Dominic Cleal
35 Author: Sam Kottler
20 Author: Greg Sutcliffe
Do people think that's the right list? Is authorship a good metric? Are
there people that should / shouldn't be on the list? Should the list be
different for each project?
Looking forward to your thoughts…
Greg
> Hi all,
>
> The volume of contributions to Foreman, Foreman proxy, and the installer
> has risen dramatically over the last few months - at present we have ~60
> open PRs across our codebase. That's absolutely amazing, and I am
> constantly blown away by the contributions you guys make.
>
> Some of those contributions are relatively complex, and require time, good
> reviews, and some back-and-forth discussion about best practices, test
> structures and so on. That's all fine. However, some contributions are
> relatively quick, but might get backed up behind some of the bigger
> discussions. That's not fine - simple stuff should be merged as soon as we
> have a consensus that thinks it should be.
>
> So, I wanted to open a discussion about how people think things should be
> done. I think the open questions are:
>
> 1) Do we agree the current system needs changing (or is it just me)?
>
Yes, but the new system needs to ensure consistent reviews still take place
and the right people look at and test the code.
> 2) How do we handle building consensus?
> 3) When do we merge?
> 4) Who gets merge rights to the lead branch?
>
> For myself, I envisage a process something like:
>
> 1) A simple PR comes in
> 2) Passes the Jenkins tests
> 3) Gets an ACK from some (2?) committers (or perhaps other knowledgable
> people?)
> 4) Gets merged the next day.
> 4a) Author's shouldn't merge their own work
>
+1 to the person who opened the pull request never merging it themselves.
I'm not sure the "next day" thing works well - I'd rather the second ACK'er
(if qualified) merges the PR. An important part of this is for the people
who are giving ACK's to actually review and test the code as well as
identify potential technical debt before it gets merged. Therefore, the
people who ACK should have a complete understanding of the code that's
being changed. Others can review and test as well, but they shouldn't ACK.
>
> I suspect whether a PR is deemed simple or complex will be decided by
> those who feel experienced enough to review, test and ACK it
>
> We also need to decide who has commit rights. I did a grep though the git
> logs for the last year, and came up with the top 6 authors (for
> foreman-core)
>
> 311 Author: Ohad Levy
> 129 Author: Amos Benari
> 61 Author: Joseph Mitchell Magen
> 38 Author: Dominic Cleal
> 35 Author: Sam Kottler
> 20 Author: Greg Sutcliffe
>
> Do people think that's the right list? Is authorship a good metric? Are
> there people that should / shouldn't be on the list? Should the list be
> different for each project?
>
>
I don't consider authorship to be a particularly good metric for who gets
commit access. It's affected by the way people write code - for example,
some people contribute through a large number of small patches. Those
people might not deserve commit access. There are others who contribute a
small number of commits but their large amounts of code. I think getting
commit access is more a qualitative thing than quantitative. If people are
nice, active in the community, willing to contribute, and have made
contributions in the past then they should get commit access. The fact that
there are current committers who aren't on the top 6 list shows this point
pretty well I think.
···
On Mon, Apr 8, 2013 at 10:43 AM, Greg Sutcliffe wrote:
Looking forward to your thoughts…
Greg
–
You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
> +1 to the person who opened the pull request never merging it
> themselves. I'm not sure the "next day" thing works well - I'd rather the
> second ACK'er (if qualified) merges the PR. An important part of this is
> for the people who are giving ACK's to actually review and test the code as
> well as identify potential technical debt before it gets merged. Therefore,
> the people who ACK should have a complete understanding of the code that's
> being changed. Others can review and test as well, but they shouldn't ACK.
+1 to all of that. The review process is the core concept.
> I don't consider authorship to be a particularly good metric for who gets
> commit access. It's affected by the way people write code - for example,
> some people contribute through a large number of small patches. Those
> people might not deserve commit access. There are others who contribute a
> small number of commits but their large amounts of code. I think getting
> commit access is more a qualitative thing than quantitative. If people are
> nice, active in the community, willing to contribute, and have made
> contributions in the past then they should get commit access. The fact that
> there are current committers who aren't on the top 6 list shows this point
> pretty well I think.
>
All good points - I just had to start with some form of list, and wanted
something slightly better than "names that occurred to me just now" I
think I would like to add Ewoud to the installer group, for example.
···
On 8 April 2013 15:56, Sam Kottler wrote:
Any update on this? Does anyone else have input on how we can improve the merge process?
···
----- Original Message -----
From: "Greg Sutcliffe"
To: "Foreman ."
Sent: Monday, April 8, 2013 11:13:49 AM
Subject: Re: [foreman-dev] Redesigning the review process for Foreman core / Foreman proxy contributions
On 8 April 2013 15:56, Sam Kottler sam@kottlerdevelopment.com wrote:
+1 to the person who opened the pull request never merging it
themselves. I’m not sure the “next day” thing works well - I’d rather the
second ACK’er (if qualified) merges the PR. An important part of this is
for the people who are giving ACK’s to actually review and test the code as
well as identify potential technical debt before it gets merged. Therefore,
the people who ACK should have a complete understanding of the code that’s
being changed. Others can review and test as well, but they shouldn’t ACK.
+1 to all of that. The review process is the core concept.
I don’t consider authorship to be a particularly good metric for who gets
commit access. It’s affected by the way people write code - for example,
some people contribute through a large number of small patches. Those
people might not deserve commit access. There are others who contribute a
small number of commits but their large amounts of code. I think getting
commit access is more a qualitative thing than quantitative. If people are
nice, active in the community, willing to contribute, and have made
contributions in the past then they should get commit access. The fact that
there are current committers who aren’t on the top 6 list shows this point
pretty well I think.
All good points - I just had to start with some form of list, and wanted
something slightly better than “names that occurred to me just now” I
think I would like to add Ewoud to the installer group, for example.
–
You received this message because you are subscribed to the Google Groups “foreman-dev” group.
To unsubscribe from this group and stop receiving emails from it, send an email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
