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.

Sign up for GitHub

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

Jump to bottom

maintainers: document expectations #250344

Merged
merged 11 commits into from
Sep 1, 2023
Merged

maintainers: document expectations #250344

Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
34c2c3e
maintainers: document expectations
zimbatm Aug 20, 2023
a888774
change the wording to be a bit more lax on losing maintainer status
zimbatm Aug 20, 2023
10e3f2c
clarify how removal happens
zimbatm Aug 23, 2023
c67b737
expand the reasoning a bit more
zimbatm Aug 23, 2023
9afac4c
Update maintainers/README.md
zimbatm Aug 23, 2023
405ea93
Update maintainers/README.md
zimbatm Aug 23, 2023
9025fb6
Update maintainers/README.md
zimbatm Aug 23, 2023
e7d700b
Update maintainers/README.md
zimbatm Aug 23, 2023
0f92e2c
Update maintainers/README.md
zimbatm Aug 23, 2023
4f78788
Update maintainers/README.md
zimbatm Aug 23, 2023
14d76e5
Update maintainers/README.md
zimbatm Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 59 additions & 5 deletions maintainers/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,64 @@
# Nixpkgs Maintainers

The *Nixpkgs maintainers* are people who have assigned themselves to
maintain specific individual packages. We encourage people who care
about a package to assign themselves as a maintainer. When a pull
request is made against a package, OfBorg will notify the appropriate
maintainer(s).
Unlike other packaging ecosystems, the maintainer doesn't have exclusive
control over the packages and modules they maintain. This more fluid approach
is one reason why we scale to so many packages.

## Definition and role of the maintainer

The main responsibility of a maintainer is to keep the packages they maintain
in a functioning state, and keep up with updates. In order to do that, they
are empowered to make decisions over the packages they maintain.

That being said, the maintainer is not alone proposing changes to the
packages. Anybody (both bots and humans) can send PRs to bump or tweak the
package.

We also allow other non-maintainer committers to merge changes to the package,
provided enough time and priority has been given to the maintainer.

For most packages, we expect committers to wait at least a week before merging
changes not endorsed by a package maintainer (which may be themselves). This should leave enough time
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean the endorsement of one maintainer is enough? What if a package has multiple maintainers? What if they disagree?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if I'm ready to open that rabbit hole. For now, I suggest leaving that unspecified.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, conflict resolution is 2 or 3 magnitudes harder than anything those PRs are doing so far, I would +1 not going into that rabbit hole.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think putting a specific duration on «wait for some maintainer reaction» is good, and everything further is reliant on judgement calls that are hard to predefine. We don't ask to definitely merge once there is a single maintainer approval, after all!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, then lets postpone this. It's just something I'm regularly asking myself when trying to decide if I should just merge something or if maybe I should wait.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess for me the correct question will now be «will I be sincere or hypocritical saying ‘Sorry, after a single maintainer approval I did not understand this is going to be controversial’?».

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have some tooling to get reminded of that time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not want to be mean, but calendars are pretty good for this.

Copy link
Member

@7c6f434c 7c6f434c Aug 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Actually, no, they are not, for multiple reasons: they are hard to set up to get exactly the relevant notifications for this batch when you cut out some time to look at Nixpkgs, and most of them don't integrate well enough with browsing the pull request.

On the other hand, an option for CI re-requesting reviews in case there is a check you want to let finish is useful in any case, and if it exists, maybe remind-me-in-a-week would be doable on top of that.

So the need is real, but not decision changing for this PR.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree; we need some more tooling for this.

for the maintainers to provide feedback.
AndersonTorres marked this conversation as resolved.
Show resolved Hide resolved

For critical packages, this convention needs to be negociated with the
zimbatm marked this conversation as resolved.
Show resolved Hide resolved
maintainer. A critical package is one that causes mass-rebuild, or where an
author is listed in the CODEOWNERS file.
zimbatm marked this conversation as resolved.
Show resolved Hide resolved

In case of critical security updates, the security team might override these
zimbatm marked this conversation as resolved.
Show resolved Hide resolved
heuristics in order to get the fixes in as fast as possible.

In case of conflict, the maintainer takes priority and is allowed to revert
the changes. This can happen for example if the maintainer was on holiday.

### How to become a maintainer

We encourage people who care about a package to assign themselves as a
maintainer. You don't require to have nixpkgs commit access to do so.
zimbatm marked this conversation as resolved.
Show resolved Hide resolved

In order to do so, add yourself to the
[`maintainer-list.nix`](./maintainer-list.nix), and then to the desired
package's `meta.maintainers` list, and send a PR with the changes.
AndersonTorres marked this conversation as resolved.
Show resolved Hide resolved

### How to lose maintainer status

Maintainers that have become inactive on a given package can be removed. This
zimbatm marked this conversation as resolved.
Show resolved Hide resolved
helps us keep an accurate view of what parts of nixpkgs are well maintained or
not.
zimbatm marked this conversation as resolved.
Show resolved Hide resolved

The inactivity measure is currently not strictly enforced. We would typically
look at it if we notice that the author hasn't reacted to package-related
notifications for more than 3 months.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that "no reaction at all" is meant here, correct? I.e. this isn't done on a per-package basis (otherwise one could lose the status if a fellow maintainer is always faster and you don't have anything to add most of the time).

When I first read this paragraph, I wasn't sure, so perhaps this should be rephrased a bit to make this clearer.

Also, we'd to give people who are about to lose their status a heads-up, correct? I guess this should be mentioned here as well :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is a non-pressing change to a package one is a maintainer of, and I guess «wait a week or a bit more for maintainer reaction» should be treated as «maintainer in question» in this case…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing a maintainer requires making a PR so that that person would be notified. But I'll add a paragraph for that.

I still think the test should be per package. Some people are active in some areas of nixpkgs, and have abandoned other parts that they don't care about anymore.

Worst case you can react on the removal PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10e3f2c

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, pretty sure I have completely forgotten about some packages I have maintained back when I used them…


Removing the maintainer happens by making a PR on the package, adding that
person as a reviewer, and then waiting a week for a reaction.

The maintainer is welcome to come back at any time.

### Tools for maintainers

When a pull request is made against a package, OfBorg will notify the
appropriate maintainer(s).
Janik-Haag marked this conversation as resolved.
Show resolved Hide resolved

## Reviewing contributions

Expand Down