[RFC 0019] RFC for a maintainers file #19

moretea · 2017-10-29T21:56:27Z

Profpatsch · 2017-10-30T10:41:40Z

We introduced the GitHub CODEOWNERS file a while back.

zimbatm · 2017-10-30T14:58:27Z

I didn't know about the Github CODEOWNERS file. There is probably a documentation problem, what is the best place to put it?

Could the RFC be extended to include the places where the CODEOWNERS would be used?

I also found https://github.com/softprops/git-codeowners which looks really nice, let's get that packaged in.

zimbatm · 2017-10-30T15:15:05Z

nix-env -iA gitAndTools.git-codeowners on master

zimbatm · 2017-10-30T17:33:14Z

So apparently, CODEOWNERS only works if the people have push access to the repo. This is something we want to avoid in the long run. isaacs/github#989 (comment)

Ekleog · 2017-10-30T19:44:24Z

Hmm... Just to bounce back on “only works if the people have push access to the repo. This is something we want to avoid in the long run.”

In my opinion having github able to add a backdoor into nixpkgs is an issue with the current setup (likely noone would notice an additional commit added by github).

In order to solve this issue, I see no other solution than signing all git commits (I wanted to write and submit an RFC somewhere down these lines, but will wait until #18 is solved, given a single RFC stuck in limbo is enough for me). This way each git commit, so each change of the nixpkgs repository, would have a single person asserting “this is not a backdoor that I know of,” and access could be revoked if it was maliciously used.

However, this scheme is pretty much incompatible with not giving people push access to the repo. Or do I misunderstand?

(BTW, a scheme where all commits are pgp-signed could also enforce that only certain OpenPGP keys are allowed to change some directories, once the scripts are setup to check for it on each pull and on hydra, so I guess that it could interoperate nicely with a MAINTAINERS or CODEOWNERS file?)

Edit (after @FRidh's comment): Sorry if I was unclear: I wasn't trying to raise the issue of commit signing, which is indeed a different issue, just to say it doesn't look incompatible to me unless removing push access from maintainers to give them instead to the bot -- in which case the two ideas will likely come into conflict.

FRidh · 2017-10-30T19:50:28Z

@Ekleog signing of commits is a different issue, please discuss that elsewhere.

@zimbatm why would we want to avoid that? The following is written in the header of our CODEOWNERS file:

# This file is used to describe who owns what in this repository. This file does not
# replace `meta.maintainers` but is instead used for other things than derivations
# and modules, like documentation, package sets, and other assets.
#
# For documentation on this file, see https://help.github.com/articles/about-codeowners/
# Mentioned users will get code review requests.

We have code-owners, and maintainers. The code-owners have merge-rights and own "the bigger picture" and maintainers (who may not have merge-rights) maintain individual expressions.

FRidh · 2017-10-30T19:51:10Z

rfcs/0019-maintainers.md

+<!--  Why are we doing this? -->
+There is no explicit (machine-readable) description of who maintains what.
+
+Currently, a bot is used to ping people based on the heuristic of who authored


That bot was replaced a while ago by the CODEOWNERS file.

FRidh · 2017-10-30T19:51:44Z

rfcs/0019-maintainers.md

+[motivation]: #motivation
+
+<!--  Why are we doing this? -->
+There is no explicit (machine-readable) description of who maintains what.


CODEOWNERS file describes who owns portions.
The maintainers fields describe who maintain certain expressions.

FRidh · 2017-10-30T19:53:12Z

rfcs/0019-maintainers.md

+might open further automation to delegate merge access.
+
+<!-- What use cases does it support? -->
+With a maintainers file, we can ping the correct maintainer(s) on a PR.


We can already. By evaluating Nixpkgs we see which derivations are affected and can thus ping those maintainers. We just need a bot for it.

shlevy · 2017-10-30T22:40:19Z

Erm, why do we want to limit push access to the repo exactly? Liberal granting of the commit bit is a great policy IME.

zimbatm · 2017-10-30T22:50:44Z

It's a bit early for the feedback, @moretea and me are still working on the RFC and the rationale will be made clearer when it will be submitted for wider review. I hope that all these questions will be answered by then.

globin

Generally, as I've said before and this being supplementary to the nixbot efforts, @grahamc and I are mostly working on reusing effort by @LnL7 and @domenkozar, I agree with this RFC mostly.

A few important points we have to make sure are respected:

We have to make sure not to duplicate meta.maintainers with a MAINTAINERS file, this has to only be supplementary for paths not being possible to be specified by meta.maintainers e.g. lib; most of the rest should be possible with nix.
IMHO the format of the MAINTAINERS file will have to be specified here before accepting the RFC

globin · 2017-10-31T11:36:08Z

rfcs/0019-maintainers.md

+[motivation]: #motivation
+<!--  Why are we doing this? -->
+Currently we have two mechanism in place that explictly describes who the owner is.
+It either is defined in the `metadata.maintainers` in a `stdenv.mkDerivation`, or when it matches a


meta.maintainers

globin · 2017-10-31T11:36:20Z

rfcs/0019-maintainers.md

+It either is defined in the `metadata.maintainers` in a `stdenv.mkDerivation`, or when it matches a
+pattern in `CODEOWNERS` file.
+
+The metadata.maintainers only covers the packages, and does not used to determine reviewers


does not used -> is not used

globin · 2017-10-31T11:38:15Z

rfcs/0019-maintainers.md

+This makes further delegation of maintainer responsibilities harder.
+
+These two systems do not integrate together.
+The meta data in` pokgs/*` is used by Hydra packages break, and the CODEOWNERS is only used by


pkgs -> pkgs

globin · 2017-10-31T11:39:37Z

rfcs/0019-maintainers.md

+This script will read in a MAINTAINERS files, with support for nested delegation like how the
+`.gitignore` files work.
+
+For files in `pkg/`, the script will try to use the metadata in the packages to find the maintainer.


globin · 2017-10-31T11:40:43Z

rfcs/0019-maintainers.md

+- Use Nix file + script to invoke this.
+
+## Implementation
+In order to facilitate integration in the GitHub PR + Hydra work by @globin and @gchristensen,


This is the issue wrt the comment: mayflower/nixborg#9

globin · 2017-10-31T11:41:24Z

rfcs/0019-maintainers.md

+currently people either have full commit access, or none at all.
+
+Therefore, we should implement a format and automation to handle both the data from the
+`metadata.maintainers` and some maintainers file.


moretea · 2017-10-31T13:06:34Z

@globin yes, the intention is that pkgs/* is special cased in the script, as is described in the RFC.

edolstra · 2017-10-31T14:46:00Z

Previous related discussion: NixOS/nixpkgs#13602

edolstra · 2017-10-31T14:48:29Z

rfcs/0019-maintainers.md

+`.gitignore` files work.
+
+For files in `pkgs/`, the script will try to use the metadata in the packages to find the maintainer.
+If this information cannot be retrieved, it falls back to the rules in the MAINTAINERS file.


It should also use the meta.maintainers field of NixOS modules.

(and tests)

edolstra · 2017-10-31T14:53:52Z

rfcs/0019-maintainers.md

+There are two high level options to take:
+- Data format, such as toml, yaml, or the
+  [Linux kernel's maintainer file format](https://github.com/torvalds/linux/blob/master/MAINTAINERS)
+- Use Nix file + script to invoke this.


This seems like a good option, since then we can include lib/maintainers.nix from that file. And it can easily be converted to JSON for processing by other tools (nix eval --json). The format could be something like:

with import lib/maintainers.nix; [ { description = "Stdenv"; paths = [ "pkgs/stdenv/*" ... ]; maintainers = [ eelco ... ]; } { description = "Haskell Infrastructure"; paths = [ "pkgs/top-level/haskell-packages.nix" "pkgs/development/compilers/ghc/*" "pkgs/development/haskell-modules/lib.nix" ]; maintainers = [ peti ... ]; } ]

edolstra · 2017-10-31T15:06:42Z

@Shley Whether we want to require approval from maintainers is a policy issue separate from having a maintainers file. The latter is useful in any case, because it enables the bot to automatically ping the maintainers who are responsible for the files changed by the PR.

I do think requiring approval for core subsystems or cross-cutting concerns is a good idea, since the situation where any of >80 committers can change (say) stdenv or lib is pretty scary.

edolstra · 2018-06-28T11:12:59Z

I would like to go ahead with this, perhaps with some minor changes:

Not sure if the delegate attribute is needed at the moment. We can also just import and concatenate other maintainers files (e.g. // (import pkgs/development/ruby-modules/maintainers.nix)).
It's a bit confusing to have a maintainers.nix and a lib/maintainers.nix. Maybe the latter can be merged into the former? (It was always a bit strange to have data like maintainer names in lib...)

@moretea Are you still interested in implementing a bot to auto-assign PRs based on the maintainers file?

zimbatm · 2018-06-28T12:04:59Z

rfcs/0019-maintainers.md

+
+## Maintainers file format
+The maintainers file format will be a Nix expression.
+This allows easy integration and sharing of the data in `lib/maintainers.nix`,


has now moved to maintainers/maintainer-list.nix

FRidh · 2018-07-17T08:36:04Z

rfcs/0019-maintainers.md

+It is not used to determine reviewers on GitHub.
+
+The [`CODEOWNERS`](https://help.github.com/articles/about-codeowners/)
+is only used to help GitHub select a reviewer and is centrally managed in a single file.


it is used to define ownership over scopes that are not or cannot be set with meta.maintainers. Examples are additional scripts and structure of and routines for a package set, and thus also library functions.

automatically selecting a reviewer is a nice benefit. Because the mention-bot had become unreliable and discussion on MAINTAINERS file was stalled, the CODEOWNERS file was adopted.

FRidh · 2018-07-17T08:37:28Z

rfcs/0019-maintainers.md

+
+The [`CODEOWNERS`](https://help.github.com/articles/about-codeowners/)
+is only used to help GitHub select a reviewer and is centrally managed in a single file.
+One of the problems is that this loses the data locality, unlike the `.gitignore` files, on which


How so? Because we have two places we describe maintainers? What about nested .gitignore files?

FRidh · 2018-07-17T08:38:11Z

rfcs/0019-maintainers.md

+This makes further delegation of maintainer responsibilities harder.
+
+These two systems do not integrate together.
+The meta data in `pkgs/*` is used by Hydra packages break, and the `CODEOWNERS` is only used by


FRidh · 2018-07-17T08:38:43Z

rfcs/0019-maintainers.md

+
+These two systems do not integrate together.
+The meta data in `pkgs/*` is used by Hydra packages break, and the `CODEOWNERS` is only used by
+GitHub for reviews.


And by individuals who simply want/need to know who owns something?

FRidh · 2018-07-17T08:39:46Z

rfcs/0019-maintainers.md

+The `CODEOWNERS` format is not extensible at all; there is no way to specify e.g. that some parts
+require one positive review of the maintainers and others might require more than one.
+
+Furthermore, GitHub is not suitable to handle the permission that the Nix community needs;


This is quite a different topic. I agree with it, but is authorization also within the scope of this RFC? If not, drop it.

FRidh · 2018-07-17T08:40:55Z

rfcs/0019-maintainers.md

+used. -->
+
+## Requirements
+The input to the maintainers script is a list of files the output will be a machine readable list


and the output

FRidh · 2018-07-17T08:42:07Z

rfcs/0019-maintainers.md

+of maintainers. It will support a `--why` option to print out which decisions points were
+encountered, in order to debug why someone is a maintainer of a file.
+
+This script will read in a maintainers files, with support for nested delegation like what is


What is the motivation for nested files?

FRidh · 2018-07-17T08:43:43Z

rfcs/0019-maintainers.md

+```
+
+## Evaluation of rules
+The first rule int the list that matches is accepted.


shlevy · 2019-01-17T13:11:28Z

Closing all draft RFCs. Please feel free to reopen when this is ready for general community review and the RFC shepherd process.

moretea added 2 commits October 29, 2017 22:49

init maintainers

0ae4cb1

Switch to PR number

abb909f

Add zimbatim as co-author

e2fa106

FRidh suggested changes Oct 30, 2017

View reviewed changes

zimbatm added the status: draft label Oct 30, 2017

Improved draft. Collaboration with @zimbatim

8de1ef1

globin reviewed Oct 31, 2017

View reviewed changes

moretea added 5 commits October 31, 2017 13:59

metadata -> meta typo

0dc1f5f

typo

5f1b536

typo

82fa97b

typo

70e9e2a

link to nixbot's issue for maintainers

e46c50c

edolstra changed the title ~~RFC for a maintainers file~~ [RFC 0019] RFC for a maintainers file Oct 31, 2017

edolstra reviewed Oct 31, 2017

View reviewed changes

Also use metadata for NixOS modules & tests

a99d77c

moretea mentioned this pull request Nov 1, 2017

[RFC 0020] Security On Call #20

Closed

Use a nix expr to describe the maintainers rules.

92eaaff

moretea force-pushed the maintainers branch from a0d68f6 to 212c9e2 Compare November 1, 2017 16:21

Fix markdown metadata

3e49959

moretea force-pushed the maintainers branch from 212c9e2 to 3e49959 Compare November 1, 2017 16:22

moretea added 2 commits November 1, 2017 17:24

Also mention modules and tests in summary

5729da9

Fix some typos and spelling

aa49c35

This was referenced Nov 10, 2017

[RFC 0021] Pull Request Testing on Hydra #21

Closed

Structured maintainers NixOS/nixpkgs#31739

Closed

Ericson2314 mentioned this pull request Dec 18, 2017

CODEOWNERS: Track owners and followers NixOS/nixpkgs#29399

Closed

8 tasks

edolstra mentioned this pull request Jun 28, 2018

Add MAINTAINERS file + associated policy / tools NixOS/nixpkgs#13602

Closed

zimbatm reviewed Jun 28, 2018

View reviewed changes

FRidh reviewed Jul 17, 2018

View reviewed changes

shlevy closed this Jan 17, 2019

[RFC 0019] RFC for a maintainers file #19

[RFC 0019] RFC for a maintainers file #19

Conversation

moretea commented Oct 29, 2017 • edited Loading

Profpatsch commented Oct 30, 2017

zimbatm commented Oct 30, 2017

zimbatm commented Oct 30, 2017

zimbatm commented Oct 30, 2017

Ekleog commented Oct 30, 2017 • edited Loading

FRidh commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlevy commented Oct 30, 2017

zimbatm commented Oct 30, 2017

globin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moretea commented Oct 31, 2017

edolstra commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edolstra commented Oct 31, 2017

edolstra commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlevy commented Jan 17, 2019

moretea commented Oct 29, 2017 •

edited

Loading

Ekleog commented Oct 30, 2017 •

edited

Loading