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

[RFC-0055] Retire inactive nixpkgs committers #55

Merged
merged 5 commits into from
May 25, 2020
Merged
Changes from all commits
Commits
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
153 changes: 153 additions & 0 deletions rfcs/0055-retired-committers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
---
feature: retired-committers
start-date: 2019-08-25
author: Till Höppner
co-authors: Graham Christensen
shepherd-team: (names, to be nominated and accepted by RFC steering committee)
shepherd-leader: (name to be appointed by RFC steering committee)
related-issues: (will contain links to implementation PRs)
---

# Summary
[summary]: #summary

Many people were given push access to the nixpkgs repository, which is kept even if
these committers become inactive. This RFC proposes moving these contributors to
a new team without push access.

# Motivation
[motivation]: #motivation

<!-- Why are we doing this? What use cases does it support? What is the expected
outcome? -->

Each committer represents secrets and access which need to be managed carefully.
These come in the form of passwords, SSH and GPG keys, and leaking them can put nixpkgs
at risk of of unauthorized modification.

Because every secret with push access can be leaked, we should keep their number as low as necessary,
here by deactivating the push access of inactive committers.
tilpner marked this conversation as resolved.
Show resolved Hide resolved
A special case of inactive committers are those who have lost access to their GitHub account entirely,
who would be unable to remove potentially leaked secrets from their account.

As of 2019-08-18, at least 2 committers have officially stepped down, and at least 1 committer has
not pushed to nixpkgs since 2014, but are still able to push directly to nixpkgs.

If implemented in this form, and assuming no further contributions, 7 contributors will be moved at the beginning of 2020.
Mic92 marked this conversation as resolved.
Show resolved Hide resolved


# Detailed design
[design]: #detailed-design

<!-- This is the bulk of the RFC. Explain the design in enough detail for somebody
familiar with the ecosystem to understand, and implement. This should get
into specifics and corner-cases, and include examples of how the feature is
used. -->

Inactive committers will have their push access disabled after not committing to nixpkgs for an entire year.

That year is measured from January 1 to December 31 instead of using a rolling window over the last 12 months,
to be more predictable for committers and reduce the evaluations from 12 times a year to just once a year.

For each committer from the [Nixpkgs Committers team](https://github.com/orgs/NixOS/teams/nixpkgs-committers), the number of commits
in that time range is checked, and the committer is considered inactive if there are none.

Previous committers are moved to a new Nixpkgs Committers Emeritus team, to honor their past contributions.
Members of this team will remain in the GitHub organisation, and may regain push access at a later time.

The process consists of the following steps:

- Running the [reference implementation][implementation] or a functionally equivalent application.
- Checking the correctness of the script output by manually inspecting the recent git history of inactive committers.
This may be done by reviewing the activity page linked in the script output of the reference implementation.
- Notification of the confirmed inactive committers, via a GitHub issue and an email to the committers address,
according to `<nixpkgs/maintainers/maintainer-list.nix>`. The text should include a notice on how to regain
commit permissions. The exact process of that is not part of this RFC, as the process of becoming a committer
in the first place is still not formalised.
- Moving of the confirmed inactive committers from the [Nixpkgs Committers team](https://github.com/orgs/NixOS/teams/nixpkgs-committers) team
to the new Nixpkgs Committers Emeritus team, either directly if sufficient permissions are available,
or indirectly by notifying an organisation administrator.

This process occurs once after the RFC is accepted, and is then repeated at the beginning of each new year.

@Mic92 has committed to performing the yearly process, but any member of the NixOS organisation can execute the above
steps if @Mic92 does not remember to do so in a reasonable amount of time after the beginning of the year.


# Drawbacks
[drawbacks]: #drawbacks

<!-- Why should we *not* do this? -->

- It might put pressure on people because they might lose their hard-earned permissions.
- Lower activity limits might encourage quota contributions of lower quality with the intention of not losing push access.

# Alternatives
[alternatives]: #alternatives

<!-- What other designs have been considered? What is the impact of not doing this? -->

- Committers could keep push access forever.
- We could be even stricter, at the risk of higher contributor churn and losing low-frequency direct contributions.

# Unresolved questions
[unresolved]: #unresolved-questions

<!-- What parts of the design are still TBD or unknowns? -->

- Is one year without commits a good activity threshold?
- How are committers informed about this change, or an impending revocation?
Copy link
Member

@Mic92 Mic92 Oct 11, 2019

Choose a reason for hiding this comment

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

I think they should be informed some time before (maybe two months?), that their push access will be removed unless they add a commit. There might be some extreme circumstances like an illness that knock out someone for a year. I think a year is a good threshold.

Copy link
Member

Choose a reason for hiding this comment

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

An alternative to this alternative is make it very easy to get commit bit back. I don't think there should be any permanence to the commit bit removal. If there is nothing difficult/annoying about getting commit bit back, is there any need to let them know in order to keep it?

Copy link
Member

Choose a reason for hiding this comment

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

But if it is very easy to get the commit bit back, doesn't that kind of negate the security motivation? At least if we assume the accounts/secrets might get compromised.

Copy link
Member

Choose a reason for hiding this comment

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

Good question, I don't know.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Mic92 An email (only required contact information in maintainer-list) two months before the end of the year sounds good.

@grahamc I definitely want a notification, it should not feel unreasonable or be unexpected, but I'm not sure the requirement for re-activation should be subject of this RFC.

If a limit is needed, something with more friction than just dropping a line on IRC, but less than the current initial limit (?) of 50 recent PRs with non-trivial contributions should be acceptable, perhaps a tenth of that.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about unresponsive maintainers and committer issues?

Hello, x committer has been unresponsive for x days.
They maintain:

List of things
- 
-

If anyone knows how to get in contact with them please let us know.

[Detailed section about the process with moving commit access]

Another issue is what do we do with packages that say they're maintained by them?
We need to be able to move them out of meta.maintainers as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@worldofpeace It's possible to keep maintaining their things without commit access, so I argue that removing them as a maintainer should not be part of this RFC.

Of course that's unlikely to happen, considering they haven't contributed in the past year, but expanding this RFC to affect all maintainers (as opposed to just the committers) and removing maintainers from packages (and potentially removing packages if they become unmaintained) feels very far out of scope for now.


# Future work
[future]: #future-work

<!-- What future work, if any, would be implied or impacted by this feature
without being directly part of the work? -->

- The threshold may need adjustment in the future.

# Reference implementation
[implementation]: #reference-implementation

```py
#! /usr/bin/env nix-shell
#! nix-shell -I nixpkgs=https://github.com/nixos/nixpkgs-channels/archive/1412af4b2cfae71d447164097d960d426e9752c0.tar.gz -i python3 -p "python3.withPackages (p: [ p.PyGithub ])"

# nixpkgs-inactive-committers expects an API token passed in the environment as GITHUB_TOKEN
# Such a token can be created at https://github.com/settings/tokens
# Make sure to enable the read:org scope

from sys import stderr
from github import Github
from datetime import date, time, datetime
import os

year = date.today().year - 1
start_of_year = datetime.combine(date(year, 1, 1), time.min)

print(f'Reporting from {start_of_year}')

gh = Github(os.environ['GITHUB_TOKEN'],
user_agent='nixpkgs-inactive-committers',
per_page=100, timeout=90, retry=5)
print(gh.get_rate_limit(), file=stderr)

org = gh.get_organization('nixos')
nixpkgs = org.get_repo('nixpkgs')
committers = org.get_team_by_slug('nixpkgs-committers').get_members()
sorted_committers = sorted(list(committers), key=lambda c: c.login.lower())

def hasCommit(commits):
# totalCount is borked, len(list(...)) eats too many API calls
try:
c = commits[0]
return True
except IndexError:
return False

for member in sorted_committers:
commits = nixpkgs.get_commits(author=member, since=start_of_year)

if not hasCommit(commits):
print(f'{member.login:<20} https://github.com/NixOS/nixpkgs/commits?author={member.login}')
```