[katello-devel] Merging and Pulling with no wait

[Jason Rist]

On 10/23/2012 02:35 PM, Tom McKay wrote:
> 
> 
> ----- 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
>>
> 
> 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.
> 

Tom - you know, I totally agree with you.  I hadn't really thought of
the Ozzak/Travis stuff that @ehelms and @lzap had been working on, but
obviously it didn't catch Adam's pylint error, so it's not perfect.
Perhaps I jumped the gun sending my email out, but my main goal is to
make the product stable each and every check-in.  Education is great,
and I like that side of pull-requests, but it's not the primary reason
for my concern.  Plus, there is nothing stopping someone from viewing
the committed files via the closed pull request.

-J

-- 
Jason E. Rist
Senior Software Engineer
Systems Management and Cloud Enablement
Red Hat, Inc.
+1.919.754.4048
Freenode: jrist
github/identi.ca: knowncitizen