diff --git a/ci/github-script/bot.js b/ci/github-script/bot.js index c66a91583140..34abe96941fb 100644 --- a/ci/github-script/bot.js +++ b/ci/github-script/bot.js @@ -576,7 +576,10 @@ module.exports = async ({ github, context, core, dry }) => { // Controls level of parallelism. Applies to both the number of concurrent requests // as well as the number of concurrent workers going through the list of PRs. - const maxConcurrent = 20 + // We'll only boost concurrency when we're running many PRs in parallel on a schedule, + // but not for single PRs. This avoids things going wild, when we accidentally make + // too many API requests on treewides. + const maxConcurrent = context.eventName === 'pull_request' ? 1 : 20 await withRateLimit({ github, core, maxConcurrent }, async (stats) => { if (context.payload.pull_request) { diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index 688a7928fe97..674d8153a7e9 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -128,11 +128,20 @@ async function handleMerge({ (await getTeamMembers('nixpkgs-committers')).map(({ id }) => id), ) - const files = await github.paginate(github.rest.pulls.listFiles, { - ...context.repo, - pull_number, - per_page: 100, - }) + const files = ( + await github.rest.pulls.listFiles({ + ...context.repo, + pull_number, + per_page: 100, + }) + ).data + + // Early exit to prevent treewides from using up a lot of API requests (and time!) to list + // all the files in the pull request. For now, the merge-bot will not work when 100 or more + // files are touched in a PR - which should be more than fine. + // TODO: Find a more efficient way of downloading all the *names* of the touched files, + // including an early exit when the first non-by-name file is found. + if (files.length >= 100) return false // Only look through comments *after* the latest (force) push. const lastPush = events.findLastIndex( diff --git a/ci/github-script/reviewers.js b/ci/github-script/reviewers.js index c5f79c11d992..07a77a5fec0d 100644 --- a/ci/github-script/reviewers.js +++ b/ci/github-script/reviewers.js @@ -13,6 +13,34 @@ async function handleReviewers({ }) { const pull_number = pull_request.number + const requested_reviewers = new Set( + pull_request.requested_reviewers.map(({ login }) => login), + ) + log( + 'reviewers - requested_reviewers', + Array.from(requested_reviewers).join(', '), + ) + + const existing_reviewers = new Set( + reviews.map(({ user }) => user?.login).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 + // PR author. Therefore, we check for a limit of 15+1, where 15 is the limit we check + // further down again. + // This is to protect against huge treewides consuming all our API requests for no + // reason. + 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 existing_reviewers.size === 0 && requested_reviewers.size === 0 + } + const users = new Set([ ...(await Promise.all( maintainers.map(async (id) => (await getUser(id)).login), @@ -47,6 +75,9 @@ async function handleReviewers({ 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, @@ -64,24 +95,8 @@ async function handleReviewers({ ).filter(Boolean) log('reviewers - reviewers', reviewers.join(', ')) - const requested_reviewers = new Set( - pull_request.requested_reviewers.map(({ login }) => login), - ) - log( - 'reviewers - requested_reviewers', - Array.from(requested_reviewers).join(', '), - ) - - const existing_reviewers = new Set( - reviews.map(({ user }) => user?.login).filter(Boolean), - ) - log( - 'reviewers - existing_reviewers', - Array.from(existing_reviewers).join(', '), - ) - if (reviewers.length > 15) { - log( + core.warning( `Too many reviewers (${reviewers.join(', ')}), skipping review requests.`, ) // Return a boolean on whether the "needs: reviewers" label should be set.