Skip to content

Comments

CODEOWNERS: add @NixOS/systemd for systemd files#164850

Merged
flokli merged 6 commits intoNixOS:masterfrom
bobvanderlinden:patch-3
Mar 22, 2022
Merged

CODEOWNERS: add @NixOS/systemd for systemd files#164850
flokli merged 6 commits intoNixOS:masterfrom
bobvanderlinden:patch-3

Conversation

@bobvanderlinden
Copy link
Member

Description of changes

In response to review spam on #164016 and a follow-up conversation on Matrix https://matrix.to/#/#systemd:nixos.org

There is a team called @NixOS/systemd, but CODEOWNERS doesn't refer to this team. This results in PRs regarding systemd will add a reviewer unrelated to systemd to the PR.

This PR attempts to make @NixOS/systemd codeowner for all systemd-related files.

When #164016 is merged, this file needs to include /nixos/modules/systemd instead of /nixos/modules/systemd-*.

The GitHub linter tells me @NixOS/systemd needs to have write access to nixpkgs. It seems NixOS/darwin-maintainers is also in there with the same warning. Is this true to make automatic adding of reviewers work? If not, should @NixOS/systemd have write access to be able to merge systemd PRs?

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: policy discussion Discuss policies to work in and around Nixpkgs label Mar 19, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Mar 19, 2022
@bobby285271
Copy link
Member

bobby285271 commented Mar 19, 2022

Only adding @NixOS/systemd won't work since the team itself has no write access (and same to @NixOS/darwin-maintainers). I think you will need to manually list the team members that have write access in this repo.

(There is also #162044 that tries to remove non-committer users/teams from the file)

@bobvanderlinden
Copy link
Member Author

Only adding @NixOS/systemd won't work since the team itself has no write access (and same to @NixOS/darwin-maintainers).

Thanks for the suggestion. I incorrectly followed the one existing examples. Another vote for #162044 to clean things up and make sure there are no incorrect examples to pull from anymore 👍

I added the members of systemd as well. I kept the systemd team. I'll remove the systemd team once #162044 is merged.

@flokli
Copy link
Member

flokli commented Mar 21, 2022

I added @Mic92 and @kloenk to the NixOS/systemd team, they also are a maintainer of the systemd package.

I'd personally prefer if we wouldn't need to manually maintain a list of usernames, it feels quite laborious.

@zimbatm
Copy link
Member

zimbatm commented Mar 21, 2022

@NixOS/systemd now has write access to nixpkgs. Note that this solution is not scaleable to all the packages, but systemd plays a special role in the project. It would be nice to be able to replace CODEOWNERS to something that userstands nix maintainers.

@zowoq
Copy link
Contributor

zowoq commented Mar 21, 2022

One member of the team didn't already have write access via nixpkgs-committers but now has write access via systemd.

Also see the note on the nixos/security team:

https://github.com/orgs/NixOS/teams/security
Note: this team has write access. Only people with existing write access should be added, as they will gain write access through this team.

@flokli
Copy link
Member

flokli commented Mar 21, 2022

@zowoq I removed @kloenk from that team for now, even though I'd have no problem giving them commit access.

I added the same note to the systemd team.

@flokli
Copy link
Member

flokli commented Mar 21, 2022

@bobvanderlinden can you update this to only include the team and @kloenk directly?

@zowoq
Copy link
Contributor

zowoq commented Mar 21, 2022

Not really a problem at the moment with only the one non-owner team with a few members but something to keep in mind is if someone is removed from nixpkgs-committers they also need to be removed from other teams with commit access somehow. Mostly thinking of maintainers who are removed from nixpkgs-committers due to inactivity.

@bobvanderlinden
Copy link
Member Author

Nice! The team having write access makes sense 👍 Makes the CODEOWNERS cleaner as well.

@bobvanderlinden can you update this to only include the team and @kloenk directly?

done 👍

@sternenseemann

This comment was marked as resolved.

bobvanderlinden and others added 3 commits March 22, 2022 19:22
Co-authored-by: zowoq <59103226+zowoq@users.noreply.github.com>
@bobvanderlinden
Copy link
Member Author

bobvanderlinden commented Mar 22, 2022

@NixOS/systemd now has write access to nixpkgs. Note that this solution is not scaleable to all the packages, but systemd plays a special role in the project. It would be nice to be able to replace CODEOWNERS to something that userstands nix maintainers.

The nixpkgs-commiters team has lost Write access to the repository, is that related to this change?

@flokli any idea?


Applied suggested changes

@flokli
Copy link
Member

flokli commented Mar 22, 2022

The nixpkgs-commiters team has lost Write access to the repository, is that related to this change?

@flokli any idea?

This was a mistake and has been reverted since. nixpkgs-committers should have commit access again.

@flokli
Copy link
Member

flokli commented Mar 22, 2022

Let's merge this in. It now only has the systemd team in codeowners, so no more errors.

@flokli flokli merged commit 66d5718 into NixOS:master Mar 22, 2022
@zowoq
Copy link
Contributor

zowoq commented Mar 23, 2022

Would have been better if this had been just one commit rather than six.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: policy discussion Discuss policies to work in and around Nixpkgs 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants