[katello-devel] Merging and Pulling with no wait

[Eric Helms]

We have previously adhered to a sense of self-responsibility on the part of the pull-requester.  They can leave it wide open on who reviews, or request reviews.  They can choose to put someone in charge of review+merge or merge it themselves with a review.  A waiting period seems to detract from that, especially without a reliable way to clearly know if the time period is up.

I do, however, agree with Jason's point that sometimes we do seem hasty to merge here lately.  That being said, and given the previous caveat, we as developers should be exercising our responsibility to as Tom said, request other eyes specifically on given pull requests, and let "large" pull requests sit longer than others.  We don't all have time to get to reading a pull request as soon as it is opened, but I think the combination of tested (and linted) pull requests and greater developer awareness should help alleviate (and if it does not, we can re-visit).

Two things:
 1) Tom, based on your notion of where people specialize, perhaps we should take a page from the Aeolus playbook http://aeolusproject.org/aeolus_team.html
 2) Should we add a caveat to our process, whereby a developer can request the desire to review a pull request prior to it's merging for cases where you don't have time at that exact moment?  Similar to how the author of a pull request can request review.


-Eric

----- Original Message -----
From: "Tom McKay" <thomasmckay at redhat.com>
To: "Bryan Kearney" <bkearney at redhat.com>
Cc: katello-devel at redhat.com
Sent: Tuesday, October 23, 2012 4:35:52 PM
Subject: Re: [katello-devel] Merging and Pulling with no wait



----- Original Message -----
> From: "Bryan Kearney" <bkearney at redhat.com>
> To: katello-devel at redhat.com
> Sent: Tuesday, October 23, 2012 4:21:17 PM
> Subject: Re: [katello-devel] Merging and Pulling with no wait
> 
> On 10/23/2012 03:52 PM, Jason Rist wrote:
> > On Tue 23 Oct 2012 12:12:27 PM MDT, Bryan Kearney wrote:
> >> On 10/23/2012 11:24 AM, Jason Rist wrote:
> >>> Hi Everyone - I started a discussion on IRC because I can't just
> >>> walk
> >>> over to people's cubes and question things, but I thought an
> >>> email would
> >>> be best to expand the conversation.
> >>>
> >>> This morning I was going through my emails and I saw two emails
> >>> for pull
> >>> request 898. ( https://github.com/Katello/katello/pull/898 ).
> >>>  One for
> >>> the pull request and 12 minutes later an ACK and merge.  Unless
> >>> I'm
> >>> checking my email all the time, I would have missed that.  And I
> >>> did
> >>> miss it.
> >>>
> >>> Now I'm picking on one pull request but the fact of the matter is
> >>> that
> >>> this has happened a bunch lately.
> >>>
> >>> The whole point of the pull request process is to make sure we're
> >>> getting a chance to prevent errors like the one that pull request
> >>> #903
> >>> was made to fix.
> >>>
> >>> Eric pointed out that there isn't a specified wait time but can
> >>> we
> >>> please take into consideration that 12 minutes isn't enough for a
> >>> pull
> >>> request to be fully discussed?
> >>
> >> What is enought time? Keep in mind that for +5 hours of each day,
> >> the
> >> other half of the team is not there.
> >> -- bk
> >>
> >
> >
> > Then doesn't it have to be at least +5 hours?
> >
> > -J
> 
> So, here is my take. Feel free to jump in and disagree. I think the
> goal
> of reviews and acks is to get a second set of eyes on the code to
> improve quality. There is an secondary benefit of educating /
> including
> other team members as well. However, I dont believe that is the
> primary
> goal. I think watching the commit logs is how that needs to be done.
> If
> we do not, then we have to go to a 24 hour delay on commits to ensure
> folks from all time zones can participate.
> 
> -- bk
> 
> _______________________________________________
> katello-devel mailing list
> katello-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/katello-devel
> 

I am against a mandated waiting period. Rather, I think, I would like to see a continuation of what I see now: Team members getting to know what other team members strengths and interests are.

For example, I am very interested in manifest and subscription areas of the code and try to keep my eye open for pull-requests that are in this area. Some, knowing my interest, include me directly in the pull-request comment or reach out on IRC.

Similarly, if I'm going to do something with javascript or css I will seek out @jrist or @ehelms when I'm in that area. For katello-configure it's @lzap. For CDN stuff it's @inecas. etc.

This is not to say they "own" these sections but rather that I trust their opinion and respect their interest. Maybe this is misplaced thinking but I don't think so.

It is my belief that with some purposeful forethought we can be more inclusive in our pull-request reviews. Again, though, I don't think making it mandatory is necessary. If you have an interest in areas of the code, sharing that with the rest of us is perfectly reasonable.

On the secondary issue of code stability, we are certainly iterating towards something better than what is there now. With @lzap and @ehelms efforts for automated testing per pull-request, plus @jweiss, @omaciel, and QE crew ramping up jenkins runs, and finally speed improvements to spec tests, I think stability and accountability has already increased.

Overall I really like working within our current structure and with our team. I've worked at a lot of companies (feel free to verify on linkedin ;) and I can say this product team is outstanding.

_______________________________________________
katello-devel mailing list
katello-devel at redhat.com
https://www.redhat.com/mailman/listinfo/katello-devel