diff --git a/ci/eval/compare/default.nix b/ci/eval/compare/default.nix index f1473367a7d9a..9791d26e95509 100644 --- a/ci/eval/compare/default.nix +++ b/ci/eval/compare/default.nix @@ -161,13 +161,12 @@ 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); }) - users - teams + maintainers packages ; in @@ -179,12 +178,10 @@ runCommand "compare" cmp-stats codeowners ]; - users = builtins.toJSON users; - teams = builtins.toJSON teams; + maintainers = builtins.toJSON maintainers; packages = builtins.toJSON packages; passAsFile = [ - "users" - "teams" + "maintainers" "packages" ]; } @@ -265,7 +262,6 @@ runCommand "compare" done - cp "$usersPath" "$out/maintainers.json" - cp "$teamsPath" "$out/teams.json" + cp "$maintainersPath" "$out/maintainers.json" cp "$packagesPath" "$out/packages.json" '' diff --git a/ci/eval/compare/maintainers.nix b/ci/eval/compare/maintainers.nix index a0180e10f193e..6eb983c9c5f63 100644 --- a/ci/eval/compare/maintainers.nix +++ b/ci/eval/compare/maintainers.nix @@ -1,5 +1,7 @@ { lib, +}: +{ changedattrs, changedpathsjson, removedattrs, @@ -49,16 +51,15 @@ 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)}/"; - # 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 [ ]; + # 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 [ ]; } )) # 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.users != [ ] || pkg.teams != [ ])) + (lib.filter (pkg: pkg.maintainers != [ ])) ]; relevantFilenames = @@ -93,44 +94,20 @@ let attrsWithModifiedFiles = lib.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames; - userPings = + listToPing = lib.concatMap ( pkg: map (maintainer: { - type = "user"; - userId = maintainer.githubId; + id = maintainer.githubId; + inherit (maintainer) github; packageName = pkg.name; - }); - - 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 + dueToFiles = pkg.filenames; + }) pkg.maintainers ) attrsWithModifiedFiles; - 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))) - ]; + byMaintainer = lib.groupBy (ping: toString ping.id) listToPing; in { - users = byUser; - teams = byTeam; - packages = lib.catAttrs "name" attrsWithModifiedFiles; + maintainers = lib.mapAttrs (_: lib.catAttrs "packageName") byMaintainer; + + packages = lib.catAttrs "packageName" listToPing; } diff --git a/ci/github-script/bot.js b/ci/github-script/bot.js index 76055a3c2d4c8..efb0b1e0a11c1 100644 --- a/ci/github-script/bot.js +++ b/ci/github-script/bot.js @@ -160,29 +160,6 @@ 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}`) @@ -215,49 +192,17 @@ 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.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( + await github.paginate(github.rest.pulls.listReviews, { + ...context.repo, + pull_number, + }) + ).filter( (r) => - // 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 && + !r.user.login.endsWith('[bot]') && + r.user.type !== 'Bot' && r.user.id !== pull_request.user?.id, ) @@ -457,16 +402,6 @@ 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. @@ -479,15 +414,14 @@ module.exports = async ({ github, context, core, dry }) => { pull_request, reviews, // TODO: Use maintainer map instead of the artifact. - user_maintainers: Object.keys( + 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 d4b5e4a0b8263..14d6fd3762aba 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 fbbcc996eba91..d10b9f6a98e2a 100644 --- a/ci/github-script/reviewers.js +++ b/ci/github-script/reviewers.js @@ -6,29 +6,28 @@ async function handleReviewers({ dry, pull_request, reviews, - user_maintainers, - team_maintainers, + maintainers, owners, + getTeamMembers, getUser, - getTeam, }) { const pull_number = pull_request.number - // 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 requested_reviewers = new Set( + pull_request.requested_reviewers.map(({ login }) => login.toLowerCase()), + ) + log( + 'reviewers - requested_reviewers', + Array.from(requested_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(', ')) + const existing_reviewers = new Set( + reviews.map(({ user }) => user?.login.toLowerCase()).filter(Boolean), + ) + log( + 'reviewers - existing_reviewers', + Array.from(existing_reviewers).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 @@ -36,17 +35,16 @@ async function handleReviewers({ // further down again. // This is to protect against huge treewides consuming all our API requests for no // reason. - if (user_maintainers.length + team_maintainers.length > 16) { + if (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 users_reached.size === 0 && teams_reached.size === 0 + return existing_reviewers.size === 0 && requested_reviewers.size === 0 } - // Users that should be reached - var users_to_reach = new Set([ + const users = new Set([ ...( await Promise.all( - user_maintainers.map(async (id) => { + maintainers.map(async (id) => { const user = await getUser(id) // User may have deleted their account return user?.login?.toLowerCase() @@ -57,108 +55,76 @@ 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. - 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(', ')) - - // 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(', ')) + 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(', ')) - if (users_to_reach.size + teams_to_reach.size > 15) { + if (reviewers.length > 15) { core.warning( - `Too many reviewers (users: ${Array.from(users_to_reach).join(', ')}, teams: ${Array.from(teams_to_reach).join(', ')}), skipping review requests.`, + `Too many reviewers (${reviewers.join(', ')}), skipping review requests.`, ) // Return a boolean on whether the "needs: reviewers" label should be set. - return users_reached.size === 0 && teams_reached.size === 0 + return existing_reviewers.size === 0 && requested_reviewers.size === 0 } - // 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) + 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 - 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(', '), + 'reviewers - non_requested_reviewers', + Array.from(non_requested_reviewers).join(', '), ) - if (users_not_yet_reached.size === 0 && teams_not_yet_reached.size === 0) { + if (non_requested_reviewers.size === 0) { log('Has reviewer changes', 'false (skipped)') } else if (dry) { core.info( - `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)`, + `Requesting reviewers for #${pull_number}: ${Array.from(non_requested_reviewers).join(', ')} (dry)`, ) } else { // We had tried the "request all reviewers at once" thing in the past, but it didn't work out: @@ -168,17 +134,15 @@ async function handleReviewers({ await github.rest.pulls.requestReviewers({ ...context.repo, pull_number, - reviewers: users_not_yet_reached, - team_reviewers: teams_not_yet_reached, + reviewers, }) } // Return a boolean on whether the "needs: reviewers" label should be set. return ( - users_not_yet_reached.size === 0 && - teams_not_yet_reached.size === 0 && - users_reached.size === 0 && - teams_reached.size === 0 + non_requested_reviewers.size === 0 && + existing_reviewers.size === 0 && + requested_reviewers.size === 0 ) } diff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix index cfc08ba9847f6..4f314801a7494 100644 --- a/pkgs/stdenv/generic/check-meta.nix +++ b/pkgs/stdenv/generic/check-meta.nix @@ -364,7 +364,6 @@ 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; @@ -630,10 +629,6 @@ 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