nathan/nixpkgs
1
0
Fork 0
mirror of https://github.com/NixOS/nixpkgs.git synced 2026-07-21 16:11:22 +00:00
Code Issues Packages Projects Releases Wiki Activity

First-class GitHub team reviews (#456481)

Browse Source
This commit is contained in:
Philip Taron
2026-02-06 01:44:31 +00:00
committed by GitHub
parent 6ed97f1e0e 193deb8533
commit ba626b8a60
6 changed files with 243 additions and 109 deletions
Download Patch File Download Diff File Expand all files Collapse all files

88
ci/github-script/bot.js
View File

@@ -160,6 +160,29 @@ module.exports = async ({ github, context, core, dry }) => {
return users[id]
}
// Same for teams
const teams = {}
function getTeam(id) {
if (!teams[id]) {
teams[id] = github
.request({
method: 'GET',
url: '/organizations/{orgId}/team/{id}',
// TODO: Make this work without pull_requests payloads
orgId: context.payload.pull_request.base.user.id,
id,
})
.then((resp) => resp.data)
.catch((e) => {
// Team may have been deleted
if (e.status === 404) return null
throw e
})
}
return teams[id]
}
async function handlePullRequest({ item, stats, events }) {
const log = (k, v) => core.info(`PR #${item.number} - ${k}: ${v}`)
@@ -192,17 +215,49 @@ module.exports = async ({ github, context, core, dry }) => {
})
// Check for any human reviews other than the PR author, GitHub actions and other GitHub apps.
// Accounts could be deleted as well, so don't count them.
const reviews = (
await github.paginate(github.rest.pulls.listReviews, {
...context.repo,
pull_number,
})
).filter(
await github.graphql(
`query($owner: String!, $repo: String!, $pr: Int!) {
repository(owner: $owner, name: $repo) {
pullRequest(number: $pr) {
# Unlikely that there's ever more than 100 reviews, so let's not bother,
# but once https://github.com/actions/github-script/issues/309 is resolved,
# it would be easy to enable pagination.
reviews(first: 100) {
nodes {
state
user: author {
# Only get users, no bots
... on User {
login
# Set the id field in the resulting JSON to GraphQL's databaseId
# databaseId in GraphQL-land is the same as id in REST-land
id: databaseId
}
}
onBehalfOf(first: 100) {
nodes {
slug
}
}
}
}
}
}
}`,
{
owner: context.repo.owner,
repo: context.repo.repo,
pr: pull_number,
},
)
).repository.pullRequest.reviews.nodes.filter(
(r) =>
r.user &&
!r.user.login.endsWith('[bot]') &&
r.user.type !== 'Bot' &&
// The `... on User` makes it such that .login only exists for users,
// but we still need to filter the others out.
// Accounts could be deleted as well, so don't count them.
r.user?.login &&
// Also exclude author reviews, can't request their review in any case
r.user.id !== pull_request.user?.id,
)
@@ -402,6 +457,16 @@ module.exports = async ({ github, context, core, dry }) => {
if (e.code !== 'ENOENT') throw e
}
let team_maintainers = []
try {
team_maintainers = Object.keys(
JSON.parse(await readFile(`${pull_number}/teams.json`, 'utf-8')),
).map((id) => parseInt(id))
} catch (e) {
// Older artifacts don't have the teams.json, yet.
if (e.code !== 'ENOENT') throw e
}
// We set this label earlier already, but the current PR state can be very different
// after handleReviewers has requested reviews, so update it in this case to prevent
// this label from flip-flopping.
@@ -414,14 +479,15 @@ module.exports = async ({ github, context, core, dry }) => {
pull_request,
reviews,
// TODO: Use maintainer map instead of the artifact.
maintainers: Object.keys(
user_maintainers: Object.keys(
JSON.parse(
await readFile(`${pull_number}/maintainers.json`, 'utf-8'),
),
).map((id) => parseInt(id)),
team_maintainers,
owners,
getTeamMembers,
getUser,
getTeam,
})
}
}

2
ci/github-script/merge.js
View File

@@ -171,7 +171,7 @@ async function handleMerge({
async function merge() {
if (dry) {
core.info(`Merging #${pull_number}... (dry)`)
return 'Merge completed (dry)'
return ['Merge completed (dry)']
}
// Using GraphQL mutations instead of the REST /merge endpoint, because the latter

190
ci/github-script/reviewers.js
View File

@@ -6,28 +6,29 @@ async function handleReviewers({
dry,
pull_request,
reviews,
maintainers,
user_maintainers,
team_maintainers,
owners,
getTeamMembers,
getUser,
getTeam,
}) {
const pull_number = pull_request.number
const requested_reviewers = new Set(
pull_request.requested_reviewers.map(({ login }) => login.toLowerCase()),
)
log(
'reviewers - requested_reviewers',
Array.from(requested_reviewers).join(', '),
)
// Users that the PR has already reached, e.g. they've left a review or have been requested for one
const users_reached = new Set([
...pull_request.requested_reviewers.map(({ login }) => login.toLowerCase()),
...reviews.map(({ user }) => user.login.toLowerCase()),
])
log('reviewers - users_reached', Array.from(users_reached).join(', '))
const existing_reviewers = new Set(
reviews.map(({ user }) => user?.login.toLowerCase()).filter(Boolean),
)
log(
'reviewers - existing_reviewers',
Array.from(existing_reviewers).join(', '),
)
// Same for teams
const teams_reached = new Set([
...pull_request.requested_teams.map(({ slug }) => slug.toLowerCase()),
...reviews.flatMap(({ onBehalfOf }) =>
onBehalfOf.nodes.map(({ slug }) => slug.toLowerCase()),
),
])
log('reviewers - teams_reached', Array.from(teams_reached).join(', '))
// Early sanity check, before we start making any API requests. The list of maintainers
// does not have duplicates so the only user to filter out from this list would be the
@@ -35,16 +36,17 @@ async function handleReviewers({
// further down again.
// This is to protect against huge treewides consuming all our API requests for no
// reason.
if (maintainers.length > 16) {
if (user_maintainers.length + team_maintainers.length > 16) {
core.warning('Too many potential reviewers, skipping review requests.')
// Return a boolean on whether the "needs: reviewers" label should be set.
return existing_reviewers.size === 0 && requested_reviewers.size === 0
return users_reached.size === 0 && teams_reached.size === 0
}
const users = new Set([
// Users that should be reached
var users_to_reach = new Set([
...(
await Promise.all(
maintainers.map(async (id) => {
user_maintainers.map(async (id) => {
const user = await getUser(id)
// User may have deleted their account
return user?.login?.toLowerCase()
@@ -55,76 +57,108 @@ async function handleReviewers({
.filter((handle) => handle && !handle.includes('/'))
.map((handle) => handle.toLowerCase()),
])
log('reviewers - users', Array.from(users).join(', '))
const teams = new Set(
owners
.map((handle) => handle.split('/'))
.filter(([org, slug]) => org === context.repo.owner && slug)
.map(([, slug]) => slug),
)
log('reviewers - teams', Array.from(teams).join(', '))
const team_members = new Set(
(await Promise.all(Array.from(teams, getTeamMembers)))
.flat(1)
.map(({ login }) => login.toLowerCase()),
)
log('reviewers - team_members', Array.from(team_members).join(', '))
const new_reviewers = users
.union(team_members)
// We can't request a review from the author.
.difference(new Set([pull_request.user?.login.toLowerCase()]))
log('reviewers - new_reviewers', Array.from(new_reviewers).join(', '))
// Filter users to repository collaborators. If they're not, they can't be requested
// for review. In that case, they probably missed their invite to the maintainers team.
const reviewers = (
await Promise.all(
Array.from(new_reviewers, async (username) => {
// TODO: Restructure this file to only do the collaborator check for those users
// who were not already part of a team. Being a member of a team makes them
// collaborators by definition.
try {
await github.rest.repos.checkCollaborator({
...context.repo,
username,
})
return username
} catch (e) {
if (e.status !== 404) throw e
core.warning(
`PR #${pull_number}: User ${username} cannot be requested for review because they don't exist or are not a repository collaborator, ignoring. They probably missed the automated invite to the maintainers team (see <https://github.com/NixOS/nixpkgs/issues/234293>).`,
)
}
}),
)
).filter(Boolean)
log('reviewers - reviewers', reviewers.join(', '))
users_to_reach = new Set(
(
await Promise.all(
Array.from(users_to_reach, async (username) => {
// TODO: Restructure this file to only do the collaborator check for those users
// who were not already part of a team. Being a member of a team makes them
// collaborators by definition.
try {
await github.rest.repos.checkCollaborator({
...context.repo,
username,
})
return username
} catch (e) {
if (e.status !== 404) throw e
core.warning(
`PR #${pull_number}: User ${username} cannot be requested for review because they don't exist or are not a repository collaborator, ignoring. They probably missed the automated invite to the maintainers team (see <https://github.com/NixOS/nixpkgs/issues/234293>).`,
)
}
}),
)
).filter(Boolean),
)
log('reviewers - users_to_reach', Array.from(users_to_reach).join(', '))
if (reviewers.length > 15) {
// Similar for teams
var teams_to_reach = new Set([
...(
await Promise.all(
team_maintainers.map(async (id) => {
const team = await getTeam(id)
// Team may have been deleted
return team?.slug?.toLowerCase()
}),
)
).filter(Boolean),
...owners
.map((handle) => handle.split('/'))
.filter(
([org, slug]) =>
org.toLowerCase() === context.repo.owner.toLowerCase() && slug,
)
.map(([, slug]) => slug.toLowerCase()),
])
teams_to_reach = new Set(
(
await Promise.all(
Array.from(teams_to_reach, async (slug) => {
try {
await github.rest.teams.checkPermissionsForRepoInOrg({
org: context.repo.owner,
team_slug: slug,
owner: context.repo.owner,
repo: context.repo.repo,
})
return slug
} catch (e) {
if (e.status !== 404) throw e
core.warning(
`PR #${pull_number}: Team ${slug} cannot be requested for review because it doesn't exist or has no repository permissions, ignoring. Probably wasn't added to the nixpkgs-maintainers team (see https://github.com/NixOS/nixpkgs/tree/master/maintainers#maintainer-teams)`,
)
}
}),
)
).filter(Boolean),
)
log('reviewers - teams_to_reach', Array.from(teams_to_reach).join(', '))
if (users_to_reach.size + teams_to_reach.size > 15) {
core.warning(
`Too many reviewers (${reviewers.join(', ')}), skipping review requests.`,
`Too many reviewers (users: ${Array.from(users_to_reach).join(', ')}, teams: ${Array.from(teams_to_reach).join(', ')}), skipping review requests.`,
)
// Return a boolean on whether the "needs: reviewers" label should be set.
return existing_reviewers.size === 0 && requested_reviewers.size === 0
return users_reached.size === 0 && teams_reached.size === 0
}
const non_requested_reviewers = new Set(reviewers)
.difference(requested_reviewers)
// We don't want to rerequest reviews from people who already reviewed.
.difference(existing_reviewers)
// We don't want to rerequest reviews from people who already reviewed or were requested
const users_not_yet_reached = users_to_reach.difference(users_reached)
log(
'reviewers - non_requested_reviewers',
Array.from(non_requested_reviewers).join(', '),
'reviewers - users_not_yet_reached',
Array.from(users_not_yet_reached).join(', '),
)
// We don't want to rerequest reviews from teams who already reviewed or were requested
const teams_not_yet_reached = teams_to_reach.difference(teams_reached)
log(
'reviewers - teams_not_yet_reached',
Array.from(teams_not_yet_reached).join(', '),
)
if (non_requested_reviewers.size === 0) {
if (users_not_yet_reached.size === 0 && teams_not_yet_reached.size === 0) {
log('Has reviewer changes', 'false (skipped)')
} else if (dry) {
core.info(
`Requesting reviewers for #${pull_number}: ${Array.from(non_requested_reviewers).join(', ')} (dry)`,
`Requesting user reviewers for #${pull_number}: ${Array.from(users_not_yet_reached).join(', ')} (dry)`,
)
core.info(
`Requesting team reviewers for #${pull_number}: ${Array.from(teams_not_yet_reached).join(', ')} (dry)`,
)
} else {
// We had tried the "request all reviewers at once" thing in the past, but it didn't work out:
@@ -134,15 +168,17 @@ async function handleReviewers({
await github.rest.pulls.requestReviewers({
...context.repo,
pull_number,
reviewers,
reviewers: users_not_yet_reached,
team_reviewers: teams_not_yet_reached,
})
}
// Return a boolean on whether the "needs: reviewers" label should be set.
return (
non_requested_reviewers.size === 0 &&
existing_reviewers.size === 0 &&
requested_reviewers.size === 0
users_not_yet_reached.size === 0 &&
teams_not_yet_reached.size === 0 &&
users_reached.size === 0 &&
teams_reached.size === 0
)
}