From f29fe5ced02ec449cfefae3d6056aa594ae88bea Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 26 Jun 2021 00:48:24 +0200 Subject: [PATCH] fix(dev-infra): use API pagination for retrieving project branches (#42666) We rely on a Github API `/branches` request to determine the active release trains. Currently this logic is broken if more than 100 protected branches exist within a repository. This issue surfaced recently where the `items_per_page` setting was set to `30`, causing the merge tooling and release tooling to not detect the proper "latest" release train. This commit uses Github pagination for retrieving branches to determine the active release trains, and makes the logic more long-term proof. PR Close #42666 --- dev-infra/ng-dev.js | 4 +- dev-infra/pr/merge/BUILD.bazel | 1 + .../pr/merge/defaults/integration.spec.ts | 39 ++++++++++++++++++- .../release/versioning/version-branches.ts | 6 +-- .../utils/testing/github-pagination-header.ts | 28 +++++++++++++ 5 files changed, 69 insertions(+), 9 deletions(-) create mode 100644 dev-infra/utils/testing/github-pagination-header.ts diff --git a/dev-infra/ng-dev.js b/dev-infra/ng-dev.js index c7a0b9f07c..ab87dd41df 100755 --- a/dev-infra/ng-dev.js +++ b/dev-infra/ng-dev.js @@ -899,9 +899,7 @@ function getVersionForVersionBranch(branchName) { */ function getBranchesForMajorVersions(repo, majorVersions) { return tslib.__awaiter(this, void 0, void 0, function* () { - // TODO(alxhub): actually paginate this, since eventually the number of branches we have will run - // off the end of the first page of data returned by `listBranches`. - const { data: branchData } = yield repo.api.repos.listBranches({ owner: repo.owner, repo: repo.name, protected: true, per_page: 100 }); + const branchData = yield repo.api.paginate(repo.api.repos.listBranches, { owner: repo.owner, repo: repo.name, protected: true }); const branches = []; for (const { name } of branchData) { if (!isVersionBranch(name)) { diff --git a/dev-infra/pr/merge/BUILD.bazel b/dev-infra/pr/merge/BUILD.bazel index 5781497c84..8a8c21bada 100644 --- a/dev-infra/pr/merge/BUILD.bazel +++ b/dev-infra/pr/merge/BUILD.bazel @@ -38,6 +38,7 @@ ts_library( "//dev-infra/release/config", "//dev-infra/release/versioning", "//dev-infra/utils", + "//dev-infra/utils/testing", "@npm//@types/jasmine", "@npm//@types/node", "@npm//@types/node-fetch", diff --git a/dev-infra/pr/merge/defaults/integration.spec.ts b/dev-infra/pr/merge/defaults/integration.spec.ts index 86959094ff..83a86bb2a8 100644 --- a/dev-infra/pr/merge/defaults/integration.spec.ts +++ b/dev-infra/pr/merge/defaults/integration.spec.ts @@ -7,12 +7,14 @@ */ import * as nock from 'nock'; +import {ParsedUrlQuery} from 'querystring'; import {ReleaseConfig} from '../../../release/config/index'; import {_npmPackageInfoCache, NpmPackageInfo} from '../../../release/versioning/npm-registry'; import {GithubConfig} from '../../../utils/config'; import * as console from '../../../utils/console'; import {GithubClient} from '../../../utils/git/github'; +import {buildGithubPaginationResponseHeader} from '../../../utils/testing/github-pagination-header'; import {TargetLabel} from '../config'; import {getBranchesFromTargetLabel, getTargetLabelFromPullRequest} from '../target-label'; @@ -79,7 +81,35 @@ describe('default target labels', () => { nock(getRepoApiRequestUrl()) .get('/branches') .query(true) - .reply(200, branches.map(name => ({name}))); + .reply(200, branches.slice(0, 29).map(name => ({name}))); + } + + /** + * Mocks a repository branch list API request with pagination. + * https://docs.github.com/en/rest/guides/traversing-with-pagination. + * https://docs.github.com/en/rest/reference/repos#list-branches. + */ + function interceptBranchesListRequestWithPagination(branches: string[]) { + const apiUrl = getRepoApiRequestUrl(); + + // For each branch, create its own API page so that pagination is required + // to resolve all given branches. + for (let index = 0; index < branches.length; index++) { + // Pages start with `1` as per the Github API specification. + const pageNum = index + 1; + const name = branches[index]; + const linkHeader = + buildGithubPaginationResponseHeader(branches.length, pageNum, `${apiUrl}/branches`); + + // For the first page, either `?page=1` needs to be set, or no `page` should be specified. + const queryMatch = pageNum === 1 ? + (params: ParsedUrlQuery) => params.page === '1' || params.page === undefined : + {page: pageNum}; + + nock(getRepoApiRequestUrl()).get('/branches').query(queryMatch).reply(200, [{name}], { + link: linkHeader, + }); + } } async function getBranchesForLabel( @@ -99,7 +129,12 @@ describe('default target labels', () => { it('should detect "master" as branch for target: minor', async () => { interceptBranchVersionRequest('master', '11.0.0-next.0'); interceptBranchVersionRequest('10.2.x', '10.2.4'); - interceptBranchesListRequest(['10.2.x']); + + // Note: We add a few more branches here to ensure that branches API requests are + // paginated properly. In Angular projects, there are usually many branches so that + // pagination is ultimately needed to detect the active release trains. + // See: https://github.com/angular/angular/commit/261b060fa168754db00248d1c5c9574bb19a72b4. + interceptBranchesListRequestWithPagination(['9.8.x', '10.1.x', '10.2.x']); expect(await getBranchesForLabel('target: minor')).toEqual(['master']); }); diff --git a/dev-infra/release/versioning/version-branches.ts b/dev-infra/release/versioning/version-branches.ts index 834210aceb..5f47dc117b 100644 --- a/dev-infra/release/versioning/version-branches.ts +++ b/dev-infra/release/versioning/version-branches.ts @@ -72,10 +72,8 @@ export function getVersionForVersionBranch(branchName: string): semver.SemVer|nu */ export async function getBranchesForMajorVersions( repo: GithubRepoWithApi, majorVersions: number[]): Promise { - // TODO(alxhub): actually paginate this, since eventually the number of branches we have will run - // off the end of the first page of data returned by `listBranches`. - const {data: branchData} = await repo.api.repos.listBranches( - {owner: repo.owner, repo: repo.name, protected: true, per_page: 100}); + const branchData = await repo.api.paginate( + repo.api.repos.listBranches, {owner: repo.owner, repo: repo.name, protected: true}); const branches: VersionBranch[] = []; for (const {name} of branchData) { diff --git a/dev-infra/utils/testing/github-pagination-header.ts b/dev-infra/utils/testing/github-pagination-header.ts new file mode 100644 index 0000000000..094250cbf5 --- /dev/null +++ b/dev-infra/utils/testing/github-pagination-header.ts @@ -0,0 +1,28 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +/** + * Builds the `Link` response header to indicate an API response that is suitable + * for pagination. This follows the specification as outlined within: + * https://docs.github.com/en/rest/guides/traversing-with-pagination + */ +export function buildGithubPaginationResponseHeader( + totalPages: number, currentPage: number, baseUrl: string) { + const links = [`<${baseUrl}?page=1>; rel="first"`, `<${baseUrl}?page=${totalPages}>; rel="last"`]; + + if (currentPage < totalPages) { + links.push(`<${baseUrl}?page=${currentPage + 1}>; rel="next"`); + } + + // Pages start with `1` as per the Github API specification. + if (currentPage > 1) { + links.push(`<${baseUrl}?page=${currentPage - 1}>; rel="prev"`); + } + + return links.join(','); +}