Skip to content

Commit

Permalink
fix(ng-dev/release): avoid accidentally incorporating unexpected chan…
Browse files Browse the repository at this point in the history
…ges in release build

There is an interesting scenario where the caretaker may make unexpected
non-changelog changes as part of the short prompt where we allow manual
polishing to the changelog.

If the caretaker modified other files there, they will NOT be part of
the release cutting PR (and also shouldn't be) - but currently the
changes are not discarded/or detected and WILL influence the release
output building. This could cause subtle differences and confusion
because the release output is different than what we committed (because
the release cut commits will not incorporate the other non-changelog
changes).

The fix is to properly detect other modifications and abort properly.
Other changes should be made beforehand, go through review process and
should not be part of the release cut commit.
  • Loading branch information
devversion committed Sep 19, 2022
1 parent d3a9405 commit 5b747aa
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 5 deletions.
9 changes: 9 additions & 0 deletions ng-dev/release/publish/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,12 +195,21 @@ export abstract class ReleaseAction {

// Commit message for the release point.
const commitMessage = getCommitMessageForRelease(newVersion);

// Create a release staging commit including changelog and version bump.
await this.createCommit(commitMessage, [
workspaceRelativePackageJsonPath,
workspaceRelativeChangelogPath,
]);

// The caretaker may have attempted to make additional changes. These changes would
// not be captured into the release commit. The working directory should remain clean,
// like we assume it being clean when we start the release actions.
if (this.git.hasUncommittedChanges()) {
Log.error(' ✘ Unrelated changes have been made as part of the changelog editing.');
throw new FatalReleaseActionError();
}

Log.info(green(` ✓ Created release commit for: "${newVersion}".`));
}

Expand Down
54 changes: 52 additions & 2 deletions ng-dev/release/publish/test/common.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,11 @@
* found in the LICENSE file at https://angular.io/license
*/

import {readFileSync} from 'fs';
import {readFileSync, writeFileSync} from 'fs';
import {join} from 'path';
import semver from 'semver';

import {CommitFromGitLog, parseCommitFromGitLog} from '../../../commit-message/parse.js';
import {NgDevConfig} from '../../../utils/config.js';
import {GitClient} from '../../../utils/git/git-client.js';
import {Log} from '../../../utils/logging.js';
import {
Expand Down Expand Up @@ -194,6 +193,57 @@ describe('common release action logic', () => {
);
});

it('should not allow for arbitrary edits to be made during changelog edit prompt', async () => {
const {repo, fork, instance, githubConfig, promptConfirmSpy} = setupReleaseActionForTesting(
DelegateTestAction,
baseReleaseTrains,
/* isNextPublishedToNpm */ true,
{useSandboxGitClient: true},
);
const {version, branchName} = baseReleaseTrains.next;

spyOn(Log, 'error');

let promptResolveFn: ((value: boolean) => void) | null = null;
const promptPromise = new Promise<boolean>((resolve) => (promptResolveFn = resolve));
promptConfirmSpy.and.returnValue(promptPromise);

const testFile = join(testTmpDir, 'some-file.txt');
const git =
SandboxGitRepo.withInitialCommit(githubConfig).createTagForHead('0.0.0-compare-base');

writeFileSync(testFile, 'content');
git.commit('feat(test): first commit');

repo
.expectBranchRequest(branchName, 'STAGING_SHA')
.expectCommitRequest('STAGING_SHA', `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);

const stagingPromise = instance.testStagingWithBuild(
version,
branchName,
parse('0.0.0-compare-base'),
);

// Before confirming that we are good with the changelog changes, modify
// an unrelated file. This should trigger a release action fatal error.
writeFileSync(testFile, 'change content');
promptResolveFn!(true);

await expectAsync(stagingPromise).toBeRejected();

expect(Log.error).toHaveBeenCalledWith(
jasmine.stringContaining(
'Unrelated changes have been made as part of the changelog editing',
),
);
});

it('should link to the changelog in the release entry if notes are too large', async () => {
const {repo, instance, gitClient, builtPackagesWithInfo, releaseConfig} =
setupReleaseActionForTesting(DelegateTestAction, baseReleaseTrains);
Expand Down
4 changes: 2 additions & 2 deletions ng-dev/release/publish/test/test-utils/action-mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export function setupMocksForReleaseAction<T extends boolean>(
// Fake confirm any prompts. We do not want to make any changelog edits and
// just proceed with the release action. Also we immediately want to confirm
// when we are prompted whether the pull request should be merged.
spyOn(Prompt, 'confirm').and.resolveTo(true);
const promptConfirmSpy = spyOn(Prompt, 'confirm').and.resolveTo(true);

const builtPackagesWithInfo: BuiltPackageWithInfo[] = testReleasePackages.map((pkg) => ({
...pkg,
Expand Down Expand Up @@ -136,5 +136,5 @@ export function setupMocksForReleaseAction<T extends boolean>(
);
}

return {gitClient, builtPackagesWithInfo};
return {gitClient, builtPackagesWithInfo, promptConfirmSpy};
}
2 changes: 2 additions & 0 deletions ng-dev/release/publish/test/test-utils/test-action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

import {GithubConfig} from '../../../../utils/config.js';
import {Prompt} from '../../../../utils/prompt.js';
import {
GithubTestingRepo,
SandboxGitClient,
Expand Down Expand Up @@ -57,6 +58,7 @@ export interface TestReleaseAction<
projectDir: string;
githubConfig: GithubConfig;
releaseConfig: ReleaseConfig;
promptConfirmSpy: jasmine.Spy<typeof Prompt['confirm']>;
builtPackagesWithInfo: BuiltPackageWithInfo[];
gitClient: O['useSandboxGitClient'] extends true ? SandboxGitClient : VirtualGitClient;
}
3 changes: 2 additions & 1 deletion ng-dev/release/publish/test/test-utils/test-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export function setupReleaseActionForTesting<
});

// Setup mocks for release action.
const {gitClient, builtPackagesWithInfo} = setupMocksForReleaseAction<
const {gitClient, builtPackagesWithInfo, promptConfirmSpy} = setupMocksForReleaseAction<
OptionsWithDefaults['useSandboxGitClient']
>(
githubConfig,
Expand All @@ -81,6 +81,7 @@ export function setupReleaseActionForTesting<
githubConfig,
releaseConfig,
gitClient,
promptConfirmSpy,
builtPackagesWithInfo,
};
}
Expand Down

0 comments on commit 5b747aa

Please sign in to comment.