Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update OWNERS file #9105

Merged
merged 1 commit into from
May 22, 2020
Merged

Conversation

johngmyers
Copy link
Member

@johngmyers johngmyers commented May 9, 2020

I ran some stats: looking at pull requests with updated:2019-05-01..2020-05-01 I counted the number of PRs that each of the existing approvers approved (either by commenting /approve or by authoring the PR) and the number of PRs that each person left a /lgtm comment on.

approvers:
   407 rifelpet
   301 justinsb
    86 mikesplain
    33 zetaab
    22 geojaz
    18 rdrgmnzs
    12 KashifSaadat
    10 granular-ryanbonham
     3 chrisz100
reviewers:
   334 rifelpet
   167 justinsb
   117 hakman
    70 johngmyers
    59 mikesplain
    23 joshbranham
    19 zetaab
    14 KashifSaadat
    14 rdrgmnzs
    13 gjtempleton
    13 olemarkus
    12 geojaz
     6 tanjunchen
     3 chrisz100
     3 granular-ryanbonham
     3 hwdef
     2 gambol99
     1 idealhack
     1 prksu

Based on that, I propose moving some approvers to emeritus status.

The remaining approvers I added to the reviewers list, excepting @justinsb and @rifelpet. I'd like to reserve @justinsb's time for things that require his expertise and don't think we need to ask more of @rifelpet.

I then propose adding myself and @hakman to the reviewers list and removing @robinpercy.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 9, 2020
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 9, 2020
@mikesplain
Copy link
Contributor

I think I'm on board with this, in principal getting more reviewers makes sense and involving approvers makes sense as well. K/k takes this approach and most approvers are reviewers, given the changes to blunderbuss.

That said, I'm a bigger fan of involving newer contributors and motivating them to get involved. Since most of kops reviewers/approvers aren't compensated for their efforts, I'm not sure it's a 1 to 1 comparison and I do think we should be cautious since kops reviews are certainly different. We should probably consider creating a review guide as well.

I do think theres something to be said about stats for a year can be limiting; a number of the people being migrated to emeritus are still top contributors given their history in the project. I'm not opposed to migrating some of them, given they seem to have moved on, just simply bringing it up out of caution. Since the emeritus term implies they may be re-migrated if they decide to get involved again, I'm not overly concerned, just figured I would bring it up.

Anyway I think I support this and appreciate you gathering both data and proposing a change @johngmyers. Given the breath of changes, I suggest we get at least a majority of approvers on this. Marking hold for that purpose and holding my lgtm until others chime in. Thanks!

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 10, 2020
@johngmyers
Copy link
Member Author

Tagging those affected by the current proposal and not previously tagged:

@olemarkus
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2020
@chrisz100
Copy link
Contributor

Given my current schedule:
/lgtm

Hope to be back rather soon and keep it up to all the new and current ones :-)

@rdrgmnzs
Copy link
Contributor

Landing as per unanimous decision during Kops office hours 05/22.

Thanks @johngmyers !

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2020
@chrisz100
Copy link
Contributor

/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 22, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrisz100, johngmyers, rdrgmnzs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 87ce5ee into kubernetes:master May 22, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone May 22, 2020
@johngmyers johngmyers deleted the update-owners branch May 22, 2020 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants