diff --git a/lib/success.js b/lib/success.js index 48246f29..eaa96d43 100644 --- a/lib/success.js +++ b/lib/success.js @@ -1,4 +1,12 @@ -import { isNil, uniqBy, template, flatten, isEmpty, merge } from "lodash-es"; +import { + isNil, + uniq, + uniqBy, + template, + flatten, + isEmpty, + merge, +} from "lodash-es"; import pFilter from "p-filter"; import AggregateError from "aggregate-error"; import issueParser from "issue-parser"; @@ -75,47 +83,41 @@ export default async function success(pluginConfig, context, { Octokit }) { const releaseInfos = releases.filter((release) => Boolean(release.name)); const shas = commits.map(({ hash }) => hash); - // Get associatedPRs - const associatedPRs = await inChunks(shas, 100, async (chunk) => { - const responsePRs = []; + // Get associatedPRs in two steps. Many commits share the same PRs, so we + // first get only the PR numbers and then load the full fields once per PR. + // This keeps each GraphQL query below GitHub's timeout of about 10 seconds. + const associatedPRNumbers = await inChunks(shas, 50, async (chunk) => { const { repository } = await octokit.graphql( - buildAssociatedPRsQuery(chunk), + buildAssociatedPRNumbersQuery(chunk), { owner, repo }, ); - const responseAssociatedPRs = Object.values(repository).map( - (item) => item.associatedPullRequests, - ); - for (const { nodes, pageInfo } of responseAssociatedPRs) { - if (nodes.length === 0) continue; - - responsePRs.push(...buildIssuesOrPRsFromResponseNode(nodes)); - if (pageInfo.hasNextPage) { - let cursor = pageInfo.endCursor; - let hasNextPage = true; - while (hasNextPage) { - const { repository } = await octokit.graphql( - loadSingleCommitAssociatedPRs, - { owner, repo, sha: response.commit.oid, cursor }, - ); - const { associatedPullRequests } = repository.commit; - responsePRs.push( - ...buildIssuesOrPRsFromResponseNode( - associatedPullRequests.nodes, - "PR", - ), - ); - if (associatedPullRequests.pageInfo.hasNextPage) { - cursor = associatedPullRequests.pageInfo.endCursor; - } else { - hasNextPage = false; - } - } + const numbers = []; + for (const commit of Object.values(repository)) { + let { nodes, pageInfo } = commit.associatedPullRequests; + numbers.push(...nodes.map(({ number }) => number)); + while (pageInfo.hasNextPage) { + const { repository } = await octokit.graphql( + loadSingleCommitAssociatedPRNumbers, + { owner, repo, sha: commit.oid, cursor: pageInfo.endCursor }, + ); + ({ nodes, pageInfo } = repository.commit.associatedPullRequests); + numbers.push(...nodes.map(({ number }) => number)); } } - return responsePRs; + return numbers; }); - const uniqueAssociatedPRs = uniqBy(flatten(associatedPRs), "number"); + const uniqueAssociatedPRs = await inChunks( + uniq(associatedPRNumbers), + 10, + async (chunk) => { + const { repository } = await octokit.graphql( + buildRelatedIssuesQuery(chunk), + { owner, repo }, + ); + return buildIssuesOrPRsFromResponseNode(Object.values(repository)); + }, + ); const prs = await pFilter(uniqueAssociatedPRs, async ({ number }) => { const commits = await octokit.paginate( @@ -172,7 +174,7 @@ export default async function success(pluginConfig, context, { Octokit }) { const uniqueParsedIssues = uniqBy(flatten(parsedIssues), "number"); // Get relatedIssues (or relatedPRs i.e. Issues/PRs that are closed by an associatedPR) - issues = await inChunks(uniqueParsedIssues, 100, async (chunk) => { + issues = await inChunks(uniqueParsedIssues, 10, async (chunk) => { const { repository } = await octokit.graphql( buildRelatedIssuesQuery(chunk.map((issue) => issue.number)), { owner, repo }, @@ -333,9 +335,11 @@ export default async function success(pluginConfig, context, { Octokit }) { } /** - * In order to speed up a function call that handles a big array of items, we split up the - * array in chunks and call the function for each chunk in parallel. At the end we combine the - * results again. + * Split a big array of items in chunks and call the function for each chunk, one after + * another. At the end we combine the results again. + * + * The chunks do not run in parallel: many heavy GraphQL queries at the same time can make + * GitHub fail with timeouts or 502 errors for large releases. * * @template TItem * @template TCallbackResult @@ -345,11 +349,10 @@ export default async function success(pluginConfig, context, { Octokit }) { * @returns TCallbackResult */ async function inChunks(items, chunkSize, callback) { - const chunkCalls = []; + const results = []; for (let i = 0; i < items.length; i += chunkSize) { - chunkCalls.push(callback(items.slice(i, i + chunkSize))); + results.push(await callback(items.slice(i, i + chunkSize))); } - const results = await Promise.all(chunkCalls); return results.flat(); } @@ -447,11 +450,11 @@ function buildRelatedIssuesQuery(numbers) { } /** - * Builds GraphQL query for fetching associated PRs to a list of commit hash (sha) + * Builds GraphQL query for fetching the numbers of PRs associated to a list of commit hash (sha) * @param {Array} shas * @returns {string} */ -function buildAssociatedPRsQuery(shas) { +function buildAssociatedPRNumbersQuery(shas) { return `#graphql query getAssociatedPRs($owner: String!, $repo: String!) { repository(owner: $owner, name: $repo) { @@ -466,19 +469,7 @@ function buildAssociatedPRsQuery(shas) { hasNextPage } nodes { - ${baseFields} - mergeable - changedFiles - mergedAt - isDraft - mergedBy { - login - avatarUrl - url - } - commits { - totalCount - } + number } } } @@ -491,9 +482,9 @@ function buildAssociatedPRsQuery(shas) { } /** - * GraphQL Query to fetch additional associatedPR for commits that has more than 100 associatedPRs + * GraphQL Query to fetch additional associatedPR numbers for commits that has more than 100 associatedPRs */ -const loadSingleCommitAssociatedPRs = `#graphql +const loadSingleCommitAssociatedPRNumbers = `#graphql query getCommitAssociatedPRs($owner: String!, $repo: String!, $sha: String!, $cursor: String) { repository(owner: $owner, name: $repo) { commit: object(oid: $sha) { @@ -505,19 +496,7 @@ const loadSingleCommitAssociatedPRs = `#graphql hasNextPage } nodes { - ${baseFields} - mergeable - changedFiles - mergedAt - isDraft - mergedBy { - login - avatarUrl - url - } - commits { - totalCount - } + number } } } diff --git a/test/success.test.js b/test/success.test.js index e88e1b70..8f6bd19c 100644 --- a/test/success.test.js +++ b/test/success.test.js @@ -95,6 +95,19 @@ test("Add comment and labels to PRs associated with release commits and issues s }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + pr1: prs[1], + }, + }, + }, + ) .postOnce( (url, { body }) => url === "https://api.github.local/graphql" && @@ -341,48 +354,62 @@ test("Add comment and labels to PRs associated with release commits and issues ( .getOnce(`https://api.github.local/repos/${owner}/${repo}`, { full_name: `${owner}/${repo}`, }) - .postOnce("https://api.github.local/graphql", { - data: { - repository: { - commit123: { - oid: "123", - associatedPullRequests: { - pageInfo: { - endCursor: "YE", - hasNextPage: true, + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getAssociatedPRs("), + { + data: { + repository: { + commit123: { + oid: "123", + associatedPullRequests: { + pageInfo: { + endCursor: "YE", + hasNextPage: true, + }, + nodes: [prs[0]], }, - nodes: [prs[0]], }, - }, - commit456: { - oid: "456", - associatedPullRequests: { - pageInfo: { - endCursor: "NI", - hasNextPage: false, + commit456: { + oid: "456", + associatedPullRequests: { + pageInfo: { + endCursor: "NI", + hasNextPage: false, + }, + nodes: [prs[1]], }, - nodes: [prs[1]], }, - }, - commit789: { - oid: "789", - associatedPullRequests: { - pageInfo: { - endCursor: "NI", - hasNextPage: false, + commit789: { + oid: "789", + associatedPullRequests: { + pageInfo: { + endCursor: "NI", + hasNextPage: false, + }, + nodes: [prs[2]], }, - nodes: [prs[2]], }, }, }, }, - }) + ) .postOnce( - "https://api.github.local/graphql", + (url, { body }) => { + if (url !== "https://api.github.local/graphql") return false; + const { query, variables } = JSON.parse(body); + return ( + query.includes("query getCommitAssociatedPRs(") && + variables.sha === "123" && + variables.cursor === "YE" + ); + }, { data: { repository: { commit: { + oid: "123", associatedPullRequests: { pageInfo: { endCursor: "NE", @@ -394,8 +421,20 @@ test("Add comment and labels to PRs associated with release commits and issues ( }, }, }, + ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), { - overwriteRoutes: true, + data: { + repository: { + pr0: prs[0], + pr1: prs[1], + pr2: prs[2], + pr3: prs[3], + }, + }, }, ) .postOnce( @@ -483,10 +522,49 @@ test("Add comment and labels to PRs associated with release commits and issues ( }, }, ) + .getOnce( + `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, + [{ sha: commits[0].hash }], + ) + .getOnce( + `https://api.github.local/repos/${owner}/${repo}/pulls/2/commits`, + [{ sha: commits[1].hash }], + ) + .getOnce( + `https://api.github.local/repos/${owner}/${repo}/pulls/5/commits`, + [{ sha: commits[2].hash }], + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/6/commits`, [{ sha: commits[0].hash }], ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/1/comments`, + { html_url: "https://github.com/successcomment-1" }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/1/labels`, + {}, + { body: ["released"] }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/2/comments`, + { html_url: "https://github.com/successcomment-2" }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/2/labels`, + {}, + { body: ["released"] }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/5/comments`, + { html_url: "https://github.com/successcomment-5" }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/5/labels`, + {}, + { body: ["released"] }, + ) .postOnce( `https://api.github.local/repos/${owner}/${repo}/issues/3/comments`, { html_url: "https://github.com/successcomment-3" }, @@ -567,16 +645,23 @@ test("Add comment and labels to PRs associated with release commits and issues ( t.true( t.context.log.calledWith("Added labels %O to issue #%d", ["released"], 4), ); - t.true( - t.context.log.calledWith( - "Added comment to PR #%d: %s", - 6, - "https://github.com/successcomment-6", - ), - ); - t.true( - t.context.log.calledWith("Added labels %O to PR #%d", ["released"], 6), - ); + // PR 6 is only on the second page of associatedPullRequests for commit 123 + for (const number of [1, 2, 5, 6]) { + t.true( + t.context.log.calledWith( + "Added comment to PR #%d: %s", + number, + `https://github.com/successcomment-${number}`, + ), + ); + t.true( + t.context.log.calledWith( + "Added labels %O to PR #%d", + ["released"], + number, + ), + ); + } t.true(fetch.done()); }); @@ -647,6 +732,19 @@ test("Add comment and labels to PRs associated with release commits and issues c }, }, ) + .postOnce( + (url, { body }) => + url === "https://custom-url.com/prefix/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + pr1: prs[1], + }, + }, + }, + ) .postOnce( (url, { body }) => url === "https://custom-url.com/prefix/graphql" && @@ -987,6 +1085,23 @@ test("Make multiple search queries if necessary", async (t) => { }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + pr1: prs[1], + pr2: prs[2], + pr3: prs[3], + pr4: prs[4], + pr5: prs[5], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, [{ sha: commits[0].hash }], @@ -1235,6 +1350,19 @@ test("Do not add comment and labels for unrelated PR returned by search (compare }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + pr1: prs[1], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, [{ sha: "rebased_sha" }], @@ -1387,6 +1515,128 @@ test("Do not add comment and labels if no PR is associated with release commits" t.true(fetch.done()); }); +test("Load associatedPRs of large releases in small, sequential GraphQL queries", async (t) => { + const owner = "test_user"; + const repo = "test_repo"; + const env = { GITHUB_TOKEN: "github_token" }; + const options = { + branch: "master", + repositoryUrl: `https://github.com/${owner}/${repo}.git`, + }; + // All commits belong to the same PR, as in a release that merges a big + // backport branch. + const commits = Array.from({ length: 120 }, (_, index) => ({ + hash: String(index).padStart(40, "0"), + message: `Commit ${index} message`, + })); + const pr = { number: 1, __typename: "PullRequest", state: "closed" }; + const nextRelease = { version: "1.0.0" }; + const releases = [ + { name: "GitHub release", url: "https://github.com/release" }, + ]; + + let requestsInFlight = 0; + let maxRequestsInFlight = 0; + const fetch = fetchMock + .sandbox() + .getOnce(`https://api.github.local/repos/${owner}/${repo}`, { + full_name: `${owner}/${repo}`, + clone_url: `https://api.github.local/${owner}/${repo}.git`, + }) + .post( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getAssociatedPRs("), + async (url, { body }) => { + requestsInFlight++; + maxRequestsInFlight = Math.max(maxRequestsInFlight, requestsInFlight); + await new Promise((resolve) => setTimeout(resolve, 10)); + requestsInFlight--; + + const shas = [...body.matchAll(/object\(oid: \\"(\w+)\\"\)/g)].map( + ([, sha]) => sha, + ); + const repository = Object.fromEntries( + shas.map((sha) => [ + `commit${sha.slice(0, 12)}`, + { + oid: sha, + associatedPullRequests: { + pageInfo: { endCursor: "NI", hasNextPage: false }, + nodes: [{ number: pr.number }], + }, + }, + ]), + ); + return { data: { repository } }; + }, + ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { data: { repository: { issue1: pr } } }, + ) + .getOnce( + `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, + [{ sha: commits[0].hash }], + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/1/comments`, + { html_url: "https://github.com/successcomment-1" }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/1/labels`, + {}, + { body: ["released"] }, + ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getSRIssues("), + { data: { repository: { issues: { nodes: [] } } } }, + ); + + await success( + { failTitle: "The automated release is failing 🚨" }, + { + env, + options, + commits, + nextRelease, + releases, + logger: t.context.logger, + }, + { + Octokit: TestOctokit.defaults((options) => ({ + ...options, + request: { ...options.request, fetch }, + })), + }, + ); + + const graphqlQueries = fetch + .calls("https://api.github.local/graphql") + .map(([, { body }]) => JSON.parse(body).query); + const associatedPRsQueries = graphqlQueries.filter((query) => + query.includes("query getAssociatedPRs("), + ); + const prDetailsQueries = graphqlQueries.filter((query) => + query.includes("query getRelatedIssues("), + ); + t.is(associatedPRsQueries.length, 3); + t.is(maxRequestsInFlight, 1); + t.is(prDetailsQueries.length, 1); + t.true( + t.context.log.calledWith( + "Added comment to PR #%d: %s", + 1, + "https://github.com/successcomment-1", + ), + ); + t.true(fetch.done()); +}); + test("Do not add comment and labels if no commits is found for release", async (t) => { const owner = "test_user"; const repo = "test_repo"; @@ -1680,6 +1930,20 @@ test("Ignore missing and forbidden issues/PRs", async (t) => { }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + pr1: prs[1], + pr2: prs[2], + }, + }, + }, + ) .postOnce( (url, { body }) => url === "https://api.github.local/graphql" && @@ -1980,6 +2244,18 @@ test("Add custom comment and labels", async (t) => { }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, [{ sha: commits[0].hash }], @@ -2092,6 +2368,18 @@ test("Add custom label", async (t) => { }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, [{ sha: commits[0].hash }], @@ -2197,6 +2485,18 @@ test("Comment on issue/PR without ading a label", async (t) => { }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, [{ sha: commits[0].hash }], @@ -2309,6 +2609,18 @@ test("Editing the release to include all release links at the bottom", async (t) }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, [{ sha: commits[0].hash }], @@ -2432,6 +2744,18 @@ test("Editing the release to include all release links at the top", async (t) => }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, [{ sha: commits[0].hash }], @@ -2552,6 +2876,18 @@ test("Editing the release to include all release links with no additional releas }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, [{ sha: commits[0].hash }], @@ -2661,6 +2997,18 @@ test("Editing the release to include all release links with no additional releas }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, [{ sha: commits[0].hash }], @@ -2763,6 +3111,18 @@ test("Editing the release to include all release links with no releases", async }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, [{ sha: commits[0].hash }], @@ -2867,6 +3227,18 @@ test("Editing the release with no ID in the release", async (t) => { }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, [{ sha: commits[0].hash }], @@ -2986,6 +3358,19 @@ test("Ignore errors when adding comments and closing issues", async (t) => { }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + pr1: prs[1], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, [{ sha: commits[0].hash }], @@ -3500,6 +3885,19 @@ test('Add comment and label to found issues/associatedPR using the "successComme }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + pr1: prs[1], + }, + }, + }, + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/4/commits`, [{ sha: commits[0].hash }], @@ -3741,6 +4139,19 @@ test('Does not comment/label associatedPR and relatedIssues created by "Bots"', }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + pr1: prs[1], + }, + }, + }, + ) .postOnce( (url, { body }) => url === "https://api.github.local/graphql" && @@ -3987,6 +4398,19 @@ test('Does not comment/label "associatedPR" when "successCommentCondition" disab }, }, ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + pr0: prs[0], + pr1: prs[1], + }, + }, + }, + ) .postOnce( (url, { body }) => url === "https://api.github.local/graphql" &&