diff --git a/ng-dev/release/publish/actions.ts b/ng-dev/release/publish/actions.ts index e8084c610..9a666cdf1 100644 --- a/ng-dev/release/publish/actions.ts +++ b/ng-dev/release/publish/actions.ts @@ -43,6 +43,7 @@ import {promptToInitiatePullRequestMerge} from './prompt-merge.js'; import {Prompt} from '../../utils/prompt.js'; import {glob} from 'fast-glob'; import {PnpmVersioning} from './pnpm-versioning.js'; +import {Commit} from '../../utils/git/octokit-types.js'; /** Interface describing a Github repository. */ export interface GithubRepo { @@ -150,28 +151,11 @@ export abstract class ReleaseAction { } /** Gets the most recent commit of a specified branch. */ - protected async getLatestCommitOfBranch(branchName: string): Promise { + protected async getLatestCommitOfBranch(branchName: string): Promise { const { data: {commit}, } = await this.git.github.repos.getBranch({...this.git.remoteParams, branch: branchName}); - return commit.sha; - } - - /** Checks whether the given revision is ahead to the base by the specified amount. */ - private async _isRevisionAheadOfBase( - baseRevision: string, - targetRevision: string, - expectedAheadCount: number, - ) { - const { - data: {ahead_by, status}, - } = await this.git.github.repos.compareCommits({ - ...this.git.remoteParams, - base: baseRevision, - head: targetRevision, - }); - - return status === 'ahead' && ahead_by === expectedAheadCount; + return commit; } /** @@ -560,7 +544,7 @@ export abstract class ReleaseAction { // Keep track of the commit where we started the staging process on. This will be used // later to ensure that no changes, except for the version bump have landed as part // of the staging time window (where the caretaker could accidentally land other stuff). - const beforeStagingSha = await this.getLatestCommitOfBranch(stagingBranch); + const {sha: beforeStagingSha} = await this.getLatestCommitOfBranch(stagingBranch); await this.assertPassingGithubStatus(beforeStagingSha, stagingBranch); await this.checkoutUpstreamBranch(stagingBranch); @@ -703,24 +687,11 @@ export abstract class ReleaseAction { npmDistTag: NpmDistTag, additionalOptions: {showAsLatestOnGitHub: boolean}, ) { - const versionBumpCommitSha = await this.getLatestCommitOfBranch(publishBranch); - - // Ensure the latest commit in the publish branch is the bump commit. - if (!(await this._isCommitForVersionStaging(releaseNotes.version, versionBumpCommitSha))) { - Log.error(` ✘ Latest commit in "${publishBranch}" branch is not a staging commit.`); - Log.error(' Please make sure the staging pull request has been merged.'); - throw new FatalReleaseActionError(); - } - - // Ensure no commits have landed since we started the staging process. This would signify - // that the locally-built release packages are not matching with the release commit on GitHub. - // Note: We expect the version bump commit to be ahead by **one** commit. This means it's - // the direct parent of the commit that was latest when we started the staging. - if (!(await this._isRevisionAheadOfBase(beforeStagingSha, versionBumpCommitSha, 1))) { - Log.error(` ✘ Unexpected additional commits have landed while staging the release.`); - Log.error(' Please revert the bump commit and retry, or cut a new version on top.'); - throw new FatalReleaseActionError(); - } + const releaseSha = await this._getAndValidateLatestCommitForPublishing( + publishBranch, + releaseNotes.version, + beforeStagingSha, + ); // Before publishing, we want to ensure that the locally-built packages we // built in the staging phase have not been modified accidentally. @@ -729,7 +700,7 @@ export abstract class ReleaseAction { // Create a Github release for the new version. await this._createGithubReleaseForVersion( releaseNotes, - versionBumpCommitSha, + releaseSha, npmDistTag === 'next', additionalOptions.showAsLatestOnGitHub, ); @@ -759,13 +730,45 @@ export abstract class ReleaseAction { } } - /** Checks whether the given commit represents a staging commit for the specified version. */ - private async _isCommitForVersionStaging(version: semver.SemVer, commitSha: string) { - const {data} = await this.git.github.repos.getCommit({ - ...this.git.remoteParams, - ref: commitSha, - }); - return data.commit.message.startsWith(getCommitMessageForRelease(version)); + /** + * Retreive the latest commit from the provided branch, and verify that it is the expected + * release commit and is the direct child of the previous sha provided. + * + * The method will make one recursive attempt to check again before throwing an error if + * any error occurs during this validation. + */ + private async _getAndValidateLatestCommitForPublishing( + branch: string, + version: semver.SemVer, + previousSha: string, + isRetry = false, + ): Promise { + try { + const commit = await this.getLatestCommitOfBranch(branch); + // Ensure the latest commit in the publish branch is the bump commit. + if (!commit.commit.message.startsWith(getCommitMessageForRelease(version))) { + /** The shortened sha of the commit for usage in the error message. */ + const sha = commit.sha.slice(0, 8); + Log.error(` ✘ Latest commit (${sha}) in "${branch}" branch is not a staging commit.`); + Log.error(' Please make sure the staging pull request has been merged.'); + throw new FatalReleaseActionError(); + } + + // We only inspect the first parent as we enforce that no merge commits are used in our + // repos, so all commits have exactly one parent. + if (commit.parents[0].sha !== previousSha) { + Log.error(` ✘ Unexpected additional commits have landed while staging the release.`); + Log.error(' Please revert the bump commit and retry, or cut a new version on top.'); + throw new FatalReleaseActionError(); + } + + return commit.sha; + } catch (e: unknown) { + if (isRetry) { + throw e; + } + return this._getAndValidateLatestCommitForPublishing(branch, version, previousSha, true); + } } // TODO: Remove this check and run it as part of common release validation. diff --git a/ng-dev/release/publish/actions/configure-next-as-major.ts b/ng-dev/release/publish/actions/configure-next-as-major.ts index 048920ac1..d829f239f 100644 --- a/ng-dev/release/publish/actions/configure-next-as-major.ts +++ b/ng-dev/release/publish/actions/configure-next-as-major.ts @@ -33,7 +33,7 @@ export class ConfigureNextAsMajorAction extends ReleaseAction { override async perform() { const {branchName} = this.active.next; const newVersion = this._newVersion; - const beforeStagingSha = await this.getLatestCommitOfBranch(branchName); + const {sha: beforeStagingSha} = await this.getLatestCommitOfBranch(branchName); await this.assertPassingGithubStatus(beforeStagingSha, branchName); await this.checkoutUpstreamBranch(branchName); diff --git a/ng-dev/release/publish/actions/exceptional-minor/prepare-exceptional-minor.ts b/ng-dev/release/publish/actions/exceptional-minor/prepare-exceptional-minor.ts index 78bd688ad..69119ad25 100644 --- a/ng-dev/release/publish/actions/exceptional-minor/prepare-exceptional-minor.ts +++ b/ng-dev/release/publish/actions/exceptional-minor/prepare-exceptional-minor.ts @@ -39,7 +39,7 @@ export class PrepareExceptionalMinorAction extends ReleaseAction { } async perform(): Promise { - const latestBaseBranchSha = await this.getLatestCommitOfBranch(this._baseBranch); + const {sha: latestBaseBranchSha} = await this.getLatestCommitOfBranch(this._baseBranch); await this.assertPassingGithubStatus(latestBaseBranchSha, this._baseBranch); diff --git a/ng-dev/release/publish/actions/shared/branch-off-next-branch.ts b/ng-dev/release/publish/actions/shared/branch-off-next-branch.ts index 87132e8c1..09a4286ad 100644 --- a/ng-dev/release/publish/actions/shared/branch-off-next-branch.ts +++ b/ng-dev/release/publish/actions/shared/branch-off-next-branch.ts @@ -59,7 +59,7 @@ export abstract class BranchOffNextBranchBaseAction extends CutNpmNextPrerelease const compareVersionForReleaseNotes = await this._computeReleaseNoteCompareVersion(); const newVersion = await this._computeNewVersion(); const newBranch = `${newVersion.major}.${newVersion.minor}.x`; - const beforeStagingSha = await this.getLatestCommitOfBranch(nextBranchName); + const {sha: beforeStagingSha} = await this.getLatestCommitOfBranch(nextBranchName); // Verify the current next branch has a passing status, before we branch off. await this.assertPassingGithubStatus(beforeStagingSha, nextBranchName); diff --git a/ng-dev/release/publish/test/branch-off-next-branch-testing.ts b/ng-dev/release/publish/test/branch-off-next-branch-testing.ts index 18a418782..5dc45a881 100644 --- a/ng-dev/release/publish/test/branch-off-next-branch-testing.ts +++ b/ng-dev/release/publish/test/branch-off-next-branch-testing.ts @@ -36,20 +36,16 @@ async function expectGithubApiRequestsForBranchOff( // We first mock the commit status check for the next branch, then expect two pull // requests from a fork that are targeting next and the new feature-freeze branch. repo - .expectBranchRequest('master', 'PRE_STAGING_SHA') + .expectBranchRequest('master', {sha: 'PRE_STAGING_SHA'}) .expectCommitStatusCheck('PRE_STAGING_SHA', 'success') .expectFindForkRequest(fork) .expectPullRequestToBeCreated(expectedNewBranch, fork, expectedStagingForkBranch, 200) .expectPullRequestMergeCheck(200, false) .expectPullRequestMerge(200) - .expectBranchRequest(expectedNewBranch, 'STAGING_COMMIT_SHA') - .expectCommitRequest( - 'STAGING_COMMIT_SHA', - `release: cut the v${expectedVersion} release\n\nPR Close #200.`, - ) - .expectCommitCompareRequest('PRE_STAGING_SHA', 'STAGING_COMMIT_SHA', { - status: 'ahead', - ahead_by: 1, + .expectBranchRequest(expectedNewBranch, { + sha: 'STAGING_COMMIT_SHA', + parents: [{sha: 'PRE_STAGING_SHA'}], + commit: {message: `release: cut the v${expectedVersion} release\n\nPR Close #200.`}, }) .expectTagToBeCreated(expectedTagName, 'STAGING_COMMIT_SHA') .expectReleaseToBeCreated( @@ -64,9 +60,7 @@ async function expectGithubApiRequestsForBranchOff( // In the fork, we make the following branches appear as non-existent, // so that the PRs can be created properly without collisions. - fork - .expectBranchRequest(expectedStagingForkBranch, null) - .expectBranchRequest(expectedNextUpdateBranch, null); + fork.expectBranchRequest(expectedStagingForkBranch).expectBranchRequest(expectedNextUpdateBranch); return {expectedNextUpdateBranch, expectedStagingForkBranch, expectedTagName}; } diff --git a/ng-dev/release/publish/test/common.spec.ts b/ng-dev/release/publish/test/common.spec.ts index 1c3f27773..f4d86faa0 100644 --- a/ng-dev/release/publish/test/common.spec.ts +++ b/ng-dev/release/publish/test/common.spec.ts @@ -185,11 +185,10 @@ describe('common release action logic', () => { const customRegistryUrl = 'https://custom-npm-registry.google.com'; repo - .expectBranchRequest(branchName, 'STAGING_SHA') - .expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`) - .expectCommitCompareRequest('BEFORE_STAGING_SHA', 'STAGING_SHA', { - status: 'ahead', - ahead_by: 1, + .expectBranchRequest(branchName, { + sha: 'STAGING_SHA', + parents: [{sha: 'BEFORE_STAGING_SHA'}], + commit: {message: `release: cut the v${version} release`}, }) .expectTagToBeCreated(tagName, 'STAGING_SHA') .expectReleaseToBeCreated(version.toString(), tagName, true); @@ -231,11 +230,10 @@ describe('common release action logic', () => { .commit('feat(test): second commit'); repo - .expectBranchRequest(branchName, 'STAGING_SHA') - .expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`) - .expectCommitCompareRequest('BEFORE_STAGING_SHA', 'STAGING_SHA', { - status: 'ahead', - ahead_by: 1, + .expectBranchRequest(branchName, { + sha: 'STAGING_SHA', + parents: [{sha: 'BEFORE_STAGING_SHA'}], + commit: {message: `release: cut the v${version} release`}, }) .expectTagToBeCreated(tagName, 'STAGING_SHA') .expectReleaseToBeCreated( @@ -285,13 +283,15 @@ describe('common release action logic', () => { git.commit('feat(test): first commit'); repo - .expectBranchRequest(branchName, 'STAGING_SHA') - .expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`) + .expectBranchRequest(branchName, { + sha: 'STAGING_SHA', + commit: {message: `release: cut the v${version} release`}, + }) .expectCommitStatusCheck('STAGING_SHA', 'success') .expectFindForkRequest(fork) .expectPullRequestToBeCreated(branchName, fork, 'release-stage-10.1.0-next.0', 10); - fork.expectBranchRequest('release-stage-10.1.0-next.0', null); + fork.expectBranchRequest('release-stage-10.1.0-next.0'); const stagingPromise = instance.testStagingWithBuild( version, @@ -334,11 +334,10 @@ describe('common release action logic', () => { ); repo - .expectBranchRequest(branchName, 'STAGING_SHA') - .expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`) - .expectCommitCompareRequest('BEFORE_STAGING_SHA', 'STAGING_SHA', { - status: 'ahead', - ahead_by: 1, + .expectBranchRequest(branchName, { + sha: 'STAGING_SHA', + parents: [{sha: 'BEFORE_STAGING_SHA'}], + commit: {message: `release: cut the v${version} release`}, }) .expectTagToBeCreated(tagName, 'STAGING_SHA') .expectReleaseToBeCreated( @@ -366,13 +365,11 @@ describe('common release action logic', () => { ); const {version, branchName} = baseReleaseTrains.latest; - repo - .expectBranchRequest(branchName, 'STAGING_SHA') - .expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`) - .expectCommitCompareRequest('BEFORE_STAGING_SHA', 'STAGING_SHA', { - status: 'ahead', - ahead_by: 2, // this implies that another unknown/new commit is in between. - }); + repo.expectBranchRequest(branchName, { + sha: 'STAGING_SHA', + parents: [{sha: 'THE_ONE_BEFORE_STAGING_SHA'}], + commit: {message: `release: cut the v${version} release`}, + }); spyOn(Log, 'error'); @@ -458,7 +455,7 @@ describe('common release action logic', () => { .expectPullRequestMerge(200); // Simulate that the fork branch name is available. - fork.expectBranchRequest(forkBranchName, null); + fork.expectBranchRequest(forkBranchName); await instance.testCherryPickWithPullRequest(version, branchName); @@ -488,7 +485,7 @@ describe('common release action logic', () => { .expectPullRequestMergeCheck(200, true); // Simulate that the fork branch name is available. - fork.expectBranchRequest(forkBranchName, null); + fork.expectBranchRequest(forkBranchName); await instance.testCherryPickWithPullRequest(version, branchName); @@ -510,7 +507,7 @@ describe('common release action logic', () => { .expectPullRequestMerge(200); // Simulate that the fork branch name is available. - fork.expectBranchRequest(forkBranchName, null); + fork.expectBranchRequest(forkBranchName); await instance.testCherryPickWithPullRequest(version, branchName); diff --git a/ng-dev/release/publish/test/configure-next-as-major.spec.ts b/ng-dev/release/publish/test/configure-next-as-major.spec.ts index b8814e390..2bfb4f9f4 100644 --- a/ng-dev/release/publish/test/configure-next-as-major.spec.ts +++ b/ng-dev/release/publish/test/configure-next-as-major.spec.ts @@ -97,7 +97,7 @@ describe('configure next as major action', () => { // We first mock the commit status check for the next branch, then expect two pull // requests from a fork that are targeting next and the new feature-freeze branch. repo - .expectBranchRequest('master', 'MASTER_COMMIT_SHA') + .expectBranchRequest('master', {sha: 'MASTER_COMMIT_SHA'}) .expectCommitStatusCheck('MASTER_COMMIT_SHA', 'success') .expectFindForkRequest(fork) .expectPullRequestToBeCreated('master', fork, expectedForkBranch, 200) @@ -106,7 +106,7 @@ describe('configure next as major action', () => { // In the fork, we make the staging branch appear as non-existent, // so that the PR can be created properly without collisions. - fork.expectBranchRequest(expectedForkBranch, null); + fork.expectBranchRequest(expectedForkBranch); await action.instance.perform(); diff --git a/ng-dev/release/publish/test/exceptional-minor/prepare-exceptional-minor.spec.ts b/ng-dev/release/publish/test/exceptional-minor/prepare-exceptional-minor.spec.ts index 60186dfc4..fa95b3d03 100644 --- a/ng-dev/release/publish/test/exceptional-minor/prepare-exceptional-minor.spec.ts +++ b/ng-dev/release/publish/test/exceptional-minor/prepare-exceptional-minor.spec.ts @@ -86,7 +86,7 @@ describe('cut exceptional minor next release candidate action', () => { ); repo - .expectBranchRequest('14.2.x', 'PATCH_LATEST_SHA') + .expectBranchRequest('14.2.x', {sha: 'PATCH_LATEST_SHA'}) .expectCommitStatusCheck('PATCH_LATEST_SHA', 'success'); await instance.perform(); @@ -119,7 +119,7 @@ describe('cut exceptional minor next release candidate action', () => { ); repo - .expectBranchRequest('14.2.x', 'PATCH_LATEST_SHA') + .expectBranchRequest('14.2.x', {sha: 'PATCH_LATEST_SHA'}) .expectCommitStatusCheck('PATCH_LATEST_SHA', 'success'); await instance.perform(); @@ -152,7 +152,7 @@ describe('cut exceptional minor next release candidate action', () => { ); repo - .expectBranchRequest('14.2.x', 'PATCH_LATEST_SHA') + .expectBranchRequest('14.2.x', {sha: 'PATCH_LATEST_SHA'}) .expectCommitStatusCheck('PATCH_LATEST_SHA', 'success'); await instance.perform(); @@ -185,7 +185,7 @@ describe('cut exceptional minor next release candidate action', () => { ); repo - .expectBranchRequest('14.2.x', 'PATCH_LATEST_SHA') + .expectBranchRequest('14.2.x', {sha: 'PATCH_LATEST_SHA'}) .expectCommitStatusCheck('PATCH_LATEST_SHA', 'success'); await instance.perform(); diff --git a/ng-dev/release/publish/test/test-utils/staging-test.ts b/ng-dev/release/publish/test/test-utils/staging-test.ts index 33a119418..b13cb63c5 100644 --- a/ng-dev/release/publish/test/test-utils/staging-test.ts +++ b/ng-dev/release/publish/test/test-utils/staging-test.ts @@ -33,7 +33,7 @@ export async function expectGithubApiRequestsForStaging( // We first mock the commit status check for the next branch, then expect two pull // requests from a fork that are targeting next and the new feature-freeze branch. repo - .expectBranchRequest(expectedBranch, 'PRE_STAGING_SHA') + .expectBranchRequest(expectedBranch, {sha: 'PRE_STAGING_SHA'}) .expectCommitStatusCheck('PRE_STAGING_SHA', 'success') .expectFindForkRequest(fork) .expectPullRequestToBeCreated(expectedBranch, fork, expectedStagingForkBranch, 200) @@ -42,7 +42,7 @@ export async function expectGithubApiRequestsForStaging( // In the fork, we make the staging branch appear as non-existent, // so that the PR can be created properly without collisions. - fork.expectBranchRequest(expectedStagingForkBranch, null); + fork.expectBranchRequest(expectedStagingForkBranch); if (opts.withCherryPicking) { const expectedCherryPickForkBranch = `changelog-cherry-pick-${expectedVersion}`; @@ -54,7 +54,7 @@ export async function expectGithubApiRequestsForStaging( // In the fork, make the cherry-pick branch appear as non-existent, so that the // cherry-pick PR can be created properly without collisions. - fork.expectBranchRequest(expectedCherryPickForkBranch, null); + fork.expectBranchRequest(expectedCherryPickForkBranch); } } @@ -75,14 +75,10 @@ export async function expectGithubApiRequests( await expectGithubApiRequestsForStaging(action, expectedBranch, expectedVersion, opts); repo - .expectBranchRequest(expectedBranch, 'STAGING_COMMIT_SHA') - .expectCommitRequest( - 'STAGING_COMMIT_SHA', - `release: cut the v${expectedVersion} release\n\nPR Close #200.`, - ) - .expectCommitCompareRequest('PRE_STAGING_SHA', 'STAGING_COMMIT_SHA', { - status: 'ahead', - ahead_by: 1, + .expectBranchRequest(expectedBranch, { + sha: 'STAGING_COMMIT_SHA', + parents: [{sha: 'PRE_STAGING_SHA'}], + commit: {message: `release: cut the v${expectedVersion} release\n\nPR Close #200.`}, }) .expectTagToBeCreated(expectedTagName, 'STAGING_COMMIT_SHA') .expectReleaseToBeCreated(expectedVersion, expectedTagName, opts.willShowAsLatestOnGitHub); diff --git a/ng-dev/utils/BUILD.bazel b/ng-dev/utils/BUILD.bazel index 9c1993db8..7153590a2 100644 --- a/ng-dev/utils/BUILD.bazel +++ b/ng-dev/utils/BUILD.bazel @@ -48,6 +48,7 @@ ts_library( "@npm//@octokit/auth-app", "@npm//@octokit/core", "@npm//@octokit/graphql", + "@npm//@octokit/openapi-types", "@npm//@octokit/plugin-paginate-rest", "@npm//@octokit/plugin-rest-endpoint-methods", "@npm//@octokit/request-error", diff --git a/ng-dev/utils/git/octokit-types.ts b/ng-dev/utils/git/octokit-types.ts new file mode 100644 index 000000000..cb85ca612 --- /dev/null +++ b/ng-dev/utils/git/octokit-types.ts @@ -0,0 +1,11 @@ +import {components} from '@octokit/openapi-types'; + +type DeepPartial = T extends object + ? { + [P in keyof T]?: DeepPartial; + } + : T; + +export type Commit = components['schemas']['commit']; + +export type PartialCommit = DeepPartial; diff --git a/ng-dev/utils/testing/github-api-testing.ts b/ng-dev/utils/testing/github-api-testing.ts index aad3e29c9..3f8f302d6 100644 --- a/ng-dev/utils/testing/github-api-testing.ts +++ b/ng-dev/utils/testing/github-api-testing.ts @@ -8,6 +8,7 @@ import {RestEndpointMethodTypes} from '@octokit/plugin-rest-endpoint-methods'; import nock from 'nock'; +import {PartialCommit} from '../git/octokit-types.js'; /** Type describing the parameters for a Github release update API request. */ type ReleaseUpdateParameters = RestEndpointMethodTypes['repos']['updateRelease']['parameters']; @@ -44,10 +45,10 @@ export class GithubTestingRepo { return this; } - expectBranchRequest(branchName: string, sha: string | null): this { + expectBranchRequest(branchName: string, commit?: PartialCommit): this { nock(this.repoApiUrl) .get(`/branches/${branchName}`) - .reply(sha ? 200 : 404, sha ? {commit: {sha}} : undefined); + .reply(commit ? 200 : 404, {commit: commit}); return this; } @@ -101,11 +102,6 @@ export class GithubTestingRepo { return this; } - expectCommitRequest(sha: string, message: string): this { - nock(this.repoApiUrl).get(`/commits/${sha}`).reply(200, {commit: {message}}); - return this; - } - expectCommitCompareRequest( baseRevision: string, headRevision: string,