github: add CODEOWNERS file by benpicco · Pull Request #13289 · RIOT-OS/RIOT

benpicco · 2020-02-04T22:43:53Z

Contribution description

Our handling of Pull Requests is not always optimal. Often things are lost or fizzle out, which can be a frustrating experience for contributors.

A lot of this comes down to the simple fact that maintainers only have a limited amount of time in a day, but there are some issues that can be improved by technical means:

When creating a PR it is often not obvious who to request a review from.
External contributors are not in the loop on future changes of their code.
It is often not clear who should feel responsible for a PR

GitHub provides the option to create a CODEOWNERS file.
If someone creates a PR that touches files or directories matching a pattern in that file, a review is automatically requested from the assigned users.

Testing procedure

The file provided is very rudimentary.
Please feel free to push to this branch directly if you want to add your name to modules or drivers that you are involved in.

If we have all directories covered in the future, we can add a check to Travis to ensure no new code gets added that doesn't have someone attached who is responsible for it.
This process is employed by Zephyr and the Linux kernel.

Issues/PRs references

Discussed briefly at a previous Virtual Maintainer Assembly and the FOSEM BoF meeting.

CODEOWNERS

kaspar030 · 2020-02-05T08:32:36Z

Good idea this.

CODEOWNERS

cgundogan · 2020-02-06T09:09:17Z

CODEOWNERS

+/tests/cpu_efm32_features/                  @basilfx
+
+Kconfig                                     @leandrolanzieri
+*.md                                        @aabadie @jia200x


Does that mean all *.md files in the repo, no matter in which directory? Isn't this a little bit vague?

That is also the case for sys/net ;-). It's just to point GitHub in the right direction about who to assign. We still can make things more precise later on.

I guess that even if the "wrong" maintainer is occasionally flaged, this will still serve the intended goal: That maintainer is likely aware who would be the right person to dial in and juat do so.

That's for @aabadie and @jia200x to decide (though, I don't doubt that they would happily be triggered for each md file (: )

I would prefer to limit this rules on md files present in the doc directory. Otherwise, I will be triggered for each changes in application README, even the ones I don't want to review.

Does anyone know if only the first match is triggered (so that @aabadie and @jia200x would only get triggered for .md files that don't fall into other categories), or all matches?

From the docs:

# These owners will be the default owners for everything in # the repo. Unless a later match takes precedence, # @global-owner1 and @global-owner2 will be requested for # review when someone opens a pull request. * @global-owner1 @global-owner2 # Order is important; the last matching pattern takes the most # precedence. When someone opens a pull request that only # modifies JS files, only @js-owner and not the global # owner(s) will be requested for a review. *.js @js-owner

Order is important; the last matching pattern takes the most precedence.

Order is important; the last matching pattern takes the most precedence.

right, so we should put these to the start of the file.

Actually Kconfig should still be at the end, so our KConfig maintainer is always in the loop about changes to KConfig since those require some global coherence.

CODEOWNERS

kaspar030 · 2020-02-06T10:44:04Z

CODEOWNERS


+Kconfig                                     @leandrolanzieri @jia200x @cgundogan
+*.md                                        @aabadie @jia200x
+*.murdock                                   @kaspar030


Just the one file:

Suggested change

*.murdock @kaspar030

/.murdock @kaspar030

or would that be /.murdock?

.murdock will be any .murdock file anywhere in the repo
/.murdock will only be the one in the root of the repo

/.murdock will only be the one in the root of the repo

that's the one I mean!

(updated suggestion)

Do you also want .travis.yml?

nope. :) maybe @aabadie wants it!

CODEOWNERS

fjmolinas

Some suggestions.

CODEOWNERS

fjmolinas · 2020-02-06T10:54:03Z

CODEOWNERS

+/sys/riotboot/                              @kaspar030
+/sys/tsrb/                                  @kaspar030
+/sys/usb/                                   @bergzand @dylad @aabadie
+/sys/xtimer/                                @kaspar030 @MichelRottleuthner


Suggested change

/sys/xtimer/ @kaspar030 @MichelRottleuthner

/sys/xtimer/ @Hyungsin @MichelRottleuthner

@Hyungsin is not a maintainer (but maybe that should change ?)

Does @kaspar030 not maintain xtimer anymore?

CODEOWNERS

roberthartung · 2020-02-07T14:02:53Z

I'd be happy to be added to sys/pm_layered/ and maybe even pm.c in the future. (#7595 #7597 #7648 #8207) Maybe along with @kaspar030 ?

aabadie

Added some more propositions for me (and others, sorry @fjmolinas).

Maybe we should also put names for files in makefiles/ ?

CODEOWNERS

aabadie · 2020-02-07T18:40:43Z

CODEOWNERS

+/tests/cpu_efm32_features/                  @basilfx
+
+# KConfig maintainers will be notified about all KConfig changes
+Kconfig                                     @leandrolanzieri @jia200x @cgundogan


Suggested change

Kconfig @leandrolanzieri @jia200x @cgundogan

Kconfig @leandrolanzieri @jia200x @cgundogan

Makefile.dep @aabadie @fjmolinas

Makefile.features @aabadie @fjmolinas

Makefile.include @aabadie @fjmolinas

Putting this here means you will be notified for every new module or driver.

miri64 · 2020-02-11T14:29:49Z

Should we maybe put a final call for this until Thursday? This gives us the opportunity to point to this at the VMA tomorrow and we likewise can finally merge this soon.

fjmolinas · 2020-02-11T16:25:05Z

CODEOWNERS

+Makefile.dep                                @aabadie @fjmolinas
+Makefile.features                           @aabadie @fjmolinas
+Makefile.include                            @aabadie @fjmolinas


I'm worried of the implication of this, I don't feel confident and wont have the time to be responsible for the build-system. At least I think there should be much more people on this list.

kaspar030 · 2020-02-12T10:50:28Z

Hey, we can fiddle with the details forever. I suggest we figure out a solution to the open questions regarding wildcard matches and get this in, then add / change actual codeowners with followups.

miri64 · 2020-02-12T11:05:02Z

Hey, we can fiddle with the details forever. I suggest we figure out a solution to the open questions regarding wildcard matches and get this in, then add / change actual codeowners with followups.

@kaspar030 see #13289 (comment)

benpicco · 2020-02-12T16:27:13Z

The next step would be to add Integration to Travis / Murdock, otherwise this list will be outdated in no time.
A warning should issued when a new file is added that doesn't have a CODEOWNER yet.
It shouldn't block the PR yet, at this time there are still many 'unclaimed' directories where one might add a single file.
E.g. if someone adds a file to boards/common/stm32/include/ as part of board support, they shouldn't 'own' that single file but there should rather a general stm32 maintainer for that. They should only be responsible for maintaining their board directory.

kaspar030

ACK.

Maybe give this another day?

maribu

ACK. There are some boards that I could be default reviewer of, but honestly: I think we really should just get this in as it is asap. All the additional nitpicking could be better done in follow up PRs: Those PRs would be small, easy to review, and could be handled in parallel.

@benpicco: Thanks a lot for addressing this. I think especially for new contributors it this will simplify and speed up the review process significantly.

aabadie reviewed Feb 5, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

cgundogan reviewed Feb 5, 2020

View reviewed changes

CODEOWNERS Show resolved Hide resolved

benpicco force-pushed the CODEOWNDERS branch 3 times, most recently from a37802b to 5c47de4 Compare February 5, 2020 21:51

benpicco removed the State: demonstrator State: This PR (loosely) demonstrates a concept and is not necessarily meant to be merged. label Feb 5, 2020

benpicco force-pushed the CODEOWNDERS branch from 5c47de4 to 2515236 Compare February 5, 2020 22:09

benpicco requested a review from a team February 5, 2020 22:10

kaspar030 reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

kaspar030 reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

kaspar030 reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

kaspar030 reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

kaspar030 reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

kaspar030 reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

kaspar030 reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

kaspar030 reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

kaspar030 reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

cgundogan reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

kaspar030 reviewed Feb 6, 2020

View reviewed changes

yegorich reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

fjmolinas reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

fjmolinas reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Show resolved Hide resolved

fjmolinas reviewed Feb 6, 2020

View reviewed changes

jia200x reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Show resolved Hide resolved

jia200x reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

jia200x reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Show resolved Hide resolved

jia200x reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Show resolved Hide resolved

jia200x reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Show resolved Hide resolved

leandrolanzieri reviewed Feb 6, 2020

View reviewed changes

miri64 reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Show resolved Hide resolved

dylad reviewed Feb 6, 2020

View reviewed changes

CODEOWNERS Show resolved Hide resolved

aabadie reviewed Feb 7, 2020

View reviewed changes

miri64 added the CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs label Feb 11, 2020

gschorcht mentioned this pull request Feb 11, 2020

[RFC] Definition of PSEUDOMODULES in Makefile.include of drivers #10577

Closed

fjmolinas reviewed Feb 11, 2020

View reviewed changes

benpicco force-pushed the CODEOWNDERS branch from a15794a to b29ca44 Compare February 12, 2020 14:09

benpicco removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Feb 12, 2020

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 13, 2020

benpicco force-pushed the CODEOWNDERS branch from b29ca44 to e2bf6b2 Compare February 13, 2020 19:18

CODEOWNERS: add CODEOWNERS file

584fc12

benpicco force-pushed the CODEOWNDERS branch from e2bf6b2 to 584fc12 Compare February 14, 2020 10:09

kaspar030 approved these changes Feb 14, 2020

View reviewed changes

maribu approved these changes Feb 14, 2020

View reviewed changes

benpicco merged commit f437a95 into RIOT-OS:master Feb 16, 2020

benpicco deleted the CODEOWNDERS branch February 16, 2020 16:35

leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020

	/sys/xtimer/ @kaspar030 @MichelRottleuthner
	/sys/xtimer/ @Hyungsin @MichelRottleuthner

Conversation

benpicco commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Uh oh!

Uh oh!

kaspar030 commented Feb 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaspar030 Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benpicco commented Feb 4, 2020 •

edited

Loading

aabadie Feb 6, 2020 •

edited

Loading

kaspar030 Feb 6, 2020 •

edited

Loading

benpicco commented Feb 12, 2020 •

edited

Loading