diff --git a/ci/eval/compare/default.nix b/ci/eval/compare/default.nix index 9791d26e95509..f1473367a7d9a 100644 --- a/ci/eval/compare/default.nix +++ b/ci/eval/compare/default.nix @@ -161,12 +161,13 @@ let ); inherit - (callPackage ./maintainers.nix { } { + (callPackage ./maintainers.nix { changedattrs = lib.attrNames (lib.groupBy (a: a.name) changedPackagePlatformAttrs); changedpathsjson = touchedFilesJson; removedattrs = lib.attrNames (lib.groupBy (a: a.name) removedPackagePlatformAttrs); }) - maintainers + users + teams packages ; in @@ -178,10 +179,12 @@ runCommand "compare" cmp-stats codeowners ]; - maintainers = builtins.toJSON maintainers; + users = builtins.toJSON users; + teams = builtins.toJSON teams; packages = builtins.toJSON packages; passAsFile = [ - "maintainers" + "users" + "teams" "packages" ]; } @@ -262,6 +265,7 @@ runCommand "compare" done - cp "$maintainersPath" "$out/maintainers.json" + cp "$usersPath" "$out/maintainers.json" + cp "$teamsPath" "$out/teams.json" cp "$packagesPath" "$out/packages.json" '' diff --git a/ci/eval/compare/maintainers.nix b/ci/eval/compare/maintainers.nix index 6eb983c9c5f63..a0180e10f193e 100644 --- a/ci/eval/compare/maintainers.nix +++ b/ci/eval/compare/maintainers.nix @@ -1,7 +1,5 @@ { lib, -}: -{ changedattrs, changedpathsjson, removedattrs, @@ -51,15 +49,16 @@ let # updates to .json files. # TODO: Support by-name package sets. filenames = lib.optional (lib.length path == 1) "pkgs/by-name/${sharded (lib.head path)}/"; - # TODO: Refactor this so we can ping entire teams instead of the individual members. - # Note that this will require keeping track of GH team IDs in "maintainers/teams.nix". - maintainers = package.meta.maintainers or [ ]; + # meta.maintainers also contains all individual team members. + # We only want to ping individuals if they're added individually as maintainers, not via teams. + users = package.meta.nonTeamMaintainers or [ ]; + teams = package.meta.teams or [ ]; } )) # No need to match up packages without maintainers with their files. # This also filters out attributes where `packge = null`, which is the # case for libintl, for example. - (lib.filter (pkg: pkg.maintainers != [ ])) + (lib.filter (pkg: pkg.users != [ ] || pkg.teams != [ ])) ]; relevantFilenames = @@ -94,20 +93,44 @@ let attrsWithModifiedFiles = lib.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames; - listToPing = lib.concatMap ( + userPings = pkg: map (maintainer: { - id = maintainer.githubId; - inherit (maintainer) github; + type = "user"; + userId = maintainer.githubId; packageName = pkg.name; - dueToFiles = pkg.filenames; - }) pkg.maintainers + }); + + teamPings = + pkg: team: + if team ? github then + [ + { + type = "team"; + teamId = team.githubId; + packageName = pkg.name; + } + ] + else + userPings pkg team.members; + + maintainersToPing = lib.concatMap ( + pkg: userPings pkg pkg.users ++ lib.concatMap (teamPings pkg) pkg.teams ) attrsWithModifiedFiles; - byMaintainer = lib.groupBy (ping: toString ping.id) listToPing; + byType = lib.groupBy (ping: ping.type) maintainersToPing; + + byUser = lib.pipe (byType.user or [ ]) [ + (lib.groupBy (ping: toString ping.userId)) + (lib.mapAttrs (_user: lib.map (pkg: pkg.packageName))) + ]; + byTeam = lib.pipe (byType.team or [ ]) [ + (lib.groupBy (ping: toString ping.teamId)) + (lib.mapAttrs (_team: lib.map (pkg: pkg.packageName))) + ]; in { - maintainers = lib.mapAttrs (_: lib.catAttrs "packageName") byMaintainer; - - packages = lib.catAttrs "packageName" listToPing; + users = byUser; + teams = byTeam; + packages = lib.catAttrs "name" attrsWithModifiedFiles; } diff --git a/ci/github-script/bot.js b/ci/github-script/bot.js index efb0b1e0a11c1..76055a3c2d4c8 100644 --- a/ci/github-script/bot.js +++ b/ci/github-script/bot.js @@ -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, }) } } diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index 14d6fd3762aba..d4b5e4a0b8263 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -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 diff --git a/ci/github-script/reviewers.js b/ci/github-script/reviewers.js index d10b9f6a98e2a..be458ba4eb326 100644 --- a/ci/github-script/reviewers.js +++ b/ci/github-script/reviewers.js @@ -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,109 @@ 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 ).`, - ) - } - }), - ) - ).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 ).`, + ) + } + }), + ) + ).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) - log( - 'reviewers - non_requested_reviewers', - Array.from(non_requested_reviewers).join(', '), + // We don't want to rerequest reviews from people who already reviewed or were requested + const users_not_yet_reached = Array.from( + users_to_reach.difference(users_reached), + ) + log('reviewers - users_not_yet_reached', 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 = Array.from( + teams_to_reach.difference(teams_reached), ) + log('reviewers - teams_not_yet_reached', teams_not_yet_reached.join(', ')) - if (non_requested_reviewers.size === 0) { + if ( + users_not_yet_reached.length === 0 && + teams_not_yet_reached.length === 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}: ${users_not_yet_reached.join(', ')} (dry)`, + ) + core.info( + `Requesting team reviewers for #${pull_number}: ${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 +169,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.length === 0 && + teams_not_yet_reached.length === 0 && + users_reached.size === 0 && + teams_reached.size === 0 ) } diff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix index 4f314801a7494..cfc08ba9847f6 100644 --- a/pkgs/stdenv/generic/check-meta.nix +++ b/pkgs/stdenv/generic/check-meta.nix @@ -364,6 +364,7 @@ let ]; sourceProvenance = listOf attrs; maintainers = listOf (attrsOf any); # TODO use the maintainer type from lib/tests/maintainer-module.nix + nonTeamMaintainers = listOf (attrsOf any); # TODO use the maintainer type from lib/tests/maintainer-module.nix teams = listOf (attrsOf any); # TODO similar to maintainers, use a teams type priority = int; pkgConfigModules = listOf str; @@ -629,6 +630,10 @@ let attrs.meta.maintainers or [ ] ++ concatMap (team: team.members or [ ]) attrs.meta.teams or [ ] ); + # Needed for CI to be able to avoid requesting reviews from individual + # team members + nonTeamMaintainers = attrs.meta.maintainers or [ ]; + identifiers = let # nix-env writes a warning for each derivation that has null in its meta values, so