Finish OWNERS implementation #1389
Comments
|
Copying from #474: Previous workflow:
Proposed workflow:
It's pretty much the same, except that it bypasses github ACLs. We've already taken direct merge powers away from most people in kubernetes-maintainers. On approvers vs. reviewers: We're bottlenecked on review bandwidth. We need to grow the reviewer pool, but we also need to move to multi-phase PR reviews in order to take load off of bottleneck people. A reviewer just has to be someone who could provide constructive feedback on a PR review -- someone who understands our project's coding conventions, ideally the code of that component, etc. We should be more liberal with inclusion in the reviewers list. An approver is someone with the authority to determine whether a PR should be merged or not. We'll want a progression by which we can grow contributors into reviewers and reviewers into approvers. A PR might have a single primary reviewer but require multiple approvers (e.g., an API approver and a Kubelet approver, for a Node API change), or multiple reviewers (e.g., people familiar with different parts of the repo) and a single approver (e.g., me). As an example, I want to be removed from the PR auto-assigner, but would like to retain my approval powers -- I should be an escalation point, not on the front lines of reviews. We should find a home for this: |
|
@hongchaodeng @xiang90 Do you have any more questions about this? |
|
@bgrant0607 |
|
Here's a follow-up to the proposed workflow:
The findowner program will generate owners for each package. Do you want me to assign according to the new owner list in the implementation?
PR (#1428) will enable munger to look for 'LGTM' comment from reviewer and apply an 'lgtm' label. Q: What's the policy for multiple reviewers?
PR (#1429) will enable munger to look for approver's "I approve" comment and apply "approved" label. Q1: What's an approver? How can I know if someone is an approver? |
|
@hongchaodeng FYI, I filter all github notifications that don't mention "bgrant0607", so I didn't notice your update. I'll try to respond later today. |
|
The bot implementation will be in contrib. The updates of OWNERS needs to happen in the kubernetes repo. These can occur relatively independently. As a starting point, current reviewers should be moved to approvers, and we could use Policy on multiple reviewers: Github now supports multiple assignees. For PRs touching multiple directories, rather than assigning reviewers from the nearest common ancestor in the source tree, for scalability, I think we should assign reviewers from the leaves, perhaps randomly selecting 2 from each directory and then choosing the least loaded review-wise (The Power of 2 Choices). In the case of multiple assignees, it would be up to them to coordinate the review of the whole thing, though eventually it would be desirable for the bot to confirm LGTM coverage of all directories touched before applying the lgtm label. I'm open to other policies, and we can also refine it over time. For approvers, I think we should assign/notify an approver from the nearest common ancestor but, again, transitive coverage of directories touched should gate application of the |
|
I know most of the talk about repo-splitting was in the main repo, but... Any objections to breaking off a new repo for "autobots" and putting all On Mon, Jul 25, 2016 at 9:40 PM, Brian Grant [email protected]
|
|
@thockin I agree, but could we please not discuss it on this issue? |
|
ACK. Is there an issue for this topic or should I open one? On Mon, Jul 25, 2016 at 11:07 PM, Brian Grant [email protected]
|
|
There is actually a doc: |
|
@kubernetes/contributor-experience My implementation plan follows the comment:
@bgrant0607 One question. What does it mean by "transitive coverage of directories touched should gate application of the approved label." |
|
@hongchaodeng A user listed as an approver in an OWNERS file applies to files in that directory and in all subdirectories of that directory, so someone in the root OWNERS file could approve all PRs. |
|
Interesting to have this! Tracking! |
|
If you need more inspiration, here's a similar product: FWIW, we have looked at pullapprove, but the functionality is a subset of what we need for our PR workflow in general, and the legal process to get it approved could well drag out for a really long time. |
|
@hongchaodeng If we are regenerating updated owners for the main repository, we may be able to leverage the code at https://github.com/foxish/findowner/tree/foxish-edits, which I used to generate owners for contrib. It has a few modifications to the original coreos/findowner code, and some of those changes may be reusable. Some things that I discovered when generating OWNERs on contrib and docs were that looking at the entire commit history (as opposed to the last 6 months) works better, and we that we nay need some decay factor as we go over the commits and compute each user's rank. |
|
@hongchaodeng can we get a status update on this? Do you need any help or input on the implementation? |
|
For the code it's done in https://github.com/coreos/findowner |
|
Well after long last all the PRs to the various cc: @philips |
|
I'll send an email out to kubernetes-dev shortly that details the approvers process and the timeline. In short, we'll start running it on the main repo next week and we'll use contribx mailing list for feedback. |
Automatic merge from submit-queue implement approval handler ref: kubernetes-retired#1389 What? This PR adds ApprovalHandler that will try to add "approved" label once all files of change has been approved by approvers. ``` // The algorithm goes as: // - Initially, we set up approverMap // - Go through all comments after latest commit. If any approver said "/approve", add him to approverMap. // - For each file, we see if any approver of this file is in approverMap. // - An approver of a file is defined as: // - It's known that each dir has a list of approvers. (This might not hold true. For usability, current situation is enough.) // - Approver of a dir is also the approver of child dirs. // - We look at only top 3 dir level's approvers . // - Iff all files has been approved, the bot will add "approved" label. ```
…rocess Automatic merge from submit-queue Approval process Implements kubernetes-retired#1389 Please do not remove the `do-not-merge` label, unless you know what you're doing.
|
Thanks for all of the hard work @grodrigues3 @apelisse and @calebamiles. For those that missed it the kubernetes-dev thread is here: https://groups.google.com/forum/#!topic/kubernetes-dev/aCuaL0DZZvI |
|
Thanks @philips. it might be time to close this issue :-) |
|
We are fixing issues and addressing feedback here: kubernetes/test-infra#1678 |
|
Closing this issue now. |
Automatic merge from submit-queue Curating Owners: pkg/cloudprovider cc @Runseb @justinsb @kerneltime @mikedanese @svanharmelen @anguslees @brendandburns @abrarshivani @imkin @luomiao @colemickens @ngtuna @dagnello @abithap In an effort to expand the existing pool of reviewers and establish a two-tiered review process (first someone lgtms and then someone experienced in the project approves), we are adding new reviewers to existing owners files. If You Care About the Process: ------------------------------ We did this by algorithmically figuring out who’s contributed code to the project and in what directories. Unfortunately, that doesn’t work well: people that have made mechanical code changes (e.g change the copyright header across all directories) end up as reviewers in lots of places. Instead of using pure commit data, we generated an excessively large list of reviewers and pruned based on all time commit data, recent commit data and review data (number of PRs commented on). At this point we have a decent list of reviewers, but it needs one last pass for fine tuning. Also, see kubernetes-retired/contrib#1389. TLDR: ----- As an owner of a sig/directory and a leader of the project, here’s what we need from you: 1. Use PR kubernetes/kubernetes#35715 as an example. 2. The pull-request is made editable, please edit the `OWNERS` file to remove the names of people that shouldn't be reviewing code in the future in the **reviewers** section. You probably do NOT need to modify the **approvers** section. Names asre sorted by relevance, using some secret statistics. 3. Notify me if you want some OWNERS file to be removed. Being an approver or reviewer of a parent directory makes you a reviewer/approver of the subdirectories too, so not all OWNERS files may be necessary. 4. Please use ALIAS if you want to use the same list of people over and over again (don't hesitate to ask me for help, or use the pull-request above as an example)
Automatic merge from submit-queue Approval process Implements kubernetes-retired/contrib#1389 Please do not remove the `do-not-merge` label, unless you know what you're doing.
…proval-process Automatic merge from submit-queue Approval process Implements kubernetes-retired/contrib#1389 Please do not remove the `do-not-merge` label, unless you know what you're doing.
…proval-process Automatic merge from submit-queue Approval process Implements kubernetes-retired/contrib#1389 Please do not remove the `do-not-merge` label, unless you know what you're doing.
Automatic merge from submit-queue Curating Owners: pkg/cloudprovider cc @Runseb @justinsb @kerneltime @mikedanese @svanharmelen @anguslees @brendandburns @abrarshivani @imkin @luomiao @colemickens @ngtuna @dagnello @abithap In an effort to expand the existing pool of reviewers and establish a two-tiered review process (first someone lgtms and then someone experienced in the project approves), we are adding new reviewers to existing owners files. If You Care About the Process: ------------------------------ We did this by algorithmically figuring out who’s contributed code to the project and in what directories. Unfortunately, that doesn’t work well: people that have made mechanical code changes (e.g change the copyright header across all directories) end up as reviewers in lots of places. Instead of using pure commit data, we generated an excessively large list of reviewers and pruned based on all time commit data, recent commit data and review data (number of PRs commented on). At this point we have a decent list of reviewers, but it needs one last pass for fine tuning. Also, see kubernetes-retired/contrib#1389. TLDR: ----- As an owner of a sig/directory and a leader of the project, here’s what we need from you: 1. Use PR kubernetes/kubernetes#35715 as an example. 2. The pull-request is made editable, please edit the `OWNERS` file to remove the names of people that shouldn't be reviewing code in the future in the **reviewers** section. You probably do NOT need to modify the **approvers** section. Names asre sorted by relevance, using some secret statistics. 3. Notify me if you want some OWNERS file to be removed. Being an approver or reviewer of a parent directory makes you a reviewer/approver of the subdirectories too, so not all OWNERS files may be necessary. 4. Please use ALIAS if you want to use the same list of people over and over again (don't hesitate to ask me for help, or use the pull-request above as an example)
Automatic merge from submit-queue Curating Owners: pkg/volume cc @jsafrane @spothanis @agonzalezro @justinsb @johscheuer @simonswine @NELCY @pmorie @quofelix @sdminonne @thockin @saad-ali @rootfs In an effort to expand the existing pool of reviewers and establish a two-tiered review process (first someone lgtms and then someone experienced in the project approves), we are adding new reviewers to existing owners files. If You Care About the Process: ------------------------------ We did this by algorithmically figuring out who’s contributed code to the project and in what directories. Unfortunately, that doesn’t work well: people that have made mechanical code changes (e.g change the copyright header across all directories) end up as reviewers in lots of places. Instead of using pure commit data, we generated an excessively large list of reviewers and pruned based on all time commit data, recent commit data and review data (number of PRs commented on). At this point we have a decent list of reviewers, but it needs one last pass for fine tuning. Also, see kubernetes-retired/contrib#1389. TLDR: ----- As an owner of a sig/directory and a leader of the project, here’s what we need from you: 1. Use PR kubernetes/kubernetes#35715 as an example. 2. The pull-request is made editable, please edit the `OWNERS` file to remove the names of people that shouldn't be reviewing code in the future in the **reviewers** section. You probably do NOT need to modify the **approvers** section. Names asre sorted by relevance, using some secret statistics. 3. Notify me if you want some OWNERS file to be removed. Being an approver or reviewer of a parent directory makes you a reviewer/approver of the subdirectories too, so not all OWNERS files may be necessary. 4. Please use ALIAS if you want to use the same list of people over and over again (don't hesitate to ask me for help, or use the pull-request above as an example)
Automatic merge from submit-queue Curating Owners: pkg/cloudprovider cc @Runseb @justinsb @kerneltime @mikedanese @svanharmelen @anguslees @brendandburns @abrarshivani @imkin @luomiao @colemickens @ngtuna @dagnello @abithap In an effort to expand the existing pool of reviewers and establish a two-tiered review process (first someone lgtms and then someone experienced in the project approves), we are adding new reviewers to existing owners files. If You Care About the Process: ------------------------------ We did this by algorithmically figuring out who’s contributed code to the project and in what directories. Unfortunately, that doesn’t work well: people that have made mechanical code changes (e.g change the copyright header across all directories) end up as reviewers in lots of places. Instead of using pure commit data, we generated an excessively large list of reviewers and pruned based on all time commit data, recent commit data and review data (number of PRs commented on). At this point we have a decent list of reviewers, but it needs one last pass for fine tuning. Also, see kubernetes-retired/contrib#1389. TLDR: ----- As an owner of a sig/directory and a leader of the project, here’s what we need from you: 1. Use PR kubernetes/kubernetes#35715 as an example. 2. The pull-request is made editable, please edit the `OWNERS` file to remove the names of people that shouldn't be reviewing code in the future in the **reviewers** section. You probably do NOT need to modify the **approvers** section. Names asre sorted by relevance, using some secret statistics. 3. Notify me if you want some OWNERS file to be removed. Being an approver or reviewer of a parent directory makes you a reviewer/approver of the subdirectories too, so not all OWNERS files may be necessary. 4. Please use ALIAS if you want to use the same list of people over and over again (don't hesitate to ask me for help, or use the pull-request above as an example)
Automatic merge from submit-queue Curating Owners: pkg/volume cc @jsafrane @spothanis @agonzalezro @justinsb @johscheuer @simonswine @NELCY @pmorie @quofelix @sdminonne @thockin @saad-ali @rootfs In an effort to expand the existing pool of reviewers and establish a two-tiered review process (first someone lgtms and then someone experienced in the project approves), we are adding new reviewers to existing owners files. If You Care About the Process: ------------------------------ We did this by algorithmically figuring out who’s contributed code to the project and in what directories. Unfortunately, that doesn’t work well: people that have made mechanical code changes (e.g change the copyright header across all directories) end up as reviewers in lots of places. Instead of using pure commit data, we generated an excessively large list of reviewers and pruned based on all time commit data, recent commit data and review data (number of PRs commented on). At this point we have a decent list of reviewers, but it needs one last pass for fine tuning. Also, see kubernetes-retired/contrib#1389. TLDR: ----- As an owner of a sig/directory and a leader of the project, here’s what we need from you: 1. Use PR kubernetes/kubernetes#35715 as an example. 2. The pull-request is made editable, please edit the `OWNERS` file to remove the names of people that shouldn't be reviewing code in the future in the **reviewers** section. You probably do NOT need to modify the **approvers** section. Names asre sorted by relevance, using some secret statistics. 3. Notify me if you want some OWNERS file to be removed. Being an approver or reviewer of a parent directory makes you a reviewer/approver of the subdirectories too, so not all OWNERS files may be necessary. 4. Please use ALIAS if you want to use the same list of people over and over again (don't hesitate to ask me for help, or use the pull-request above as an example)
Automatic merge from submit-queue Curating Owners: pkg/cloudprovider cc @Runseb @justinsb @kerneltime @mikedanese @svanharmelen @anguslees @brendandburns @abrarshivani @imkin @luomiao @colemickens @ngtuna @dagnello @abithap In an effort to expand the existing pool of reviewers and establish a two-tiered review process (first someone lgtms and then someone experienced in the project approves), we are adding new reviewers to existing owners files. If You Care About the Process: ------------------------------ We did this by algorithmically figuring out who’s contributed code to the project and in what directories. Unfortunately, that doesn’t work well: people that have made mechanical code changes (e.g change the copyright header across all directories) end up as reviewers in lots of places. Instead of using pure commit data, we generated an excessively large list of reviewers and pruned based on all time commit data, recent commit data and review data (number of PRs commented on). At this point we have a decent list of reviewers, but it needs one last pass for fine tuning. Also, see kubernetes-retired/contrib#1389. TLDR: ----- As an owner of a sig/directory and a leader of the project, here’s what we need from you: 1. Use PR kubernetes/kubernetes#35715 as an example. 2. The pull-request is made editable, please edit the `OWNERS` file to remove the names of people that shouldn't be reviewing code in the future in the **reviewers** section. You probably do NOT need to modify the **approvers** section. Names asre sorted by relevance, using some secret statistics. 3. Notify me if you want some OWNERS file to be removed. Being an approver or reviewer of a parent directory makes you a reviewer/approver of the subdirectories too, so not all OWNERS files may be necessary. 4. Please use ALIAS if you want to use the same list of people over and over again (don't hesitate to ask me for help, or use the pull-request above as an example)
Automatic merge from submit-queue Curating Owners: pkg/cloudprovider cc @Runseb @justinsb @kerneltime @mikedanese @svanharmelen @anguslees @brendandburns @abrarshivani @imkin @luomiao @colemickens @ngtuna @dagnello @abithap In an effort to expand the existing pool of reviewers and establish a two-tiered review process (first someone lgtms and then someone experienced in the project approves), we are adding new reviewers to existing owners files. If You Care About the Process: ------------------------------ We did this by algorithmically figuring out who’s contributed code to the project and in what directories. Unfortunately, that doesn’t work well: people that have made mechanical code changes (e.g change the copyright header across all directories) end up as reviewers in lots of places. Instead of using pure commit data, we generated an excessively large list of reviewers and pruned based on all time commit data, recent commit data and review data (number of PRs commented on). At this point we have a decent list of reviewers, but it needs one last pass for fine tuning. Also, see kubernetes-retired/contrib#1389. TLDR: ----- As an owner of a sig/directory and a leader of the project, here’s what we need from you: 1. Use PR kubernetes/kubernetes#35715 as an example. 2. The pull-request is made editable, please edit the `OWNERS` file to remove the names of people that shouldn't be reviewing code in the future in the **reviewers** section. You probably do NOT need to modify the **approvers** section. Names asre sorted by relevance, using some secret statistics. 3. Notify me if you want some OWNERS file to be removed. Being an approver or reviewer of a parent directory makes you a reviewer/approver of the subdirectories too, so not all OWNERS files may be necessary. 4. Please use ALIAS if you want to use the same list of people over and over again (don't hesitate to ask me for help, or use the pull-request above as an example)
Automatic merge from submit-queue Curating Owners: pkg/cloudprovider cc @Runseb @justinsb @kerneltime @mikedanese @svanharmelen @anguslees @brendandburns @abrarshivani @imkin @luomiao @colemickens @ngtuna @dagnello @abithap In an effort to expand the existing pool of reviewers and establish a two-tiered review process (first someone lgtms and then someone experienced in the project approves), we are adding new reviewers to existing owners files. If You Care About the Process: ------------------------------ We did this by algorithmically figuring out who’s contributed code to the project and in what directories. Unfortunately, that doesn’t work well: people that have made mechanical code changes (e.g change the copyright header across all directories) end up as reviewers in lots of places. Instead of using pure commit data, we generated an excessively large list of reviewers and pruned based on all time commit data, recent commit data and review data (number of PRs commented on). At this point we have a decent list of reviewers, but it needs one last pass for fine tuning. Also, see kubernetes-retired/contrib#1389. TLDR: ----- As an owner of a sig/directory and a leader of the project, here’s what we need from you: 1. Use PR kubernetes/kubernetes#35715 as an example. 2. The pull-request is made editable, please edit the `OWNERS` file to remove the names of people that shouldn't be reviewing code in the future in the **reviewers** section. You probably do NOT need to modify the **approvers** section. Names asre sorted by relevance, using some secret statistics. 3. Notify me if you want some OWNERS file to be removed. Being an approver or reviewer of a parent directory makes you a reviewer/approver of the subdirectories too, so not all OWNERS files may be necessary. 4. Please use ALIAS if you want to use the same list of people over and over again (don't hesitate to ask me for help, or use the pull-request above as an example)
Described here:
#474 (comment)
cc @eparis @davidopp @philips @mike-saparov @hongchaodeng @kubernetes/contributor-experience
@philips Please add your suggestions here.
The text was updated successfully, but these errors were encountered: