SIG Node contributor ladder review guidance #6725
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
approved
|
/cc @Random-Liu @dchen1107 @yujuhong @sjenning @mrunalp @klueska @ehashman @endocrimes @SergeyKanzhelev Emeritus perspectives appreciated across key subsystems. |
k8s-ci-robot
requested review from
ehashman,
endocrimes,
klueska,
mrunalp,
Random-Liu,
sjenning and
yujuhong
derekwaynecarr
force-pushed
the
sig-node-contributor-ladder
branch
from
c687e80
to
e16a5c0
Compare
|
ensuring time for additional feedback. /hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
/lgtm I reviewed and approved the original doc a while back. Waiting for others' feedback. |
|
/lgtm This is great! |
k8s-ci-robot
added
the
lgtm
|
Same as @dchen1107 I reviewed and approved the original doc a while back and see that my comments on that doc have been addressed in this PR. /lgtm |
| We anticipate providing opportunities for emerging contributors to make these contributions where they express their desire for long term maintenance. This is facilitated during release planning when pairing approvers with contributors for new feature work and/or e2e health with more testing focused specific contributions as we maintain CI. Helping maintain our CI and test suite is a great way of demonstrating competency! | ||
|
|
||
| ### Reviewer | ||
| Reviewer status is an indication that the person is committed to the SIG activities, technically sound, has accumulated enough context, and is overall trustworthy to help inform approvers and aid contributors. The requirements listed in the [membership document](../community-membership.md#reviewer) highlight this as well. |
There was a problem hiding this comment.
Maybe explicitly comment that you don't have to be a reviewer to add a review / comment on a PR.
Under what circumstances are informal reviews helpful?
There was a problem hiding this comment.
Added following:
Reviewer status is an indication that the person is committed to the SIG activities, technically sound, has accumulated enough context, and is overall trustworthy to help inform approvers and aid contributors by applying an lgtm. Anyone is welcome to review a PR with a comment or feedback even if they do not have rights to apply an lgtm.
|
|
||
| ## New Contributors | ||
|
|
||
| SIG Node welcomes new contributors to the community, help is always desired. Not all contributors are able to provide sustained contribution, but each contribution is always welcome. This document intends to capture a leadership path for contributors that intend to provide sustain contribution to the SIG and its associated code base by taking on reviewer and approver responsibilities at various levels. |
There was a problem hiding this comment.
Might be worth providing a link to SIG Node subprojects and capturing some workstreams here like E2E testing, kubelet runtime, storage, resource management, pleg and networking to name a few. Given that Kubelet code base is large, new contributors could potentially benefit by focussing on a smaller set of sub projects and developing a level of expertise on subprojects as a step towards getting a grasp of Kubelet's code base in the long run.
There was a problem hiding this comment.
great suggestion, added a link.
| The following is the breakdown of requirements listed in the membership document broken down to these categories: | ||
|
|
||
| - Committed - proof of sustained contributions | ||
| - member for at least 3 months | ||
| - Technically sound | ||
| - Primary reviewer for at least 5 PRs to the codebase | ||
| - Reviewed or merged at least 20 substantial PRs to the codebase | ||
| - Has enough context | ||
| - Knowledgeable about the codebase | ||
| - Trustworthy - established trust with the community | ||
| - Sponsored by a subproject approver | ||
| - With no objections from other approvers |
There was a problem hiding this comment.
I think it would be good to assign a mentorship / shadowing program for folks who want to become a reviewer if wanted. This will lower the bar for new folks to get into the SIG and the code as well.
There was a problem hiding this comment.
We had one of these briefly in 2020. I'd love us to do the same again, but I'm not sure how effectively we could staff it :(
|
|
||
| SIG Node approvers have a lot of responsibilities and it is a very demanding role as the project keeps expanding in capability. It is expected that the SIG Node approver keeps the codebase quality high by giving feedback, thoroughly reviewing code, and giving recommendations to SIG members and reviewers. SIG Node approvers are essentially gatekeepers to keep the code base at high quality. SIG Node maintains a rigidly high bar for becoming a SIG Node approver by developing trust in a community and demonstrating expertise with a bias towards initial code contributions over reviewing PRs. | ||
|
|
||
| SIG Node approver role is not based on a volume of ongoing work, there is no absolute quota. Approver role is based on accumulated trust and knowledge and it is not expected to require 100% full time commitment from any individual member. The expectation is that an active (non emeritus) approver is responsive to a direct ask to review a complex PR or KEP in their area of expertise when solicited. It is encouraged for a top-level approver to opt out of a review where another approver is more appropriate. |
There was a problem hiding this comment.
Do we want to handle the emeritus approvers handling here as well? At which point do we consider an approver as emeritus? I guess the easiest way would be to defer it to the global community cleanup like kubernetes/org#3094
There was a problem hiding this comment.
@saschagrunert i prefer to keep the current PR on how to grow scope.
|
|
||
| ## Goal | ||
|
|
||
| This document intends to define a standard for supporting community members to grow responsibility in the SIG. The SIG has a goal to scale individual participation using transparent criteria. It is aspirational, and should be re-evaluated to ensure we can meet the needs of the project with the resources available to participate. It is expected that it provides the SIG a log for future evolution of decision making criteria. |
There was a problem hiding this comment.
| This document intends to define a standard for supporting community members to grow responsibility in the SIG. The SIG has a goal to scale individual participation using transparent criteria. It is aspirational, and should be re-evaluated to ensure we can meet the needs of the project with the resources available to participate. It is expected that it provides the SIG a log for future evolution of decision making criteria. | |
| This document intends to define a standard for supporting community members to grow responsibility in the SIG. The SIG has a goal to scale individual participation using transparent criteria. It is aspirational, and should be re-evaluated periodically to ensure we can meet the needs of the project with the resources available to participate. It is expected that it provides the SIG a log for future evolution of decision making criteria. |
|
|
||
| ## SIG Node reviewers and approvers | ||
|
|
||
| SIG Node maintains Kubernetes components that are running large amounts of workloads across clouds, and standalone deployments. Changes in these components have a drastic effect on workload availability. This sets a high bar for SIG Node contributions and puts a lot of emphasis on reliability and security of these changes. Practices and policies of earlier stages of Kubernetes need to be adjusted to the current maturity level of Kubernetes. |
There was a problem hiding this comment.
I'm a little unclear on what the last sentence "Practices and policies..." is saying here in the context of this doc?
There was a problem hiding this comment.
earlier in the project, if you contributed a handful of PRs to a particular sub-project, it was very likely you had a broad context over that component because the component was much smaller in scope.
|
|
||
| As a high level guidance, SIG Node reviewers and approvers must have a bias to say “no” in reviews and design discussions, and an initial bias towards coding over reviews to demonstrate a baseline for konwledge of code base. This high level guidance is dictated by the maturity stage of Kubernetes. It is better to have inefficiencies or known bugs than break existing workloads. It is also important to keep the codebase healthy and maintainable. | ||
|
|
||
| We anticipate providing opportunities for emerging contributors to make these contributions where they express their desire for long term maintenance. This is facilitated during release planning when pairing approvers with contributors for new feature work and/or e2e health with more testing focused specific contributions as we maintain CI. Helping maintain our CI and test suite is a great way of demonstrating competency! |
There was a problem hiding this comment.
| We anticipate providing opportunities for emerging contributors to make these contributions where they express their desire for long term maintenance. This is facilitated during release planning when pairing approvers with contributors for new feature work and/or e2e health with more testing focused specific contributions as we maintain CI. Helping maintain our CI and test suite is a great way of demonstrating competency! | |
| We anticipate providing opportunities for emerging contributors to make these contributions where they express their desire for long term maintenance. This is facilitated during release planning when pairing approvers with contributors for new feature work and/or specific contributions focused on testing and e2e health as we maintain our CI. Helping maintain our CI and test suite is a great way of demonstrating competency! |
| We anticipate providing opportunities for emerging contributors to make these contributions where they express their desire for long term maintenance. This is facilitated during release planning when pairing approvers with contributors for new feature work and/or e2e health with more testing focused specific contributions as we maintain CI. Helping maintain our CI and test suite is a great way of demonstrating competency! | ||
|
|
||
| ### Reviewer | ||
| Reviewer status is an indication that the person is committed to the SIG activities, technically sound, has accumulated enough context, and is overall trustworthy to help inform approvers and aid contributors. The requirements listed in the [membership document](../community-membership.md#reviewer) highlight this as well. |
There was a problem hiding this comment.
| Reviewer status is an indication that the person is committed to the SIG activities, technically sound, has accumulated enough context, and is overall trustworthy to help inform approvers and aid contributors. The requirements listed in the [membership document](../community-membership.md#reviewer) highlight this as well. | |
| Reviewer status is an indication that the person is committed to the SIG activities, demonstrates technical depth, has accumulated enough context, and is overall trustworthy to help inform approvers and aid contributors. The requirements listed in the [membership document](../community-membership.md#reviewer) highlight this as well. |
|
|
||
| There are different ways to earn trust: | ||
|
|
||
| **Deep expertise across multiplate sub-systems** |
There was a problem hiding this comment.
| **Deep expertise across multiplate sub-systems** | |
| **Deep expertise across multiple sub-systems** |
| - Making the most of reviews in a single area is a good indication that a subdirectory reviewer role may be preferred or top-level node reviewer. Finding the right balance helps balance the nominees future workload. | ||
| - A primary reviewer should drive the review of the PR without significant input / guidance from the approver or other reviewers. | ||
|
|
||
| **Has enough context** |
There was a problem hiding this comment.
As a reader, it feels like having context blends in with the technical depth/being "technically sound" above?
There was a problem hiding this comment.
Agree, combined the sections.
| - “alpha” - design proposal and discussions | ||
| - “beta” - initial customer feedback collection | ||
| - “GA/deprecation” - stabilizing feature, following PRRs, or managing deprecation. | ||
| - Demonstrate ability to stage changes and pass PRRs keeping end users experience and Kubernetes reliability as top priorities. |
There was a problem hiding this comment.
| - Demonstrate ability to stage changes and pass PRRs keeping end users experience and Kubernetes reliability as top priorities. | |
| - Demonstrate ability to stage changes and pass PRRs keeping the end user experience and Kubernetes reliability as top priorities. |
| Actively triage issues and PRs, provide support to contributors to drive their PRs to completion. | ||
|
|
||
| **Be present** | ||
| Participate in SIG Node meetings by speaking about KEPs or improvements driven, or find some other way to prove the identity behind GitHub handle. |
There was a problem hiding this comment.
| Participate in SIG Node meetings by speaking about KEPs or improvements driven, or find some other way to prove the identity behind GitHub handle. | |
| Participate in SIG Node meetings by speaking about KEPs or improvements driven, or find some other way to engage SIG member outside of Github activities. |
Is this closer to what you're looking for? I presume it's about engagement and not verifying identity?
There was a problem hiding this comment.
@kikisdeliveryservice it is about knowing there is a real person behind the GitHub handle. there is some context for this request based on past requests for membership growth. as a community, we do want to ensure we know the person by some visible engagement in a meeting, meet-up, conference, call, etc.
There was a problem hiding this comment.
gotcha, thanks for the clarification!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, sjenning 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 |
derekwaynecarr
force-pushed
the
sig-node-contributor-ladder
branch
from
e16a5c0
to
7be03c7
Compare
k8s-ci-robot
removed
the
lgtm
|
I pushed updates per the feedback. Thanks everyone. Removing the hold per feedback. /hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Which issue(s) this PR fixes:
Fixes #6612
As noted in the document, the SIG had a desire to establish transparent criteria for how it evaluates requests for reviewer and approver rights. This document has been reviewed by existing SIG Node and CI sub-project leads with input suggested over past many months.
To highlight a key desire for the SIG, top-level approval rights in the
kubeletsub-project have a strong preference to acquiring prior approval rights in an intermediate sub-folder or sub-system or adjacent area like e2e testing or CRI (client or server) side. Sincekubelethas intersection with both storage and networking code paths given our current project approach, making this explicit in our guidance felt appropriate given our project maturity.Our goals are to maintain a high bar for security and reliability going forward, suggestions welcome!