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

Define and implement entry/exit criteria for kubernetes-maintainers GitHub team #2332

Open
nikhita opened this issue Nov 18, 2020 · 31 comments
Labels
area/github-permissions Permissions requests and problems help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/documentation Categorizes issue or PR as related to documentation. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. sig/release Categorizes an issue or PR as relevant to SIG Release.
Milestone

Comments

@nikhita
Copy link
Member

nikhita commented Nov 18, 2020

The legacy kubernetes-maintainers GitHub team has write access to the k/k repo. This issue is to discuss pruning the team.

There has been previous discussion about removing the team in kubernetes/kubernetes#57667. Given that we don't have good automated solutions around editing issue/PR bodies, completely removing write access and deleting the team is not feasible today.

Having said that, the kubernetes-maintainers team hasn't had any significant updates over the last ~3 years (!!!). There are members in that team who are no longer active in the Kubernetes project and should not have write access to k/k anymore.

My suggestion - we could limit this to milestone-maintainers + all approvers in k/k.

If there is general consensus on this GitHub issue, I'll start a thread on the k-dev and leads mailing lists.

@nikhita
Copy link
Member Author

nikhita commented Nov 18, 2020

cc @kubernetes/owners
GitHub admin team

cc @dims @derekwaynecarr @johnbelamaric
sig-arch leads

cc @liggitt @sttts @deads2k @tpepper @justaugustus
who've had opinions about this team before (in kubernetes/kubernetes#57667 + #2330)

@mrbobbytables
Copy link
Member

+1. It's been needed for some time.

@dims
Copy link
Member

dims commented Nov 18, 2020

+1 to prune. isn't milestone-maintainers a larger group?

@idvoretskyi
Copy link
Member

+1

@liggitt
Copy link
Member

liggitt commented Nov 18, 2020

My suggestion - we could limit this to milestone-maintainers + all approvers in k/k.

Is this suggesting we remove current members of the kubernetes-maintainers list if either of the following are true:

  • they are not in milestone-maintainers
  • they are not an approver in k/k

If so, that seems very reasonable to me and moves in the desired direction of shrinking the list.

@nikhita
Copy link
Member Author

nikhita commented Nov 18, 2020

Is this suggesting we remove current members of the kubernetes-maintainers list if either of the following are true:

  • they are not in milestone-maintainers
  • they are not an approver in k/k

Yes 👍

@nikhita
Copy link
Member Author

nikhita commented Nov 18, 2020

To clarify - we won't bulk-add milestone-maintainers + k/k approvers if they aren't already in the kubernetes-maintainers GitHub team.

If someone in milestone-maintainers + k/k approvers wants to be on the team, they can create a PR to add themselves separately.

Want to keep the team as small as possible.

@dims
Copy link
Member

dims commented Nov 18, 2020

@nikhita @liggitt sounds like a good plan. Let's do this!

@justaugustus
Copy link
Member

One note

If someone in milestone-maintainers + k/k approvers wants to be on the team, they can create a PR to add themselves separately.

What would be the approval process here?
My one concern is that Release Team members (specifically Release Team shadows) are on the milestone-maintainers group and I'm not in favor of them getting write access to k/k without some process in place.

@nikhita
Copy link
Member Author

nikhita commented Nov 18, 2020

Thinking more about this, how about:

  • If an existing member of kubernetes-maintainers is not a k/k approver, we remove them from the team
  • We don't add any new members to the team

@justaugustus
Copy link
Member

* If an existing member of `kubernetes-maintainers` is not a k/k approver, we remove them from the team

I like this.

* We don't add any new members to the team

Hmmmmm, so I think there should be a route to write access.

@dims is not a root approver for k/k, but I do feel he's deserving of the access that was granted in #2330.

How can we cover those cases in future?

BenTheElder added a commit to BenTheElder/k8s-org that referenced this issue Dec 3, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 16, 2021
@justaugustus
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 16, 2021
@BenTheElder
Copy link
Member

BenTheElder commented Mar 2, 2021

NOTE: due to branch protection write isn't really write to the repo unless you are in the admin superset which is very small / restricted.

We should prune inactive members.

I disagree that we should refuse to add anyone new. Being able to manually correct PR state (PR body, labels) is quite useful, and we should be able to trust people with this (approvers or maybe SIG chairs / leads).

EDIT: write permission gates pretty much any mutable state of any sort (labels, titles, milestones, etc.), well beyond the repo contents. While branch protection turns around and restricts the part of write most people probably think about (pushing/merging to branches).

@justaugustus
Copy link
Member

What requirements do triagers have that could not be covered with the Triage role?

Triage: Recommended for contributors who need to proactively manage issues and pull requests without write access

ref: https://docs.github.com/en/github/setting-up-and-managing-organizations-and-teams/repository-permission-levels-for-an-organization

@spiffxp
Copy link
Member

spiffxp commented Mar 9, 2021

I'm fine trying the triage role but would prefer we ensure peribolos has support for that. Unclear to me how actively it's used elsewhere

@spiffxp
Copy link
Member

spiffxp commented Mar 9, 2021

https://kubernetes.slack.com/archives/C1L57L91V/p1614017710014100 based on k/enhancements experience with the triage role, I don't think it meets our needs here (inability to edit issue/pr descriptions)

@spiffxp
Copy link
Member

spiffxp commented Mar 10, 2021

We need to decide on policy to prune, and policy to add. Then implement. Summarizing what I've read above from @nikhita @liggitt @BenTheElder @justaugustus

Possible reasons to prune:

  • A: is not an approver somewhere in k/k
  • B: is not a member of milestone-maintainers
  • C: has not used superpowers in previous N months (superpowers: edit issue/title description, since this is something Triage cannot do)
  • D: all of the above

Would like to understand what the reduction would be based on each of these. My preference would be C. If only I knew how to measure it at a glance...

Requirements to be added:

  • A: !(A or B from above) aka "is an approver somewhere in k/k AND is a member of milestone-maintainers"
  • B: +1 from a root approver
  • C: all of the above
  • D: none of the above, nobody new gets added

My preference would be C

@spiffxp
Copy link
Member

spiffxp commented Mar 10, 2021

I don't think Triage as a role is really helpful for us:

  • Apply/dismiss labels - it can do this, so it can bypass OWNERS by directly adding the approved label
  • Edit and delete anyone's comments on commits, pull requests, and issues - it can't do this, so it wouldn't help correct release-note foibles

@spiffxp
Copy link
Member

spiffxp commented Mar 10, 2021

/retitle Define and implement entry/exit criteria for kubernetes-maintainers GitHub team

@k8s-ci-robot k8s-ci-robot changed the title Prune kubernetes-maintainers GitHub team Define and implement entry/exit criteria for kubernetes-maintainers GitHub team Mar 10, 2021
@spiffxp
Copy link
Member

spiffxp commented Mar 10, 2021

/help
We could really use someone's help in measuring the impact of reasons to prune: #2332 (comment)

A: parse all OWNERS files in kubernetes/kubernetes

  • I have done this in the past with find and yq in a pinch, or python with glob and ruamel.yaml when I needed to parse OWNERS_ALIASES too
  • Another maybe easier approach is write golang that uses k8s.io/test-infra/prow/repoowners

B: diff the two teams' membership

  • can use files in kubernetes/org, or github's api

C: this is the one I'm not sure about

  • does a github search query for involves: spiffxp pick up issues I have edited (but not commented on, reviewed, or been mentioned)? -> can implement script based on "what this member has done on issues/PRs updated in last N months"
  • is there a GitHub API way of identifying edits to an issue/PR?

D: intersection of the above

  • if all done in the same script/golang, whatever boolean logic suits you best
  • silly bash way of doing intersection: dump "who gets pruned" into a file for each of the above, then cat pruned-by-[ABC].txt | sort | uniq -c | grep '^[ ]*3

@k8s-ci-robot
Copy link
Contributor

@spiffxp:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help
We could really use someone's help in measuring the impact of reasons to prune: #2332 (comment)

A: parse all OWNERS files in kubernetes/kubernetes

  • I have done this in the past with find and yq in a pinch, or python with glob and ruamel.yaml when I needed to parse OWNERS_ALIASES too
  • Another maybe easier approach is write golang that uses k8s.io/test-infra/prow/repoowners

B: diff the two teams' membership

  • can use files in kubernetes/org, or github's api

C: this is the one I'm not sure about

  • does a github search query for involves: spiffxp pick up issues I have edited (but not commented on, reviewed, or been mentioned)? -> can implement script based on "what this member has done on issues/PRs updated in last N months"
  • is there a GitHub API way of identifying edits to an issue/PR?

D: intersection of the above

  • if all done in the same script/golang, whatever boolean logic suits you best
  • silly bash way of doing intersection: dump "who gets pruned" into a file for each of the above, then cat pruned-by-[ABC].txt | sort | uniq -c | grep '^[ ]*3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Mar 10, 2021
@spiffxp spiffxp added the area/github-permissions Permissions requests and problems label Mar 10, 2021
@spiffxp spiffxp added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/documentation Categorizes issue or PR as related to documentation. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. sig/release Categorizes an issue or PR as relevant to SIG Release. labels Mar 10, 2021
@spiffxp spiffxp added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Mar 10, 2021
@justaugustus
Copy link
Member

/help
We could really use someone's help in measuring the impact of reasons to prune: #2332 (comment)

@kubernetes/release-engineering -- Anyone interested in helping out here?

@nikhita nikhita added this to the v1.22 milestone Apr 6, 2021
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 5, 2021
@nikhita
Copy link
Member Author

nikhita commented Jul 5, 2021

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 5, 2021
@BenTheElder
Copy link
Member

I have also recently found it more productive to fix PR bodies that do not meet our template criteria after one attempt at telling the author to please do this than lengthy back and forth (especially with time zone latency this can become very drawn out). Things like the release-note "language" code block trip people up and are trivial for a maintainer with write to fix.

We have a lot of things built around expecting PR metadata to be just so and everything is built around having the source of truth be in PRs themselves. Otherwise I'd suggest redesigning the mechanisms. E.g. cherry picks rely on parent PR's note so it would be somewhat prohibitive to move it to say scanning all comments on the PR.

In addition to brach protection we also monitor pushes and alert in other repos, we should be doing this here.

@spiffxp
Copy link
Member

spiffxp commented Aug 17, 2021

/milestone v1.23

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.22, v1.23 Aug 17, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 15, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 15, 2021
@justaugustus
Copy link
Member

/remove-lifecycle rotten
/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Dec 21, 2021
@BenTheElder
Copy link
Member

BenTheElder commented May 19, 2022

I have also recently found it more productive to fix PR bodies that do not meet our template criteria after one attempt at telling the author to please do this than lengthy back and forth (especially with time zone latency this can become very drawn out). Things like the release-note "language" code block trip people up and are trivial for a maintainer with write to fix.

TIL https://prow.k8s.io/command-help /release-note-edit (which ... can't be linked to ... that's broken ...)

It looks ... awkward to use (multi-line??) but ... one possible option for that aspect ... if we make people aware of it. I'm also not sure if this works in the face of mangling the "language" as mentioned previously, I think it just substitutes the contents of an already correctly formatted block.

Still, a somewhat promising thought.

In addition to brach protection we also monitor pushes and alert in other repos, we should be doing this here.

we should still really have this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/github-permissions Permissions requests and problems help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/documentation Categorizes issue or PR as related to documentation. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. sig/release Categorizes an issue or PR as relevant to SIG Release.
Projects
None yet
Development

No branches or pull requests