Discussion of pull request / review / commit process

Development
#1

There are currently 41 open pull requests at https://github.com/theforeman/foreman/pulls

I suggest we discuss among the community how to refine the review / commit process with the goals of:

  1. broader feedback of PR's (not just from the maintainers)
  2. higher code quality and test coverage
  3. lesser waiting time until PR's are merged

The current process recently (but not always) is Ohad or Dominic reviewing a PR and sometimes asking @somebody to assist in review. As more PR's are submitted, the job of reviewing needs to be spread across more developers, but we need some guidelines to ensure quality code. Some options include:

  1. Ack from any one (1) developer
  2. Ack from minimum of any two (2) developers
  3. Ack from minimum of any three (3) developers
  4. Any one (1) core maintainer (Ohad, Dominic, Amos, Brian, Sam) - i.e. the current process

The discussion could be one option OR another option. Ex. minimum of two (2) developers OR one (1) core maintainer

Another question is what tool we want to use mange the process? I don't think Github provides "minimum 2 ack merges" out of the box. We would need to build something using their API. Other tools include GitLab, Gerrit, Atlassian/BitBucket, etc.

Let's hear what you think.

Regards,

Joseph Magen
Redhat

#2

> 1) Ack from any one (1) developer
> 2) Ack from minimum of any two (2) developers
> 3) Ack from minimum of any three (3) developers
> 4) Any one (1) core maintainer (Ohad, Dominic, Amos, Brian, Sam) - i.e. the current process

I don't think this is an issue of how are we doing the process, but
who is doing the review. Adopting options 1 to 4 will not help to
improve anything I guess.

If we are not able to efficiently volunteer for reviews, let's have
someone (a review nanny if you will) who would dispatch reviews that
are, for any reason, being stuck and no progress is there.

> Another question is what tool we want to use mange the process? I don't think Github provides "minimum 2 ack merges" out of the box. We would need to build something using their API. Other tools include GitLab, Gerrit, Atlassian/BitBucket, etc.

I love the agility of Pull Requests on github and I particularly love it
is rather discussion than formal form with some checkboxes or something.

If you really want to track thumbs up from N developers, we can create a
bot or something that will add a comment automatically in this format:

Review status:

  • [ ] Developer A
  • [ ] Developer B
  • [ ] Developer C

Github has support for checkboxes allowing you to tick them. But again,
I don't think this will improve things.

We should learn assigning reviews to ourselves everytime we think I know
the most information about that stuff. For tracking the "owner" that has
the final decision we could leverage "Assigned" flag in the Pull
Request. But for some reason, I am not able to edit that - perhaps I
don't have permission for that. Can someone recheck? Everyone should
have a veto right of course, this remains the same.

If this is not technically possible, how about autocomment in this
format:

Review status:

  • [ ] This PR has a main reviewer

Maybe we can add some additional checkboxes there like:

  • [ ] Packages are built (if new dependency is introduced)
  • [ ] No license issues
  • [ ] Source is commented :slight_smile:

So if I am going to take the PR, I am just checking the box. If review
nanny decides to assign an old PR to someone, he checks the box adding a
comment with @username to be assigned.

··· On Mon, Aug 05, 2013 at 07:46:27AM -0400, Joseph Magen wrote:

–
Later,

Lukas “lzap” Zapletal
irc: lzap #theforeman

#3

I am resurrecting this discussion about the pull request / review / commit process

We currently have 61 open PR's. Let's discuss how we can share the load, help each other out, and make all of us happier :slight_smile:

I suggest something like this. Please comment:

  • for trivial PR's, one ACK required. same person that ACK's can also merge (assuming he merge rights)
  • for non-trivial PR's, two ACK's required

Question: a review/ACK from Ohad or Dominic is always preferred, but what are the guidelines for having their review be a "must have" as opposed to "nice to have"?
I realize that non-trival PR's is a fuzzy definition in and of itself.

Your thoughts.

Regards,

Joseph

··· ----- Original Message ----- > From: "Lukas Zapletal" > To: "foreman-dev" > Sent: Monday, August 5, 2013 6:26:49 PM > Subject: Re: [foreman-dev] discussion of pull request / review / commit process > > On Mon, Aug 05, 2013 at 07:46:27AM -0400, Joseph Magen wrote: > > 1) Ack from any one (1) developer > > 2) Ack from minimum of any two (2) developers > > 3) Ack from minimum of any three (3) developers > > 4) Any one (1) core maintainer (Ohad, Dominic, Amos, Brian, Sam) - i.e. the > > current process > > I don't think this is an issue of *how* are we doing the process, but > *who* is doing the review. Adopting options 1 to 4 will not help to > improve anything I guess. > > If we are not able to efficiently volunteer for reviews, let's have > someone (a review nanny if you will) who would dispatch reviews that > are, for any reason, being stuck and no progress is there. > > > Another question is what tool we want to use mange the process? I don't > > think Github provides "minimum 2 ack merges" out of the box. We would > > need to build something using their API. Other tools include GitLab, > > Gerrit, Atlassian/BitBucket, etc. > > I love the agility of Pull Requests on github and I particularly love it > is rather discussion than formal form with some checkboxes or something. > > If you really want to track thumbs up from N developers, we can create a > bot or something that will add a comment automatically in this format: > > Review status: > > * [ ] Developer A > * [ ] Developer B > * [ ] Developer C > > Github has support for checkboxes allowing you to tick them. But again, > I don't think this will improve things. > > We should learn assigning reviews to ourselves everytime we think I know > the most information about that stuff. For tracking the "owner" that has > the final decision we could leverage "Assigned" flag in the Pull > Request. But for some reason, I am not able to edit that - perhaps I > don't have permission for that. Can someone recheck? Everyone should > have a veto right of course, this remains the same. > > If this is not technically possible, how about autocomment in this > format: > > Review status: > > * [ ] This PR has a main reviewer > > Maybe we can add some additional checkboxes there like: > > * [ ] Packages are built (if new dependency is introduced) > * [ ] No license issues > * [ ] Source is commented :-) > > So if I am going to take the PR, I am just checking the box. If review > nanny decides to assign an old PR to someone, he checks the box adding a > comment with @username to be assigned. > > -- > Later, > > Lukas "lzap" Zapletal > irc: lzap #theforeman > > -- > 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. > > >
#4

I like that. Defining trivial vs non-trivial however seems up to the
reviewer.
For the moment I'd go with trusting everybody's judgement, and making some
definition of it it only if it's necessary.

Just one comment, I don't think the 2nd ACK in non-trivial PRs has to come
from Dom or Ohad, but from someone knowledgeable in the topic, or with some
experience. Just as an example, for stuff such as roles and permissions, I
think Marek should be able to give the 2nd ACK. I don't think we have to
define per-person areas of expertise and we can organize ourselves without
a fixed document that specifies "Joseph 2nd ACKs API PRs, Ohad 2nd ACKS
etc…"

··· On Thu, Jul 3, 2014 at 4:34 PM, Joseph Magen wrote:

I am resurrecting this discussion about the pull request / review / commit
process

We currently have 61 open PR’s. Let’s discuss how we can share the load,
help each other out, and make all of us happier :slight_smile:

I suggest something like this. Please comment:

  • for trivial PR’s, one ACK required. same person that ACK’s can also
    merge (assuming he merge rights)
  • for non-trivial PR’s, two ACK’s required

Question: a review/ACK from Ohad or Dominic is always preferred, but what
are the guidelines for having their review be a “must have” as opposed to
"nice to have"?
I realize that non-trival PR’s is a fuzzy definition in and of itself.

Your thoughts.

Regards,

Joseph

----- Original Message -----

From: “Lukas Zapletal” lzap@redhat.com
To: “foreman-dev” foreman-dev@googlegroups.com
Sent: Monday, August 5, 2013 6:26:49 PM
Subject: Re: [foreman-dev] discussion of pull request / review / commit
process

On Mon, Aug 05, 2013 at 07:46:27AM -0400, Joseph Magen wrote:

  1. Ack from any one (1) developer
  2. Ack from minimum of any two (2) developers
  3. Ack from minimum of any three (3) developers
  4. Any one (1) core maintainer (Ohad, Dominic, Amos, Brian, Sam) -
    i.e. the

current process

I don’t think this is an issue of how are we doing the process, but
who is doing the review. Adopting options 1 to 4 will not help to
improve anything I guess.

If we are not able to efficiently volunteer for reviews, let’s have
someone (a review nanny if you will) who would dispatch reviews that
are, for any reason, being stuck and no progress is there.

Another question is what tool we want to use mange the process? I
don’t

think Github provides “minimum 2 ack merges” out of the box. We would
need to build something using their API. Other tools include GitLab,
Gerrit, Atlassian/BitBucket, etc.

I love the agility of Pull Requests on github and I particularly love it
is rather discussion than formal form with some checkboxes or something.

If you really want to track thumbs up from N developers, we can create a
bot or something that will add a comment automatically in this format:

Review status:

  • [ ] Developer A
  • [ ] Developer B
  • [ ] Developer C

Github has support for checkboxes allowing you to tick them. But again,
I don’t think this will improve things.

We should learn assigning reviews to ourselves everytime we think I know
the most information about that stuff. For tracking the “owner” that has
the final decision we could leverage “Assigned” flag in the Pull
Request. But for some reason, I am not able to edit that - perhaps I
don’t have permission for that. Can someone recheck? Everyone should
have a veto right of course, this remains the same.

If this is not technically possible, how about autocomment in this
format:

Review status:

  • [ ] This PR has a main reviewer

Maybe we can add some additional checkboxes there like:

  • [ ] Packages are built (if new dependency is introduced)
  • [ ] No license issues
  • [ ] Source is commented :slight_smile:

So if I am going to take the PR, I am just checking the box. If review
nanny decides to assign an old PR to someone, he checks the box adding a
comment with @username to be assigned.

–
Later,

Lukas “lzap” Zapletal
irc: lzap #theforeman

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

–
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/d/optout.

–
Daniel Lobato

@elobatoss
blog.daniellobato.me
daniellobato.me

GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30

#5

IMHO all devs who has access to a repo should be experienced enough to
waive things. With one exception - foreman-core (we have broader list of
ppl with merge access).

Therefore for the rest, I'd stick with the "review needed by someone
with merge access".

In short: If you see the big green merge button, go for review if the PR
is stale.

We should review who has merge access and distribute the workload a bit,
with the exception of foreman-core which I don't know what is the best
approach :slight_smile:

LZ

··· On Thu, Jul 03, 2014 at 04:43:14PM +0200, Daniel Lobato wrote: > I like that. Defining trivial vs non-trivial however seems up to the > reviewer. > For the moment I'd go with trusting everybody's judgement, and making some > definition of it it only if it's necessary. > > Just one comment, I don't think the 2nd ACK in non-trivial PRs has to come > from Dom or Ohad, but from someone knowledgeable in the topic, or with some > experience. Just as an example, for stuff such as roles and permissions, I > think Marek should be able to give the 2nd ACK. I don't think we have to > define per-person areas of expertise and we can organize ourselves without > a fixed document that specifies "Joseph 2nd ACKs API PRs, Ohad 2nd ACKS > etc..." > > > On Thu, Jul 3, 2014 at 4:34 PM, Joseph Magen wrote: > > > I am resurrecting this discussion about the pull request / review / commit > > process > > > > We currently have 61 open PR's. Let's discuss how we can share the load, > > help each other out, and make all of us happier :-) > > > > I suggest something like this. Please comment: > > > > * for trivial PR's, one ACK required. same person that ACK's can also > > merge (assuming he merge rights) > > * for non-trivial PR's, two ACK's required > > > > Question: a review/ACK from Ohad or Dominic is always preferred, but what > > are the guidelines for having their review be a "must have" as opposed to > > "nice to have"? > > I realize that non-trival PR's is a fuzzy definition in and of itself. > > > > Your thoughts. > > > > Regards, > > > > Joseph > > > > > > > > > > ----- Original Message ----- > > > From: "Lukas Zapletal" > > > To: "foreman-dev" > > > Sent: Monday, August 5, 2013 6:26:49 PM > > > Subject: Re: [foreman-dev] discussion of pull request / review / commit > > process > > > > > > On Mon, Aug 05, 2013 at 07:46:27AM -0400, Joseph Magen wrote: > > > > 1) Ack from any one (1) developer > > > > 2) Ack from minimum of any two (2) developers > > > > 3) Ack from minimum of any three (3) developers > > > > 4) Any one (1) core maintainer (Ohad, Dominic, Amos, Brian, Sam) - > > i.e. the > > > > current process > > > > > > I don't think this is an issue of *how* are we doing the process, but > > > *who* is doing the review. Adopting options 1 to 4 will not help to > > > improve anything I guess. > > > > > > If we are not able to efficiently volunteer for reviews, let's have > > > someone (a review nanny if you will) who would dispatch reviews that > > > are, for any reason, being stuck and no progress is there. > > > > > > > Another question is what tool we want to use mange the process? I > > don't > > > > think Github provides "minimum 2 ack merges" out of the box. We would > > > > need to build something using their API. Other tools include GitLab, > > > > Gerrit, Atlassian/BitBucket, etc. > > > > > > I love the agility of Pull Requests on github and I particularly love it > > > is rather discussion than formal form with some checkboxes or something. > > > > > > If you really want to track thumbs up from N developers, we can create a > > > bot or something that will add a comment automatically in this format: > > > > > > Review status: > > > > > > * [ ] Developer A > > > * [ ] Developer B > > > * [ ] Developer C > > > > > > Github has support for checkboxes allowing you to tick them. But again, > > > I don't think this will improve things. > > > > > > We should learn assigning reviews to ourselves everytime we think I know > > > the most information about that stuff. For tracking the "owner" that has > > > the final decision we could leverage "Assigned" flag in the Pull > > > Request. But for some reason, I am not able to edit that - perhaps I > > > don't have permission for that. Can someone recheck? Everyone should > > > have a veto right of course, this remains the same. > > > > > > If this is not technically possible, how about autocomment in this > > > format: > > > > > > Review status: > > > > > > * [ ] This PR has a main reviewer > > > > > > Maybe we can add some additional checkboxes there like: > > > > > > * [ ] Packages are built (if new dependency is introduced) > > > * [ ] No license issues > > > * [ ] Source is commented :-) > > > > > > So if I am going to take the PR, I am just checking the box. If review > > > nanny decides to assign an old PR to someone, he checks the box adding a > > > comment with @username to be assigned. > > > > > > -- > > > Later, > > > > > > Lukas "lzap" Zapletal > > > irc: lzap #theforeman > > > > > > -- > > > 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. > > > > > > > > > > > > > -- > > 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/d/optout. > > > > > > -- > Daniel Lobato > > @elobatoss > blog.daniellobato.me > daniellobato.me > > GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30 > > -- > 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/d/optout.

–
Later,

Lukas “lzap” Zapletal
irc: lzap #theforeman