-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Alternate more flexible code owners mechanism, soon to avoid mass pings #336261
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# | ||
# Currently unused! Use CODEOWNERS for now, see workflows/codeowners.yml | ||
# | ||
#################### | ||
# | ||
# This file is used to describe who owns what in this repository. | ||
# Users/teams will get review requests for PRs that change their files. | ||
# | ||
# 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. | ||
# | ||
# This file uses the same syntax as the natively supported CODEOWNERS file, | ||
# see https://help.github.com/articles/about-codeowners/ for documentation. | ||
# However it comes with some notable differences: | ||
# - There is no need for user/team listed here to have write access. | ||
# - No reviews will be requested for PRs that target the wrong base branch. | ||
# | ||
# Processing of this file is implemented in workflows/codeowners.yml |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
name: Codeowners | ||
|
||
# This workflow depends on a GitHub App with the following permissions: | ||
# - Repository > Administration: read-only | ||
# - Organization > Members: read-only | ||
# - Repository > Pull Requests: read-write | ||
# The App needs to be installed on this repository | ||
# the OWNER_APP_ID repository variable needs to be set | ||
# the OWNER_APP_PRIVATE_KEY repository secret needs to be set | ||
|
||
on: | ||
pull_request_target: | ||
types: [opened, ready_for_review, synchronize, reopened, edited] | ||
|
||
env: | ||
# TODO: Once confirmed that this works by seeing that the action would request | ||
# reviews from the same people (or refuse for wrong base branches), | ||
# move all entries from CODEOWNERS to OWNERS and change this value here | ||
# OWNERS_FILE: .github/OWNERS | ||
OWNERS_FILE: .github/CODEOWNERS | ||
# Also remove this | ||
DRY_MODE: 1 | ||
|
||
jobs: | ||
# Check that code owners is valid | ||
check: | ||
name: Check | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: cachix/install-nix-action@08dcb3a5e62fa31e2da3d490afc4176ef55ecd72 # v30 | ||
|
||
# Important: Because we use pull_request_target, this checks out the base branch of the PR, not the PR itself. | ||
# We later build and run code from the base branch with access to secrets, | ||
# so it's important this is not the PRs code. | ||
- uses: actions/checkout@d632683dd7b4114ad314bca15554477dd762a938 # v4.2.0 | ||
with: | ||
path: base | ||
|
||
- name: Build codeowners validator | ||
run: nix-build base/ci -A codeownersValidator | ||
|
||
- uses: actions/create-github-app-token@5d869da34e18e7287c1daad50e0b8ea0f506ce69 # v1.11.0 | ||
id: app-token | ||
with: | ||
app-id: ${{ vars.OWNER_APP_ID }} | ||
private-key: ${{ secrets.OWNER_APP_PRIVATE_KEY }} | ||
|
||
- uses: actions/checkout@d632683dd7b4114ad314bca15554477dd762a938 # v4.2.0 | ||
with: | ||
ref: refs/pull/${{ github.event.number }}/merge | ||
path: pr | ||
|
||
- name: Validate codeowners | ||
run: result/bin/codeowners-validator | ||
env: | ||
OWNERS_FILE: pr/${{ env.OWNERS_FILE }} | ||
GITHUB_ACCESS_TOKEN: ${{ steps.app-token.outputs.token }} | ||
REPOSITORY_PATH: pr | ||
OWNER_CHECKER_REPOSITORY: ${{ github.repository }} | ||
# Set this to "notowned,avoid-shadowing" to check that all files are owned by somebody | ||
philiptaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
EXPERIMENTAL_CHECKS: "avoid-shadowing" | ||
|
||
# Request reviews from code owners | ||
request: | ||
name: Request | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: cachix/install-nix-action@08dcb3a5e62fa31e2da3d490afc4176ef55ecd72 # v30 | ||
|
||
# Important: Because we use pull_request_target, this checks out the base branch of the PR, not the PR head. | ||
# This is intentional, because we need to request the review of owners as declared in the base branch. | ||
philiptaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- uses: actions/checkout@d632683dd7b4114ad314bca15554477dd762a938 # v4.2.0 | ||
|
||
- uses: actions/create-github-app-token@5d869da34e18e7287c1daad50e0b8ea0f506ce69 # v1.11.0 | ||
id: app-token | ||
with: | ||
app-id: ${{ vars.OWNER_APP_ID }} | ||
private-key: ${{ secrets.OWNER_APP_PRIVATE_KEY }} | ||
|
||
- name: Build review request package | ||
run: nix-build ci -A requestReviews | ||
|
||
- name: Request reviews | ||
run: result/bin/request-reviews.sh ${{ github.repository }} ${{ github.event.number }} "$OWNERS_FILE" | ||
env: | ||
GH_TOKEN: ${{ steps.app-token.outputs.token }} | ||
# Don't do anything on draft PRs | ||
DRY_MODE: ${{ github.event.pull_request.draft && '1' || '' }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
{ | ||
buildGoModule, | ||
fetchFromGitHub, | ||
fetchpatch, | ||
}: | ||
buildGoModule { | ||
name = "codeowners-validator"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The last release of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I've thought about this too. It would be even better if this entire alternate mechanism were implemented by the same codebase, instead of this hacky mess of two third-party tools and some shell scripts 😆 Let's keep that in mind for the future, but for now I think it's fine as is :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fork-and-mutate-for-our-own-use-case is the right strategy. As long as we're agreed on that, any interim steps are fine as-is, in my view. 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also it should set pname+version |
||
src = fetchFromGitHub { | ||
owner = "mszostok"; | ||
repo = "codeowners-validator"; | ||
rev = "f3651e3810802a37bd965e6a9a7210728179d076"; | ||
hash = "sha256-5aSmmRTsOuPcVLWfDF6EBz+6+/Qpbj66udAmi1CLmWQ="; | ||
}; | ||
patches = [ | ||
# https://github.com/mszostok/codeowners-validator/pull/222 | ||
(fetchpatch { | ||
name = "user-write-access-check"; | ||
url = "https://github.com/mszostok/codeowners-validator/compare/f3651e3810802a37bd965e6a9a7210728179d076...840eeb88b4da92bda3e13c838f67f6540b9e8529.patch"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it kinda a bit up to luck if the commits get garbage collected in unmerged PRs? When you receive review feedback and force push things, the URL might break over time. |
||
hash = "sha256-t3Dtt8SP9nbO3gBrM0nRE7+G6N/ZIaczDyVHYAG/6mU="; | ||
}) | ||
# Undoes part of the above PR: We don't want to require write access | ||
# to the repository, that's only needed for GitHub's native CODEOWNERS. | ||
# Furthermore, it removes an unneccessary check from the code | ||
# that breaks tokens generated for GitHub Apps. | ||
./permissions.patch | ||
# Allows setting a custom CODEOWNERS path using the OWNERS_FILE env var | ||
./owners-file-name.patch | ||
winterqt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
]; | ||
postPatch = "rm -r docs/investigation"; | ||
vendorHash = "sha256-R+pW3xcfpkTRqfS2ETVOwG8PZr0iH5ewroiF7u8hcYI="; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
diff --git a/pkg/codeowners/owners.go b/pkg/codeowners/owners.go | ||
index 6910bd2..e0c95e9 100644 | ||
--- a/pkg/codeowners/owners.go | ||
+++ b/pkg/codeowners/owners.go | ||
@@ -39,6 +39,10 @@ func NewFromPath(repoPath string) ([]Entry, error) { | ||
// openCodeownersFile finds a CODEOWNERS file and returns content. | ||
// see: https://help.github.com/articles/about-code-owners/#codeowners-file-location | ||
func openCodeownersFile(dir string) (io.Reader, error) { | ||
+ if file, ok := os.LookupEnv("OWNERS_FILE"); ok { | ||
+ return fs.Open(file) | ||
+ } | ||
+ | ||
var detectedFiles []string | ||
for _, p := range []string{".", "docs", ".github"} { | ||
pth := path.Join(dir, p) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
diff --git a/internal/check/valid_owner.go b/internal/check/valid_owner.go | ||
index a264bcc..610eda8 100644 | ||
--- a/internal/check/valid_owner.go | ||
+++ b/internal/check/valid_owner.go | ||
@@ -16,7 +16,6 @@ import ( | ||
const scopeHeader = "X-OAuth-Scopes" | ||
|
||
var reqScopes = map[github.Scope]struct{}{ | ||
- github.ScopeReadOrg: {}, | ||
} | ||
|
||
type ValidOwnerConfig struct { | ||
@@ -223,10 +222,7 @@ func (v *ValidOwner) validateTeam(ctx context.Context, name string) *validateErr | ||
for _, t := range v.repoTeams { | ||
// GitHub normalizes name before comparison | ||
if strings.EqualFold(t.GetSlug(), team) { | ||
- if t.Permissions["push"] { | ||
- return nil | ||
- } | ||
- return newValidateError("Team %q cannot review PRs on %q as neither it nor any parent team has write permissions.", team, v.orgRepoName) | ||
+ return nil | ||
} | ||
} | ||
|
||
@@ -245,10 +241,7 @@ func (v *ValidOwner) validateGitHubUser(ctx context.Context, name string) *valid | ||
for _, u := range v.repoUsers { | ||
// GitHub normalizes name before comparison | ||
if strings.EqualFold(u.GetLogin(), userName) { | ||
- if u.Permissions["push"] { | ||
- return nil | ||
- } | ||
- return newValidateError("User %q cannot review PRs on %q as they don't have write permissions.", userName, v.orgRepoName) | ||
+ return nil | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
let | ||
pinnedNixpkgs = builtins.fromJSON (builtins.readFile ./pinned-nixpkgs.json); | ||
in | ||
{ | ||
system ? builtins.currentSystem, | ||
|
||
nixpkgs ? null, | ||
}: | ||
let | ||
nixpkgs' = | ||
if nixpkgs == null then | ||
fetchTarball { | ||
philiptaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
url = "https://github.com/NixOS/nixpkgs/archive/${pinnedNixpkgs.rev}.tar.gz"; | ||
sha256 = pinnedNixpkgs.sha256; | ||
} | ||
else | ||
nixpkgs; | ||
|
||
pkgs = import nixpkgs' { | ||
inherit system; | ||
config = { }; | ||
overlays = [ ]; | ||
}; | ||
in | ||
{ | ||
inherit pkgs; | ||
requestReviews = pkgs.callPackage ./request-reviews { }; | ||
codeownersValidator = pkgs.callPackage ./codeowners-validator { }; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
{ | ||
lib, | ||
stdenvNoCC, | ||
makeWrapper, | ||
coreutils, | ||
codeowners, | ||
jq, | ||
curl, | ||
github-cli, | ||
gitMinimal, | ||
}: | ||
stdenvNoCC.mkDerivation { | ||
name = "request-reviews"; | ||
src = lib.fileset.toSource { | ||
root = ./.; | ||
fileset = lib.fileset.unions [ | ||
./get-reviewers.sh | ||
./request-reviews.sh | ||
./verify-base-branch.sh | ||
./dev-branches.txt | ||
]; | ||
}; | ||
nativeBuildInputs = [ makeWrapper ]; | ||
dontBuild = true; | ||
installPhase = '' | ||
mkdir -p $out/bin | ||
mv dev-branches.txt $out/bin | ||
for bin in *.sh; do | ||
mv "$bin" "$out/bin" | ||
wrapProgram "$out/bin/$bin" \ | ||
--set PATH ${ | ||
lib.makeBinPath [ | ||
coreutils | ||
codeowners | ||
jq | ||
curl | ||
github-cli | ||
gitMinimal | ||
] | ||
} | ||
done | ||
''; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# Trusted development branches: | ||
# These generally require PRs to update and are built by Hydra. | ||
master | ||
staging | ||
release-* | ||
staging-* | ||
haskell-updates |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
#!/usr/bin/env bash | ||
|
||
# Get the code owners of the files changed by a PR, | ||
# suitable to be consumed by the API endpoint to request reviews: | ||
# https://docs.github.com/en/rest/pulls/review-requests?apiVersion=2022-11-28#request-reviewers-for-a-pull-request | ||
|
||
set -euo pipefail | ||
|
||
log() { | ||
echo "$@" >&2 | ||
} | ||
|
||
if (( "$#" < 5 )); then | ||
log "Usage: $0 GIT_REPO BASE_REF HEAD_REF OWNERS_FILE PR_AUTHOR" | ||
exit 1 | ||
fi | ||
|
||
gitRepo=$1 | ||
baseRef=$2 | ||
headRef=$3 | ||
ownersFile=$4 | ||
prAuthor=$5 | ||
|
||
tmp=$(mktemp -d) | ||
trap 'rm -rf "$tmp"' exit | ||
|
||
git -C "$gitRepo" diff --name-only --merge-base "$baseRef" "$headRef" > "$tmp/touched-files" | ||
readarray -t touchedFiles < "$tmp/touched-files" | ||
log "This PR touches ${#touchedFiles[@]} files" | ||
|
||
# Get the owners file from the base, because we don't want to allow PRs to | ||
# remove code owners to avoid pinging them | ||
git -C "$gitRepo" show "$baseRef":"$ownersFile" > "$tmp"/codeowners | ||
|
||
# Associative arrays with the team/user as the key for easy deduplication | ||
declare -A teams users | ||
|
||
for file in "${touchedFiles[@]}"; do | ||
result=$(codeowners --file "$tmp"/codeowners "$file") | ||
|
||
read -r file owners <<< "$result" | ||
if [[ "$owners" == "(unowned)" ]]; then | ||
log "File $file is unowned" | ||
continue | ||
fi | ||
log "File $file is owned by $owners" | ||
|
||
# Split up multiple owners, separated by arbitrary amounts of spaces | ||
IFS=" " read -r -a entries <<< "$owners" | ||
|
||
for entry in "${entries[@]}"; do | ||
# GitHub technically also supports Emails as code owners, | ||
# but we can't easily support that, so let's not | ||
if [[ ! "$entry" =~ @(.*) ]]; then | ||
warn -e "\e[33mCodeowner \"$entry\" for file $file is not valid: Must start with \"@\"\e[0m" >&2 | ||
# Don't fail, because the PR for which this script runs can't fix it, | ||
# it has to be fixed in the base branch | ||
continue | ||
fi | ||
# The first regex match is everything after the @ | ||
entry=${BASH_REMATCH[1]} | ||
if [[ "$entry" =~ .*/(.*) ]]; then | ||
# Teams look like $org/$team, where we only need $team for the API | ||
# call to request reviews from teams | ||
teams[${BASH_REMATCH[1]}]= | ||
else | ||
# Everything else is a user | ||
users[$entry]= | ||
fi | ||
done | ||
|
||
done | ||
|
||
# Cannot request a review from the author | ||
if [[ -v users[$prAuthor] ]]; then | ||
log "One or more files are owned by the PR author, ignoring" | ||
unset 'users[$prAuthor]' | ||
fi | ||
|
||
# Turn it into a JSON for the GitHub API call to request PR reviewers | ||
jq -n \ | ||
--arg users "${!users[*]}" \ | ||
--arg teams "${!teams[*]}" \ | ||
'{ | ||
reviewers: $users | split(" "), | ||
team_reviewers: $teams | split(" ") | ||
}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may still be worthwhile to keep the native codeowners file around for specific parts of the codebase, that way we can limit who can merge using branch rules on parts that could be considered critical.
Of course that kind usage should probably be limited, and wouldn't help with avoiding pings for those parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, for what it's worth, validate codeowners could probably be contorted slightly to support failing on certain files we specify, and then have a CI step that could be required with a GitHub branch rule to block merge, but that would then need to be overridden either by someone with correct permissions or some more complicated mechanism.
(but that's not something to do for this PR ofc!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah there's a bunch more things we could do here. In particular I'm very interested in implementing local declaration of code ownership, e.g. using
This could then be extended to support specifying required approvals or more:
Or we could even do something like: Request a review by the maintainer, but if they don't respond within a week, request a review by the maintainer of the parent directory, etc.
Of course this all needs to be implemented unless we can find a third-party project that did it already, but the possibilities are almost endless :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To workshop a bit more: why would an NTA not be an owner and vice-versa?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say one would implicitly be added to the other. I guess expressing it like
would make this less confusing