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

Add emeritus to owners guidelines #3627

Merged
merged 1 commit into from
May 6, 2019

Conversation

mrbobbytables
Copy link
Member

Adds "emeritus" to owners. This should be safely ignored by prow, and something we can build future tooling around. The intent is to provide a means for contributors who have potentially shifted roles or may be less active for a period of time to remove themselves from automated assignment, but still have an entry there if another opinion is needed.

The suggested change is just to mimic the owner spec under the emeritus keyword. This would make it easy for people to swap over, but less useful for future tooling. An alternative might be to mimic + do something like this:

emeritus:
  approvers:
  - name: bob
    date: 2019-04-22

Which would make it easier to build a report or dashboard on.

ref: kubernetes/kubernetes#76269 (comment)
ref: kubernetes/test-infra#10226 (comment)
ref: https://youtu.be/MS0AB0Sm6iA?list=PL69nYSiGNLP2x_48wbOPO0vXQgNTm_xxr&t=578

/cc @dims @nikhita @cblecker @spiffxp

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/contributor-guide Issues or PRs related to the contributor guide sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. labels Apr 22, 2019
@nikhita
Copy link
Member

nikhita commented Apr 22, 2019

Thanks Bob!!! 🎉

Which would make it easier to build a report or dashboard on.

I like the alternative version more because it can help us in auditing later.

@spiffxp
Copy link
Member

spiffxp commented Apr 22, 2019

I’m wary of the date field, blame or source control can be used for this. The sigs.yaml fields lack this

I’m also not clear why mirror the spec, I think the only emeritus we care about is approvers, no?

@mrbobbytables
Copy link
Member Author

I’m also not clear why mirror the spec, I think the only emeritus we care about is approvers, no?

That is really what we care about, yes -- My general thought there was more to play to the social aspect of it. People generally don't want to remove themselves from something once they're there. Being in an owners file is considered climbing the ladder, removing completely is a step down. Being added as emeritus still leaves them in there.

@cblecker
Copy link
Member

Thinking about it, I don't think we need emeritus reviewers. I also agree with @spiffxp that we don't need a date.

@mrbobbytables
Copy link
Member Author

👍 I'll update to just target approvers.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 28, 2019
Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more comments

contributors/guide/owners.md Outdated Show resolved Hide resolved
contributors/guide/owners.md Outdated Show resolved Hide resolved
@mrbobbytables mrbobbytables force-pushed the add-emeritus-owners branch from 1f77f9d to 7f79f18 Compare May 1, 2019 00:01
@nikhita
Copy link
Member

nikhita commented May 1, 2019

/cc @fejta
because you have opinions around this? (kubernetes/test-infra#10226 (comment))

@k8s-ci-robot k8s-ci-robot requested a review from fejta May 1, 2019 07:32
It is inevitable, but there are times when someone may shift focuses, change jobs or step away from
a specific area in the project for a time. These people may be domain experts over certain areas
of the codebase, but can no longer dedicate the time needed to handle the responsibilities of
reviewing and approving changes. They are encouraged to add themselves as an _"emeritus"_ approver
Copy link
Contributor

@fejta fejta May 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What more does this accomplish than what we can do already:

approvers:
# - fejta (emeritus)
- mrbobbytables
- nikhita
- spiffxp

Prior art: https://github.com/kubernetes/test-infra/blob/5406eea909597700cfe778d2d3fddf421f5284c1/prow/OWNERS#L8

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been used informally in a few places. Trying to put some language in here for wider use and encourage folks to self-remove; along with adding a potential avenue for future tooling.

reviewing and approving changes. They are encouraged to add themselves as an _"emeritus"_ approver
under the `emeritus_approvers` key.

Any GitHub username under the `emeritus_approvers` key will be ignored by prow for assignment.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One might interpret this to mean that prow will not use emeritus_approvers to assign people work while still granting emeritus_approvers the ability to approve code.

If so then this seems even better than being a regular approver -- all the same power without the hassle of helping others.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can call that out more explicitly. Something like:
"GitHub usernames listed under the emeritus_approvers key can no longer approve code (use the /approve command) and will be ignored by prow for assignment."

@fejta
Copy link
Contributor

fejta commented May 1, 2019

The specific request in kubernetes/test-infra#10226 (or at least my understanding of it) is to provide a way to give people the power to approve code without the responsibility of ever getting assigned any boring work. I think this is an unhealthy social pattern that we should avoid.

It is unclear whether this is attempting to make a glorified yaml comment (functionally equivalent to removing the approver). If so then my concerns are minimal -- although I would prefer a comment as it would be much harder to teach prow to empower a comment than the emeritus_approvers list.

In terms of the social aspect, I like the idea of encouraging people to remove themselves from the approver list. We could automate this by looking for people who have neither contributed nor reviewed code under a given path for a certain period of time and create a PR that adds them to the emeritus list (using whatever mechanism).

@mrbobbytables
Copy link
Member Author

It is unclear whether this is attempting to make a glorified yaml comment (functionally equivalent to removing the approver). If so then my concerns are minimal -- although I would prefer a comment as it would be much harder to teach prow to empower a comment than the emeritus_approvers list.

It is more inline with the yaml comment. Adding it as an key instead of a comment was more so future tooling could potentially be built around it.

In terms of the social aspect, I like the idea of encouraging people to remove themselves from the approver list. We could automate this by looking for people who have neither contributed nor reviewed code under a given path for a certain period of time and create a PR that adds them to the emeritus list (using whatever mechanism).

💯

@fejta
Copy link
Contributor

fejta commented May 1, 2019

Official comment that prow ignores SGTM

@mrbobbytables mrbobbytables force-pushed the add-emeritus-owners branch from 7f79f18 to 4bda78d Compare May 2, 2019 11:26
Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/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 4, 2019
@spiffxp
Copy link
Member

spiffxp commented May 6, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrbobbytables, spiffxp

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2019
@k8s-ci-robot k8s-ci-robot merged commit 4aedcd4 into kubernetes:master May 6, 2019
@mrbobbytables mrbobbytables deleted the add-emeritus-owners branch August 26, 2019 00:37
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. area/contributor-guide Issues or PRs related to the contributor guide 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. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. 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.

6 participants