[Pulp-dev] Pulp Code Owners

Tue Aug 14 14:27:16 UTC 2018

I know I said that code owners should be SMEs but I don’t think it should
it should be a strict requirement. The term SME and what constitutes an SME
are nebulous and hard to define. I think the lazy consensus model would be
a good first start as determining who qualifies for code ownership.

For having a bot automatically merge PRs, I can think of some examples of
where we wouldn't want to merge automatically. Maybe there’s a way to
handle them though? I wonder if this ought to be a separate conversation.

David

On Tue, Aug 14, 2018 at 9:56 AM Milan Kovacik <mkovacik at redhat.com> wrote:

>
>
> On Tue, Aug 14, 2018 at 3:47 PM, David Davis <daviddavis at redhat.com>
> wrote:
>
>> On Tue, Aug 14, 2018 at 9:35 AM Milan Kovacik <mkovacik at redhat.com>
>> wrote:
>>
>>>
>>>
>>> On Tue, Aug 14, 2018 at 1:26 PM, David Davis <daviddavis at redhat.com>
>>> wrote:
>>>
>>>> The relevant party could either be a subset of the commit bit owners
>>>> (e.g. task group) or a set of people who don’t have the commit bit (e.g.
>>>> QE). See the team examples from my original email.
>>>>
>>>
>>>  So what you mean is actually a trusted subset of commit bit owners,
>>> like the SMEs?
>>>
>>
>> These teams aren’t necessarily a subset of commit bit owners but yes
>> they’ll be subject matter experts (SMEs) for the code they own. Take QE for
>> example. They might not have the commit bit to the pulp repo but they are
>> still the SMEs for pulp_smash tests and thus they’ll probably be code
>> owners for the smash tests in pulp and pulp_file.
>>
>>
>>>  So we don't trust all commit bit owners equally when it comes to
>>> particular git (sub)tree?
>>>
>>  And we trust (by blocking the merge) on e.g QE approving a PR more than
>>> the commit bit owner that is outside of the subset?
>>>  Or is it rather about decoupling the code review duty from the commit
>>> bit ownership?
>>>
>>
>> To answer these questions, I don’t think it’s about trust. It’s about (as
>> you mention) decoupling merging code from code reviews. We want to make
>> sure the appropriate people get notified and have a chance to review the
>> PRs for which they are SMEs.
>>
>
> This has the same issue as the commit bit ownership, it's just more fine
> grained and bound to particular subtrees: how does one become an SME?
> What's the SME lifecycle?
>
>
>>
>>
>>>  Why do we need commit bit owners then?
>>>
>>
>> How else do we merge the code if no one has a commit bit?
>>
>
>   thru a bot for instance
>
>
>>
>>
>>>
>>> Cheers,
>>> milan
>>>
>>>
>>>>
>>>> Daniel, you are correct. The only caveat is that PRs can’t be merged if
>>>> they touch a file owned by a team and haven’t been approved by that team.
>>>>
>>>> David
>>>>
>>>>
>>>> On Tue, Aug 14, 2018 at 6:35 AM Milan Kovacik <mkovacik at redhat.com>
>>>> wrote:
>>>>
>>>>> +0 who's the relevant party if not the commit bit owner?
>>>>> +1 for commit bit owners receiving automatic notification to review
>>>>>
>>>>> --
>>>>> milan
>>>>>
>>>>> On Tue, Aug 14, 2018 at 12:56 AM, Daniel Alley <dalley at redhat.com>
>>>>> wrote:
>>>>>
>>>>>> +1. My understanding is that this will not directly limit who can
>>>>>> review or merge code, but should streamline the review process by notifying
>>>>>> relevant parties?
>>>>>>
>>>>>> On Mon, Aug 13, 2018 at 5:29 PM, David Davis <daviddavis at redhat.com>
>>>>>> wrote:
>>>>>>
>>>>>>> We have come up with initial proposal of how to use code owners
>>>>>>> feature in Pulp. Feedback on the initial proposal below is welcome. I will
>>>>>>> try to collect the feedback and open a PUP by the end of the week. Thanks!
>>>>>>>
>>>>>>>
>>>>>>> # Problem Statement
>>>>>>>
>>>>>>> For Pulp's review process, there are several areas we could improve:
>>>>>>>
>>>>>>> 1. It’s not always clear who should review files. Over time we have
>>>>>>> developed subject matter experts for different areas of the codebase, but
>>>>>>> these are not codified anywhere. It would be useful for us to define teams
>>>>>>> need to review projects using code owners.
>>>>>>>
>>>>>>> 2. PRs go unnoticed. Github has a request-review feature, but only
>>>>>>> members of the github organization can click 'request review' button. It
>>>>>>> would be great if when a PR is opened people automatically received PR
>>>>>>> review requests.
>>>>>>>
>>>>>>>
>>>>>>> # Team Examples
>>>>>>>
>>>>>>> Functional Tests - The QE Team should be code owners of functional
>>>>>>> tests that test core or core-maintained plugins
>>>>>>> The Tasking System  - bmbouter, daviddavis, and dalley are the SME
>>>>>>> in this area
>>>>>>>
>>>>>>>
>>>>>>> # Solution
>>>>>>>
>>>>>>> 1. Configure the code-owners feature of Github (
>>>>>>> https://blog.github.com/2017-07-06-introducing-code-owners/). It
>>>>>>> will allow a team of 2 or more people to be notified and asked for review
>>>>>>> when a PR modifies a file within a certain area of the code.
>>>>>>>
>>>>>>> 2. Require code-owner review to merge. This is described in this
>>>>>>> section:
>>>>>>> https://blog.github.com/2017-07-06-introducing-code-owners/#an-extra-layer-of-code-security
>>>>>>>
>>>>>>>
>>>>>>> # Process
>>>>>>>
>>>>>>> The code owner role is not related to the commit bit. It's designed
>>>>>>> to get better reviews. Well reviewed work can be confidently merged by
>>>>>>> anyone with the commit bit.
>>>>>>>
>>>>>>> To make a change to code owners, open a PR with the changes, and
>>>>>>> call for a lazy consensus vote by mailing list. Similar to the PUP decision
>>>>>>> making process, voting must be open for 10 days, and the committers of the
>>>>>>> respective repository are voting.
>>>>>>>
>>>>>>> The code owners file itself should be owned by the core committers
>>>>>>> of the repository.
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Pulp-dev mailing list
>>>>>>> Pulp-dev at redhat.com
>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Pulp-dev mailing list
>>>>>> Pulp-dev at redhat.com
>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>>
>>>>>>
>>>>> _______________________________________________
>>>>> Pulp-dev mailing list
>>>>> Pulp-dev at redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>
>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20180814/570490d3/attachment.htm>