proposal: add proposal for issue/PR label convention #65
Conversation
jan--f
changed the title
jan--f
force-pushed
the
issue-pr-labels
branch
from
79b6366 to
6f7c5a7
Compare
Signed-off-by: Jan Fajerski <[email protected]>
jan--f
force-pushed
the
issue-pr-labels
branch
from
6f7c5a7 to
86fd9cc
Compare
beorn7
left a comment
beorn7
left a comment
There was a problem hiding this comment.
Looks good to me in general. However, I also don't feel very strongly about the details here. I guess once we see in in practice, we will see what works well and what needs changes. We shouldn't shy away from cutting out unused parts later. We have way too much label cruft already.
|
|
||
| - Identify which issues and PRs need immediate attention | ||
| - Understand whether action is required from maintainers, reviewers, or authors | ||
| - Distinguish between items that are ready for work versus those that are blocked |
There was a problem hiding this comment.
Most importantly for me is to mark issues that haven't been vetted at all and might just be bogus. E.g. a bug that is not actually a bug, or somebody requests a feature that we do not want to add. I have seen eager contributors to work on those issues, leading to wasted work.
Not sure if we can include those in this line or maybe better add another line describing this use case.
| - PRs and issues that languish without clear ownership or next steps | ||
| - No clear signal for when an item is blocked versus ready for action | ||
| - Maintainer time wasted re-evaluating items that haven't changed state | ||
|
|
There was a problem hiding this comment.
- Contributors starting to work on issues that haven't been vetted and might be bogus.
(I'm sure you'll find a nicer word than "bogus".)
| This proposal targets: | ||
|
|
||
| - Prometheus maintainers who need to triage and review issues/PRs | ||
| - Contributors who need to understand the state of their submissions |
There was a problem hiding this comment.
Something about helping new contributors that are looking for issues to work on?
| - **`triage/accepted`**: Issue has been triaged and accepted as valid work. It is ready for someone to work on. | ||
| - **`triage/needs-information`**: Issue needs more details from the author before it can be properly evaluated or worked on. | ||
|
|
||
| **Workflow**: All issues start with `triage/needs-triage`. During triage, maintainers either replace this with another triage label or remove it entirely. The absence of any triage label (along with issue comments) indicates that triage action has been taken, such as declining or marking as duplicate. |
There was a problem hiding this comment.
Does that imply that an issue without a triage/... label MUST be in closed state?
There was a problem hiding this comment.
That is the usual case I'd say. But I thought an issue owner (or anyone really) might want to remove the label for some reason. Ideally they'd leave a comment. I wouldn't want to prescribe a state change on that.
There was a problem hiding this comment.
The implication is rather reversed, a closed issue shouldn't have a triage/ label.
| - **`review/lgtm`**: "Looks Good To Me" - PR has received technical approval from a reviewer with domain expertise. | ||
| - **`review/approved`**: PR has received final approval from a maintainer with merge rights (per OWNERS or equivalent). | ||
|
|
||
| **Workflow**: All PRs start with `review/needs-review`. After review, this transitions to either `review/changes-requested` or `review/lgtm`. Once approved by appropriate maintainers, it becomes `review/approved`. |
There was a problem hiding this comment.
What about the state "author has processed the requested changes and is now waiting for the next round of review"? Is this also marked as review/needs-review? If so, let's mention that the PR switches back and forth between review/needs-review and review/changes-requested until it gets closed or approved.
I'm a bit unsure about review/approved. Wouldn't we just merge in this case?
Maybe it makes sense in the case where the author has merge rights themself, because we tend to leave merging to the author them to give them the final say. In that case, maybe let's explain the flow here so that the reader understands why we have that label?
There was a problem hiding this comment.
What about the state "author has processed the requested changes and is now waiting for the next round of review"? Is this also marked as
review/needs-review? If so, let's mention that the PR switches back and forth betweenreview/needs-reviewandreview/changes-requesteduntil it gets closed or approved.
Yes it can move back and forth. Will add that.
I'm a bit unsure about review/approved. Wouldn't we just merge in this case?
Maybe it makes sense in the case where the author has merge rights themself, because we tend to leave merging to the author them to give them the final say. In that case, maybe let's explain the flow here so that the reader understands why we have that label?
This comes from the k8s workflow. The idea is that reviews have two sides, technical review and code ownership. lgtm can be added by non-codeowners, but the codeowners get final say. This might be too complicated for Prometheus, so happy to drop review/approved.
|
|
||
| - **Active PR ready for review**: `review/needs-review` (no blocking labels) | ||
| - **PR waiting on author**: `review/changes-requested` | ||
| - **PR approved but held**: `review/approved` + `blocked/hold` |
There was a problem hiding this comment.
I see, another use case for review/approved… :)
| - **`good first issue`**: Good for newcomers to the project | ||
| - **`low hanging fruit`**: Easy to implement |
There was a problem hiding this comment.
It always rubbed me the wrong way that we have these two labels. Their difference is very subtle in practice.
There was a problem hiding this comment.
Agreed, I left out low hanging fruit. Mostly since good first issue is clearer for second language speakers.
| - **Issue needing more info**: `triage/needs-information` | ||
| - **Issue declined or duplicate**: No triage label (removed after triage with explanatory comment) | ||
|
|
||
| ### Existing Label Taxonomies |
There was a problem hiding this comment.
Maybe this section should have stronger recommendations how to deal with the existing labels? Most of these seem useful to keep using on a best effort basis, but it might also be confusing to have so many labels, so we might want to be a bit stricter what to use and what to kick out.
| automatically. Some labels should be abandoned in favor of the structured labels | ||
| proposed above. |
There was a problem hiding this comment.
This should be worded more strongly.
I think the labels in this lists are all good candidates to be replaced by the new taxonomy, and we should say so. Something like "The usage of these labels is replaced by the new labels. These labels will be deprecated once the new labels are introduced."
Signed-off-by: Jan Fajerski <[email protected]>
Signed-off-by: Jan Fajerski <[email protected]>
jan--f
force-pushed
the
issue-pr-labels
branch
from
8c23d92 to
6aec48f
Compare
2 participants
No description provided.