Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CODEOWNERS file for review requests #27499

Merged
merged 2 commits into from
Jul 30, 2017
Merged

CODEOWNERS file for review requests #27499

merged 2 commits into from
Jul 30, 2017

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Jul 19, 2017

Some time ago GitHub introduced the CODEOWNERS file. The file is similar to the MAINTAINERS file that was proposed in #13602. Code owners will automatically receive a review request.

I've added myself to the files I like to monitor. The mention-bot does not seem to function anymore so I suggest we replace it with this. In the future we may populate this file automatically.

@edolstra
Copy link
Member

Interesting. I see 2 problems with CODEOWNERS:

  • It duplicates meta.maintainers (for packages and modules) and thus can get out of sync.

  • Since it's a single file, it will be the source of a large number of merge conflicts. E.g. a cherry-pick on the stable branch will become much more likely to fail.

So my preference would be to use CODEOWNERS only for files that are not covered by meta.maintainers.

@FRidh
Copy link
Member Author

FRidh commented Jul 19, 2017

I suppose we could divide the file into two parts:

  1. The top part which is periodically populated from meta.maintainers. This should be done on each branch.
  2. The lower part which is manually populated and takes precedence because order matters. There wouldn't be many of these entries and they would change also only rarely.

Cherry-picking shouldn't be an issue then I think.

Otherwise, we indeed keep it as @edolstra suggested and later on try to implement some kind of bot that handles meta.maintainers.

@edolstra
Copy link
Member

Please no generated files in the repository. This requires that people know how to run the generator and doesn't solve the code churn problem.

@fpletz
Copy link
Member

fpletz commented Jul 19, 2017

@volth Regarding vulnerability roundups, the software tools @grahamc developed don't work reliably anymore. For instance, LWN discontinued its vulnerability database which we were using. We are currently looking at new tools like vulnix but that doesn't work on a nixpkgs checkout yet. The security team is nonetheless monitoring the relevant channels to deliver security updates in a timely manner.

@matthewbauer
Copy link
Member

Why not just get rid of .mention-bot and put that in CODEOWNERS for now? It looks like CODEOWNERS is a little bit more standardized than mention bot and besides I don't think mention bot is even working now.

Since it's a single file, it will be the source of a large number of merge conflicts. E.g. a cherry-pick on the stable branch will become much more likely to fail.

Chromium repos solve this problem by having OWNERS throughout their tree. This at least means that you only have to deal with fairly small OWNERS files (although it does probably litter the structure a bit). Does GitHub support this?

@FRidh
Copy link
Member Author

FRidh commented Jul 22, 2017

Why not just get rid of .mention-bot and put that in CODEOWNERS for now? It looks like CODEOWNERS is a little bit more standardized than mention bot and besides I don't think mention bot is even working now.

Yes I can make that change. mention-bot has indeed been broken for a while again.

One thing by the way to think about. When opening PR's to forks of this repo, those listed in this file will be notified as well (if the fork has the CODEOWNERS file).

FRidh added 2 commits July 28, 2017 15:04
Some time ago GitHub introduced the CODEOWNERS file. The file is similar
to the MAINTAINERS file that was proposed in
NixOS#13602. Code owners will
automatically receive a review request.
because it hasn't been functioning anymore for a while now.
@FRidh
Copy link
Member Author

FRidh commented Jul 28, 2017

I've update the PR to now replace .mention-bot. Please have a look.

Unless there's feedback I'll merge this in 2 days.

@edolstra
Copy link
Member

Looks good to me.

@NeQuissimus
Copy link
Member

LGTM

@FRidh FRidh merged commit 8977bee into NixOS:master Jul 30, 2017
@FRidh FRidh deleted the codeowners branch July 30, 2017 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants