Skip to content

Commit c4207bf

Browse files
josephperrottjosephperrott
committed
fix(ng-dev): allow for a retry during the validation that the commit sha being released is the expected commit (#2750)
PR Close #2750
1 parent dcd6553 commit c4207bf

File tree

8 files changed

+100
-107
lines changed

8 files changed

+100
-107
lines changed

‎ng-dev/release/publish/actions.ts

Lines changed: 45 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -158,23 +158,6 @@ export abstract class ReleaseAction {
158158
return commit;
159159
}
160160

161-
/** Checks whether the given revision is ahead to the base by the specified amount. */
162-
private async _isRevisionAheadOfBase(
163-
baseRevision: string,
164-
targetRevision: string,
165-
expectedAheadCount: number,
166-
) {
167-
const {
168-
data: {ahead_by, status},
169-
} = await this.git.github.repos.compareCommits({
170-
...this.git.remoteParams,
171-
base: baseRevision,
172-
head: targetRevision,
173-
});
174-
175-
return status === 'ahead' && ahead_by === expectedAheadCount;
176-
}
177-
178161
/**
179162
* Verifies that the given commit has passing all statuses.
180163
*
@@ -704,24 +687,11 @@ export abstract class ReleaseAction {
704687
npmDistTag: NpmDistTag,
705688
additionalOptions: {showAsLatestOnGitHub: boolean},
706689
) {
707-
const {sha: versionBumpCommitSha} = await this.getLatestCommitOfBranch(publishBranch);
708-
709-
// Ensure the latest commit in the publish branch is the bump commit.
710-
if (!(await this._isCommitForVersionStaging(releaseNotes.version, versionBumpCommitSha))) {
711-
Log.error(` ✘ Latest commit in "${publishBranch}" branch is not a staging commit.`);
712-
Log.error(' Please make sure the staging pull request has been merged.');
713-
throw new FatalReleaseActionError();
714-
}
715-
716-
// Ensure no commits have landed since we started the staging process. This would signify
717-
// that the locally-built release packages are not matching with the release commit on GitHub.
718-
// Note: We expect the version bump commit to be ahead by **one** commit. This means it's
719-
// the direct parent of the commit that was latest when we started the staging.
720-
if (!(await this._isRevisionAheadOfBase(beforeStagingSha, versionBumpCommitSha, 1))) {
721-
Log.error(` ✘ Unexpected additional commits have landed while staging the release.`);
722-
Log.error(' Please revert the bump commit and retry, or cut a new version on top.');
723-
throw new FatalReleaseActionError();
724-
}
690+
const releaseSha = await this._getAndValidateLatestCommitForPublishing(
691+
publishBranch,
692+
releaseNotes.version,
693+
beforeStagingSha,
694+
);
725695

726696
// Before publishing, we want to ensure that the locally-built packages we
727697
// built in the staging phase have not been modified accidentally.
@@ -730,7 +700,7 @@ export abstract class ReleaseAction {
730700
// Create a Github release for the new version.
731701
await this._createGithubReleaseForVersion(
732702
releaseNotes,
733-
versionBumpCommitSha,
703+
releaseSha,
734704
npmDistTag === 'next',
735705
additionalOptions.showAsLatestOnGitHub,
736706
);
@@ -760,13 +730,45 @@ export abstract class ReleaseAction {
760730
}
761731
}
762732

763-
/** Checks whether the given commit represents a staging commit for the specified version. */
764-
private async _isCommitForVersionStaging(version: semver.SemVer, commitSha: string) {
765-
const {data} = await this.git.github.repos.getCommit({
766-
...this.git.remoteParams,
767-
ref: commitSha,
768-
});
769-
return data.commit.message.startsWith(getCommitMessageForRelease(version));
733+
/**
734+
* Retreive the latest commit from the provided branch, and verify that it is the expected
735+
* release commit and is the direct child of the previous sha provided.
736+
*
737+
* The method will make one recursive attempt to check again before throwing an error if
738+
* any error occurs during this validation.
739+
*/
740+
private async _getAndValidateLatestCommitForPublishing(
741+
branch: string,
742+
version: semver.SemVer,
743+
previousSha: string,
744+
isRetry = false,
745+
): Promise<string> {
746+
try {
747+
const commit = await this.getLatestCommitOfBranch(branch);
748+
// Ensure the latest commit in the publish branch is the bump commit.
749+
if (!commit.commit.message.startsWith(getCommitMessageForRelease(version))) {
750+
/** The shortened sha of the commit for usage in the error message. */
751+
const sha = commit.sha.slice(0, 8);
752+
Log.error(` ✘ Latest commit (${sha}) in "${branch}" branch is not a staging commit.`);
753+
Log.error(' Please make sure the staging pull request has been merged.');
754+
throw new FatalReleaseActionError();
755+
}
756+
757+
// We only inspect the first parent as we enforce that no merge commits are used in our
758+
// repos, so all commits have exactly one parent.
759+
if (commit.parents[0].sha !== previousSha) {
760+
Log.error(` ✘ Unexpected additional commits have landed while staging the release.`);
761+
Log.error(' Please revert the bump commit and retry, or cut a new version on top.');
762+
throw new FatalReleaseActionError();
763+
}
764+
765+
return commit.sha;
766+
} catch (e: unknown) {
767+
if (isRetry) {
768+
throw e;
769+
}
770+
return this._getAndValidateLatestCommitForPublishing(branch, version, previousSha, true);
771+
}
770772
}
771773

772774
// TODO: Remove this check and run it as part of common release validation.

‎ng-dev/release/publish/test/branch-off-next-branch-testing.ts

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -36,20 +36,16 @@ async function expectGithubApiRequestsForBranchOff(
3636
// We first mock the commit status check for the next branch, then expect two pull
3737
// requests from a fork that are targeting next and the new feature-freeze branch.
3838
repo
39-
.expectBranchRequest('master', 'PRE_STAGING_SHA')
39+
.expectBranchRequest('master', {sha: 'PRE_STAGING_SHA'})
4040
.expectCommitStatusCheck('PRE_STAGING_SHA', 'success')
4141
.expectFindForkRequest(fork)
4242
.expectPullRequestToBeCreated(expectedNewBranch, fork, expectedStagingForkBranch, 200)
4343
.expectPullRequestMergeCheck(200, false)
4444
.expectPullRequestMerge(200)
45-
.expectBranchRequest(expectedNewBranch, 'STAGING_COMMIT_SHA')
46-
.expectCommitRequest(
47-
'STAGING_COMMIT_SHA',
48-
`release: cut the v${expectedVersion} release\n\nPR Close #200.`,
49-
)
50-
.expectCommitCompareRequest('PRE_STAGING_SHA', 'STAGING_COMMIT_SHA', {
51-
status: 'ahead',
52-
ahead_by: 1,
45+
.expectBranchRequest(expectedNewBranch, {
46+
sha: 'STAGING_COMMIT_SHA',
47+
parents: [{sha: 'PRE_STAGING_SHA'}],
48+
commit: {message: `release: cut the v${expectedVersion} release\n\nPR Close #200.`},
5349
})
5450
.expectTagToBeCreated(expectedTagName, 'STAGING_COMMIT_SHA')
5551
.expectReleaseToBeCreated(
@@ -64,9 +60,7 @@ async function expectGithubApiRequestsForBranchOff(
6460

6561
// In the fork, we make the following branches appear as non-existent,
6662
// so that the PRs can be created properly without collisions.
67-
fork
68-
.expectBranchRequest(expectedStagingForkBranch, null)
69-
.expectBranchRequest(expectedNextUpdateBranch, null);
63+
fork.expectBranchRequest(expectedStagingForkBranch).expectBranchRequest(expectedNextUpdateBranch);
7064

7165
return {expectedNextUpdateBranch, expectedStagingForkBranch, expectedTagName};
7266
}

‎ng-dev/release/publish/test/common.spec.ts

Lines changed: 25 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -185,11 +185,10 @@ describe('common release action logic', () => {
185185
const customRegistryUrl = 'https://custom-npm-registry.google.com';
186186

187187
repo
188-
.expectBranchRequest(branchName, 'STAGING_SHA')
189-
.expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`)
190-
.expectCommitCompareRequest('BEFORE_STAGING_SHA', 'STAGING_SHA', {
191-
status: 'ahead',
192-
ahead_by: 1,
188+
.expectBranchRequest(branchName, {
189+
sha: 'STAGING_SHA',
190+
parents: [{sha: 'BEFORE_STAGING_SHA'}],
191+
commit: {message: `release: cut the v${version} release`},
193192
})
194193
.expectTagToBeCreated(tagName, 'STAGING_SHA')
195194
.expectReleaseToBeCreated(version.toString(), tagName, true);
@@ -231,11 +230,10 @@ describe('common release action logic', () => {
231230
.commit('feat(test): second commit');
232231

233232
repo
234-
.expectBranchRequest(branchName, 'STAGING_SHA')
235-
.expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`)
236-
.expectCommitCompareRequest('BEFORE_STAGING_SHA', 'STAGING_SHA', {
237-
status: 'ahead',
238-
ahead_by: 1,
233+
.expectBranchRequest(branchName, {
234+
sha: 'STAGING_SHA',
235+
parents: [{sha: 'BEFORE_STAGING_SHA'}],
236+
commit: {message: `release: cut the v${version} release`},
239237
})
240238
.expectTagToBeCreated(tagName, 'STAGING_SHA')
241239
.expectReleaseToBeCreated(
@@ -285,13 +283,15 @@ describe('common release action logic', () => {
285283
git.commit('feat(test): first commit');
286284

287285
repo
288-
.expectBranchRequest(branchName, 'STAGING_SHA')
289-
.expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`)
286+
.expectBranchRequest(branchName, {
287+
sha: 'STAGING_SHA',
288+
commit: {message: `release: cut the v${version} release`},
289+
})
290290
.expectCommitStatusCheck('STAGING_SHA', 'success')
291291
.expectFindForkRequest(fork)
292292
.expectPullRequestToBeCreated(branchName, fork, 'release-stage-10.1.0-next.0', 10);
293293

294-
fork.expectBranchRequest('release-stage-10.1.0-next.0', null);
294+
fork.expectBranchRequest('release-stage-10.1.0-next.0');
295295

296296
const stagingPromise = instance.testStagingWithBuild(
297297
version,
@@ -334,11 +334,10 @@ describe('common release action logic', () => {
334334
);
335335

336336
repo
337-
.expectBranchRequest(branchName, 'STAGING_SHA')
338-
.expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`)
339-
.expectCommitCompareRequest('BEFORE_STAGING_SHA', 'STAGING_SHA', {
340-
status: 'ahead',
341-
ahead_by: 1,
337+
.expectBranchRequest(branchName, {
338+
sha: 'STAGING_SHA',
339+
parents: [{sha: 'BEFORE_STAGING_SHA'}],
340+
commit: {message: `release: cut the v${version} release`},
342341
})
343342
.expectTagToBeCreated(tagName, 'STAGING_SHA')
344343
.expectReleaseToBeCreated(
@@ -366,13 +365,11 @@ describe('common release action logic', () => {
366365
);
367366
const {version, branchName} = baseReleaseTrains.latest;
368367

369-
repo
370-
.expectBranchRequest(branchName, 'STAGING_SHA')
371-
.expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`)
372-
.expectCommitCompareRequest('BEFORE_STAGING_SHA', 'STAGING_SHA', {
373-
status: 'ahead',
374-
ahead_by: 2, // this implies that another unknown/new commit is in between.
375-
});
368+
repo.expectBranchRequest(branchName, {
369+
sha: 'STAGING_SHA',
370+
parents: [{sha: 'THE_ONE_BEFORE_STAGING_SHA'}],
371+
commit: {message: `release: cut the v${version} release`},
372+
});
376373

377374
spyOn(Log, 'error');
378375

@@ -458,7 +455,7 @@ describe('common release action logic', () => {
458455
.expectPullRequestMerge(200);
459456

460457
// Simulate that the fork branch name is available.
461-
fork.expectBranchRequest(forkBranchName, null);
458+
fork.expectBranchRequest(forkBranchName);
462459

463460
await instance.testCherryPickWithPullRequest(version, branchName);
464461

@@ -488,7 +485,7 @@ describe('common release action logic', () => {
488485
.expectPullRequestMergeCheck(200, true);
489486

490487
// Simulate that the fork branch name is available.
491-
fork.expectBranchRequest(forkBranchName, null);
488+
fork.expectBranchRequest(forkBranchName);
492489

493490
await instance.testCherryPickWithPullRequest(version, branchName);
494491

@@ -510,7 +507,7 @@ describe('common release action logic', () => {
510507
.expectPullRequestMerge(200);
511508

512509
// Simulate that the fork branch name is available.
513-
fork.expectBranchRequest(forkBranchName, null);
510+
fork.expectBranchRequest(forkBranchName);
514511

515512
await instance.testCherryPickWithPullRequest(version, branchName);
516513

‎ng-dev/release/publish/test/configure-next-as-major.spec.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ describe('configure next as major action', () => {
9797
// We first mock the commit status check for the next branch, then expect two pull
9898
// requests from a fork that are targeting next and the new feature-freeze branch.
9999
repo
100-
.expectBranchRequest('master', 'MASTER_COMMIT_SHA')
100+
.expectBranchRequest('master', {sha: 'MASTER_COMMIT_SHA'})
101101
.expectCommitStatusCheck('MASTER_COMMIT_SHA', 'success')
102102
.expectFindForkRequest(fork)
103103
.expectPullRequestToBeCreated('master', fork, expectedForkBranch, 200)
@@ -106,7 +106,7 @@ describe('configure next as major action', () => {
106106

107107
// In the fork, we make the staging branch appear as non-existent,
108108
// so that the PR can be created properly without collisions.
109-
fork.expectBranchRequest(expectedForkBranch, null);
109+
fork.expectBranchRequest(expectedForkBranch);
110110

111111
await action.instance.perform();
112112

‎ng-dev/release/publish/test/exceptional-minor/prepare-exceptional-minor.spec.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ describe('cut exceptional minor next release candidate action', () => {
8686
);
8787

8888
repo
89-
.expectBranchRequest('14.2.x', 'PATCH_LATEST_SHA')
89+
.expectBranchRequest('14.2.x', {sha: 'PATCH_LATEST_SHA'})
9090
.expectCommitStatusCheck('PATCH_LATEST_SHA', 'success');
9191

9292
await instance.perform();
@@ -119,7 +119,7 @@ describe('cut exceptional minor next release candidate action', () => {
119119
);
120120

121121
repo
122-
.expectBranchRequest('14.2.x', 'PATCH_LATEST_SHA')
122+
.expectBranchRequest('14.2.x', {sha: 'PATCH_LATEST_SHA'})
123123
.expectCommitStatusCheck('PATCH_LATEST_SHA', 'success');
124124

125125
await instance.perform();
@@ -152,7 +152,7 @@ describe('cut exceptional minor next release candidate action', () => {
152152
);
153153

154154
repo
155-
.expectBranchRequest('14.2.x', 'PATCH_LATEST_SHA')
155+
.expectBranchRequest('14.2.x', {sha: 'PATCH_LATEST_SHA'})
156156
.expectCommitStatusCheck('PATCH_LATEST_SHA', 'success');
157157

158158
await instance.perform();
@@ -185,7 +185,7 @@ describe('cut exceptional minor next release candidate action', () => {
185185
);
186186

187187
repo
188-
.expectBranchRequest('14.2.x', 'PATCH_LATEST_SHA')
188+
.expectBranchRequest('14.2.x', {sha: 'PATCH_LATEST_SHA'})
189189
.expectCommitStatusCheck('PATCH_LATEST_SHA', 'success');
190190

191191
await instance.perform();

‎ng-dev/release/publish/test/test-utils/staging-test.ts

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ export async function expectGithubApiRequestsForStaging(
3333
// We first mock the commit status check for the next branch, then expect two pull
3434
// requests from a fork that are targeting next and the new feature-freeze branch.
3535
repo
36-
.expectBranchRequest(expectedBranch, 'PRE_STAGING_SHA')
36+
.expectBranchRequest(expectedBranch, {sha: 'PRE_STAGING_SHA'})
3737
.expectCommitStatusCheck('PRE_STAGING_SHA', 'success')
3838
.expectFindForkRequest(fork)
3939
.expectPullRequestToBeCreated(expectedBranch, fork, expectedStagingForkBranch, 200)
@@ -42,7 +42,7 @@ export async function expectGithubApiRequestsForStaging(
4242

4343
// In the fork, we make the staging branch appear as non-existent,
4444
// so that the PR can be created properly without collisions.
45-
fork.expectBranchRequest(expectedStagingForkBranch, null);
45+
fork.expectBranchRequest(expectedStagingForkBranch);
4646

4747
if (opts.withCherryPicking) {
4848
const expectedCherryPickForkBranch = `changelog-cherry-pick-${expectedVersion}`;
@@ -54,7 +54,7 @@ export async function expectGithubApiRequestsForStaging(
5454

5555
// In the fork, make the cherry-pick branch appear as non-existent, so that the
5656
// cherry-pick PR can be created properly without collisions.
57-
fork.expectBranchRequest(expectedCherryPickForkBranch, null);
57+
fork.expectBranchRequest(expectedCherryPickForkBranch);
5858
}
5959
}
6060

@@ -75,14 +75,10 @@ export async function expectGithubApiRequests(
7575
await expectGithubApiRequestsForStaging(action, expectedBranch, expectedVersion, opts);
7676

7777
repo
78-
.expectBranchRequest(expectedBranch, 'STAGING_COMMIT_SHA')
79-
.expectCommitRequest(
80-
'STAGING_COMMIT_SHA',
81-
`release: cut the v${expectedVersion} release\n\nPR Close #200.`,
82-
)
83-
.expectCommitCompareRequest('PRE_STAGING_SHA', 'STAGING_COMMIT_SHA', {
84-
status: 'ahead',
85-
ahead_by: 1,
78+
.expectBranchRequest(expectedBranch, {
79+
sha: 'STAGING_COMMIT_SHA',
80+
parents: [{sha: 'PRE_STAGING_SHA'}],
81+
commit: {message: `release: cut the v${expectedVersion} release\n\nPR Close #200.`},
8682
})
8783
.expectTagToBeCreated(expectedTagName, 'STAGING_COMMIT_SHA')
8884
.expectReleaseToBeCreated(expectedVersion, expectedTagName, opts.willShowAsLatestOnGitHub);

‎ng-dev/utils/git/octokit-types.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,11 @@
11
import {components} from '@octokit/openapi-types';
22

3+
type DeepPartial<T> = T extends object
4+
? {
5+
[P in keyof T]?: DeepPartial<T[P]>;
6+
}
7+
: T;
8+
39
export type Commit = components['schemas']['commit'];
10+
11+
export type PartialCommit = DeepPartial<Commit>;

0 commit comments

Comments
 (0)