search

RIOT-OS / RIOT

RIOT - The friendly OS for IoT
https://riot-os.org
GNU Lesser General Public License v2.1
4.94k stars 1.99k forks source link

RFC: Further GitHub labeling improvements #10057

Open danpetry opened 6 years ago

danpetry commented 6 years ago

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:

miri64 commented 6 years ago

I think rather than State: inactive a bit more descriptive small set of labels could be helpful, but maybe also overkill (?):

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.

miri64 commented 6 years ago

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)

dylad commented 6 years ago

Do the State: xxxxxxx labels are automatically set by some kind of scripts or should they be set by maintainers ?

miri64 commented 6 years ago

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"?

dylad commented 6 years ago

@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..

danpetry commented 6 years ago

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:

miri64 commented 6 years ago
  • 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).

danpetry commented 6 years ago

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.

miri64 commented 6 years ago
  • Review requirement: light
  • Review 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.

miri64 commented 6 years ago

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.

miri64 commented 6 years ago

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.

miri64 commented 6 years ago

As discussed offline, I added Type: security flaw to OP. This label is intended for bug fixes which were already discussed on security@riot-os.org (and solicited from a bug that was reported there or accidentally reported in the issue tracker ;-)).

miri64 commented 6 years ago

I also added the Priority: category, which was also discussed in #10030.

danpetry commented 6 years ago

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?

miri64 commented 6 years ago

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.

aabadie commented 3 years ago

16476 is related to this issue. Are we happy with our labels and should we consider closing this issue ?

miri64 commented 3 years ago

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?

maribu commented 2 years ago

Can we get a consensus to introduce them before closing?

+1 for priority labels