RIOT-OS
RFC: Further GitHub labeling improvements
Description
This issue follows on from https://github.com/RIOT-OS/RIOT/issues/10030.
That issue deals only with re-naming current labels, but during the discussion, a number of further points of action are being brought up.
These will be captured below. Feel free to add more points yourself (but preferably not delete).
Further labeling actions:
-
[ ] Add further labels proposed during that discussion. A couple of suggestions:
-
State: under review -
State: inactive -
State: blocked -
Priority: {high|low|medium} -
Review requirement: light -
Review requirement: thorough -
Type: security flaw
-
-
[ ] Issue triaging: sorting and re-labelling issues
-
[ ] Extracting useful information for the CI (show warning if there's a new feature with no test, not test everything if there's only a change in documentation, etc).
-
[ ] Automated label-setting
I think rather than State: inactive a bit more descriptive small set of labels could be helpful, but maybe
also overkill (?):
-
State: waiting on maintainer -
State: waiting on author
Last point about CI: Warnings should rather be solved commenting than labeling IMHO, to give more detailed information. Not testing everything is a good idea, but this could also be done mostly automated (e.g. if a file isn't involved in the applications binary, don't compile that application). But I guess a "CI: Doc only" label might also be helpful.
But I guess a "CI: Doc only" label might also be helpful.
(this I would however rather discuss with the CI team in a separate issue)
Do the State: xxxxxxx labels are automatically set by some kind of scripts or should they be set by maintainers ?
I guess for the future automation of most of the labels would be great, but for now there is no specific plan for that. @danpetry maybe put this onto the list for "Further labeling actions"?
@danpetry maybe put this onto the list for "Further labeling actions"?
It would be great because I think this part will be really time consuming for maintainers. Even if not all State: labels are automated but at least a few of them to avoid scanning from the huge (and increasing ?) PRs list to check the state one by one..
I've put this on the list.
The "issue triaging" is my favourite item on the list above in fact. Yes, it will be time consuming, but organizing our "big ball of mud" of issues and PRs will really pay dividends in helping us prioritize our work - in other words, it will save more time in the long run, and is required really if we want to scale. Some suggestions for how this could be done:
- During hack 'n ack
- Dedicated regular triage sessions
- Ad hoc as/when maintainers want to
- A "triager" status for contributors as a stepping stone to becoming maintainers, with privileges to sort the labels but not review
- During hack 'n ack
- Dedicated regular triage sessions
- Ad hoc as/when maintainers want to
- A "triager" status for contributors as a stepping stone to becoming maintainers, with privileges to sort the labels but not review
The monthly developer meeting the day before the Hack'n'ACK (ping @kYc0o) could also be a possible candidate.
- A "triager" status for contributors as a stepping stone to becoming maintainers, with privileges to sort the labels but not review
Mh... I don't think GitHub's permission levels allow for such a fine-grained configuration. A user can either read, write, administrate, or own a repository. Setting labels is allowed from write onwards, but that also means they can push (i.e. merge PRs) and give effective reviews (i.e. an approval results the PR being not blocked anymore for merging).
Hmm, yeah. This does also head in the direction of stratifying, which doesn't really fit in with our grassroots philosophy as I understand it.
Review requirement: lightReview requirement: thorough
As stated in https://github.com/RIOT-OS/RIOT/issues/10030#issuecomment-425371758, I think these are very much redundant with the labels from the Impact: category (while the latter have additional meaning attached to them, as pointed out by @kaspar030 in https://github.com/RIOT-OS/RIOT/issues/10030#issuecomment-425231162). There might be cases where e.g. Review requirement: light wouldn't be automatically set in tandem with the Impact: minor, but at the moment I can't think of any example. Same goes for Review requirement: thorough.
On the flip-side e.g. for Process: needs >1 ACK I see a clear reason to exist beyond what Impact: major implies: A PR against the maintainer guidelines e.g. needs two ACKs, but the review might not need to be thorough. Likewise, the newly re-introduced RDMs require three ACKs, the review might need to be thorough, but the actual impact of the discussed feature might be pretty low.
Likewise, the newly re-introduced RDMs require three ACKs, the review might need to be thorough, but the actual impact of the discussed feature might be pretty low.
Actually, this might be an argument for having a distinct Review requirement: thorough and Impact: major label... But the thoroughness of the review required here could also be covered by whatever label we introduce to steer the RDM integration process.
As discussed offline, I added Type: security flaw to OP. This label is intended for bug fixes which were already discussed on [email protected] (and solicited from a bug that was reported there or accidentally reported in the issue tracker ;-)).
Review requirement: light Review requirement: thorough
As stated in #10030 (comment), I think these are very much redundant with the labels from the Impact: category (while the latter have additional meaning attached to them, as pointed out by @kaspar030 in #10030 (comment)). There might be cases where e.g. Review requirement: light wouldn't be automatically set in tandem with the Impact: minor, but at the moment I can't think of any example. Same goes for Review requirement: thorough.
I know that the impact category labels imply a certain depth of review. But is this enough to make sure that maintainers actually give it that depth of review?
I know that the impact category labels imply a certain depth of review. But is this enough to make sure that maintainers actually give it that depth of review?
IMHO if this is properly documented and agreed upon amongst maintainers, yes. See also my current documentation on the major label.
#16476 is related to this issue. Are we happy with our labels and should we consider closing this issue ?
At least the priority labels I would like to have introduced. Not sure what happened, but in https://github.com/RIOT-OS/RIOT/issues/10057#issuecomment-425721392 I said, I added them, but I can't find them.
Can we get a consensus to introduce them before closing?
Can we get a consensus to introduce them before closing?
+1 for priority labels
