Skip to content

Commit f0d4b23

Browse files
authored
Add guidelines for node PR triage (#5436)
* Add guidelines for node PR triage * Address reviewer feedback * Address more of Sergey's feedback * Add 'kind' guide and more label links * Add note on authors' responsibilities and stale PRs
1 parent 4225574 commit f0d4b23

File tree

1 file changed

+215
-0
lines changed

1 file changed

+215
-0
lines changed

contributors/devel/sig-node/triage.md

Lines changed: 215 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,215 @@
1+
# SIG Node Triage Process
2+
3+
Currently, SIG Node is limiting its triage review to pull requests. In the
4+
near-future, we hope to expand to issues as well.
5+
6+
This is intended to guide developers through the SIG Node triage process. You
7+
will learn about the roles of authors, reviewers, and approvers, as well as
8+
what labels are required for each stage of the PR process.
9+
10+
We track most node PRs on the [SIG Node Triage project board]. All test and
11+
CI-related PRs are tracked on the [CI subproject board].
12+
13+
For help with the commands listed in the document below, review the [bot command documentation].
14+
15+
[SIG Node Triage project board]: https://github.com/orgs/kubernetes/projects/49
16+
[CI subproject board]: https://github.com/orgs/kubernetes/projects/43
17+
[bot command documentation]: https://go.k8s.io/bot-commands
18+
19+
## Triage
20+
21+
All new PRs added to the board should begin in the **Triage** column.
22+
23+
When a pull request is made against kubernetes/kubernetes, it will typically
24+
have the following [labels], applied by the Prow bot:
25+
26+
- needs-ok-to-test (not needed for project members)
27+
- needs-kind
28+
- needs-priority
29+
- needs-triage
30+
31+
In order to be moved out of this column, all of the above labels must be set.
32+
33+
[labels]: https://github.com/kubernetes/test-infra/blob/master/label_sync/labels.md
34+
35+
### needs-ok-to-test
36+
37+
Use your best judgment in determining whether a PR is OK to test. You don't
38+
have to do a full review: just make sure that the code does not appear to be
39+
actively malicious, and the PR appears to be doing something useful. Use the
40+
command `/ok-to-test`.
41+
42+
Only [Kubernetes org members] can add this [label][labels].
43+
44+
If the PR is trivial and doesn't provide much value, feel free to close it
45+
using the `/close` command and link the author to the [trivial edits policy].
46+
47+
You can use the following message:
48+
49+
/close
50+
51+
Thank you for your PR. It seems to only contain trivial edits. Please read our [trivial edits policy](https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#trivial-edits). We encourage you to take a look at confirmed issues and bugs or issues marked `help-wanted`.
52+
53+
[Kubernetes org members]: https://github.com/kubernetes/community/blob/master/community-membership.md#member
54+
[trivial edits policy]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#trivial-edits
55+
56+
### needs-kind
57+
58+
Most authors will already set this, as it's part of the PR template, but they
59+
may not set it correctly. A PR may have multiple "kind" labels, so ensure only
60+
the correct ones are applied.
61+
62+
Anyone can add these [labels].
63+
64+
- **kind/api-change:** API change, that will require special API review
65+
- **kind/bug:** related to a bug
66+
- **kind/cleanup:** cleaning up code, process, or technical debt
67+
- **kind/deprecation:** deprecation, that will require special API review
68+
- **kind/design:** related to design. not commonly used
69+
- **kind/documentation:** related to documentation (including code comments)
70+
- **kind/failing-test:** related to a consistently or frequently failing test
71+
- **kind/feature:** related to a new feature or enhancement; should have an
72+
associated KEP linked
73+
- **kind/flake:** related to a flaky test
74+
- **kind/regression:** related to a regression in performance or functionality
75+
from a prior release
76+
- **kind/support:** not applicable to PRs
77+
78+
### needs-priority
79+
80+
You can take a quick look at what the PR is addressing and then apply a
81+
priority label.
82+
83+
Anyone can add this [label][labels].
84+
85+
- **priority/critical-urgent:** Urgent bug fix, required ASAP. If not
86+
addressed, will block a release. These issues should always be discussed in
87+
the `#sig-node` channel on Slack.
88+
- **priority/important-soon:** Needs to be completed this release. Important
89+
bug fixes + KEPs targeted for the current milestone.
90+
- **priority/important-longterm:** Has an attached issue/KEP, but unclear what
91+
the specific priority is.
92+
- **priority/backlog:** Non-urgent changes such as minor performance
93+
optimizations, improving error logs, increasing test code coverage, code
94+
refactoring, and addressing static code analysis issues.
95+
96+
### needs-triage
97+
98+
There are two more things to check before accepting a PR for triage.
99+
100+
The first is whether the SIG is correct. If the PR does not appear to touch SIG
101+
Node code or require a SIG Node approver, you should remove the SIG Node label,
102+
and add other SIG labels as appropriate.
103+
104+
The second is verifying the kind of PR. Most will be bug fixes, cleanups, or
105+
documentation. Feature PRs should generally have an attached KEP. API changes
106+
and deprecations require special review; those labels may be mistakenly
107+
applied, so check over the PR to make sure they're accurate.
108+
109+
Once you've quickly looked over a PR, applied the appropriate labels, and it
110+
looks ready to proceed to review (i.e. it doesn't have any labels that would
111+
mean it's waiting on more work from the author), you can mark the PR as triaged
112+
with `/triage accepted`.
113+
114+
Only [Kubernetes org members] can add this [label][labels].
115+
116+
## Waiting on Author
117+
118+
This column means that the PR is waiting on some action from the author. A
119+
reviewer may have requested changes, or a PR may have one of the following
120+
do-not-merge [labels]:
121+
122+
- **do-not-merge/hold:** usually set by a reviewer
123+
- **do-not-merge/work-in-progress:** usually set by an author
124+
- **do-not-merge/release-note-needed:** needs a release note to be added by the
125+
author, [Kubernetes org members] can override with `/release-note-none` if
126+
not required
127+
- **do-not-merge/contains-merge-commits:** PR needs to be rebased
128+
- **needs-rebase:** PR needs to be rebased
129+
130+
If [tests are failing] due to an issue with the change (rather than a [flake]), PRs
131+
should also be assigned to this column.
132+
133+
Authors are encouraged to fix any of the label issues above, resolve or reply
134+
to all PR feedback, and leave a comment indicating when their PR is ready for
135+
review.
136+
137+
PRs that do not have any of the above labeled in this column should be
138+
evaluated occasionally to see if they are ready for review.
139+
140+
If PRs are not updated for a long period (90d), they will be marked as stale.
141+
After 30d more, they will be marked as rotten, and then closed automatically.
142+
Reviewers should feel free to close stale PRs (4+ months of no changes) with a
143+
note that the author can reopen when they are ready to work on it.
144+
145+
[tests are failing]: contributors/devel/sig-testing/testing.md#troubleshooting-a-failure
146+
[flake]: contributors/devel/sig-testing/flaky-tests.md
147+
148+
## Waiting on Reviewer
149+
150+
This PR needs review! If you're not sure how to review a PR, start by
151+
familiarizing yourself with the Kubernetes [pull request guidelines] and
152+
[review guidelines].
153+
154+
PRs in this column must have the following [labels] set:
155+
156+
- triage-accepted
157+
- priority
158+
- kind
159+
160+
Only [Kubernetes org members] can add an `/lgtm`.
161+
162+
If you want to become an official Node reviewer, you should read through the
163+
[reviewer responsibilities and requirements].
164+
165+
As part of code review, you should ensure that:
166+
167+
- the change is needed
168+
- the metadata on the PR and the release note are accurate
169+
- the change works the way it is intended to
170+
- alternative implementations have been explored and this is an appropriate
171+
solution
172+
- the code has been reviewed by everyone it needs to be: it may have an LGTM
173+
label already, but still needs feedback from Node reviewers
174+
175+
TODO: Add some node-specific stuff here.
176+
177+
[pull request guidelines]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md
178+
[review guidelines]: https://github.com/kubernetes/community/blob/master/contributors/guide/review-guidelines.md
179+
[reviewer responsibilities and requirements]: https://github.com/kubernetes/community/blob/master/community-membership.md#reviewer
180+
181+
## Waiting on Approver
182+
183+
These PRs are waiting on an `approved` label that can only be provided by
184+
approvers. The bot will always tell you whose approval is needed on which
185+
directories ([example comment]), and will update its comment as approvals are
186+
provided.
187+
188+
PRs in this column must have the following [labels] set:
189+
190+
- lgtm
191+
- triage-accepted
192+
- priority
193+
- kind
194+
195+
Check for the bot's comment to see which files still need approvers from the
196+
appropriate OWNERS. If the PR already has an approval for the node components
197+
(commonly, anything in `./pkg/kubelet/*`), you can mark the PR as Done manually
198+
while waiting on other approvers.
199+
200+
Only [Kubernetes approvers] can use `/approve` on a PR to address this
201+
requirement.
202+
203+
[example comment]: https://github.com/kubernetes/kubernetes/pull/97992#issuecomment-759450299
204+
[Kubernetes approvers]: https://github.com/kubernetes/community/blob/master/community-membership.md#approver
205+
206+
## Done
207+
208+
This column has automation to include all closed and merged PRs on the board.
209+
Huzzah!
210+
211+
It may also include PRs that have LGTMs and approvals, but are not yet merged
212+
(i.e. requires a non-node approver signoff, API review, or a release team
213+
cherry-pick approval).
214+
215+
TODO: We should archive this column per release.

0 commit comments

Comments
 (0)