Skip to content

Commit 30def70

Browse files
committed
feat(ng-dev): add conditional autosquash merge strategy
Introduces a new conditional autosquash merge strategy. This strategy uses the autosquash merge strategy if the pull request contains fixup or squash commits, and the GitHub API merge strategy otherwise. This allows pull requests that do not need to be squashed to be closed as "merged" on GitHub, rather than "closed".
1 parent b02901c commit 30def70

File tree

5 files changed

+66
-29
lines changed

5 files changed

+66
-29
lines changed

.github/local-actions/branch-manager/main.js

Lines changed: 13 additions & 2 deletions
Large diffs are not rendered by default.

.ng-dev/pull-request.mts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@ import {PullRequestConfig} from '../ng-dev/pr/config/index.js';
22

33
/** Configuration for interacting with pull requests in the repo. */
44
export const pullRequest: PullRequestConfig = {
5-
githubApiMerge: false,
5+
githubApiMerge: {
6+
default: 'rebase-with-fixup',
7+
labels: [{pattern: 'merge: squash commits', method: 'squash'}],
8+
},
69
requiredStatuses: [{name: 'test', type: 'check'}],
710

811
// Disable target labeling in the dev-infra repo as we don't have

ng-dev/pr/config/index.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,14 @@
88

99
import {ConfigValidationError, GithubConfig, NgDevConfig} from '../../utils/config.js';
1010

11+
// TODO(alanagius): remove `rebase-with-fixup` and replace it's logic with `rebase`.
12+
// This is just temporary to allow testing in the dev-infra repo. Without breaking workflows in other repos.
13+
1114
/**
1215
* Possible merge methods supported by the Github API.
1316
* https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button.
1417
*/
15-
export type GithubApiMergeMethod = 'merge' | 'squash' | 'rebase';
18+
export type GithubApiMergeMethod = 'merge' | 'squash' | 'rebase' | 'rebase-with-fixup';
1619

1720
/** Configuration for the Github API merge strategy. */
1821
export interface GithubApiMergeStrategyConfig {
@@ -76,7 +79,8 @@ export function assertValidPullRequestConfig<T extends NgDevConfig>(
7679
);
7780
}
7881

79-
if (config.pullRequest.githubApiMerge === undefined) {
82+
const {githubApiMerge} = config.pullRequest;
83+
if (githubApiMerge === undefined) {
8084
errors.push('No explicit choice of merge strategy. Please set `githubApiMerge`.');
8185
}
8286

ng-dev/pr/merge/strategies/api-merge.ts

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
import {RestEndpointMethodTypes} from '@octokit/plugin-rest-endpoint-methods';
1010

11-
import {parseCommitMessage} from '../../../commit-message/parse.js';
1211
import {AuthenticatedGitClient} from '../../../utils/git/authenticated-git-client.js';
1312
import {GithubApiMergeMethod, GithubApiMergeStrategyConfig} from '../../config/index.js';
1413
import {PullRequest} from '../pull-request.js';
@@ -17,13 +16,11 @@ import {MergeStrategy} from './strategy.js';
1716
import {isGithubApiError} from '../../../utils/git/github.js';
1817
import {FatalMergeToolError, MergeConflictsFatalError} from '../failures.js';
1918
import {Prompt} from '../../../utils/prompt.js';
19+
import {AutosquashMergeStrategy} from './autosquash-merge.js';
2020

2121
/** Type describing the parameters for the Octokit `merge` API endpoint. */
2222
type OctokitMergeParams = RestEndpointMethodTypes['pulls']['merge']['parameters'];
2323

24-
type OctokitPullRequestCommitsList =
25-
RestEndpointMethodTypes['pulls']['listCommits']['response']['data'];
26-
2724
/** Separator between commit message header and body. */
2825
const COMMIT_HEADER_SEPARATOR = '\n\n';
2926

@@ -37,7 +34,7 @@ const COMMIT_HEADER_SEPARATOR = '\n\n';
3734
export class GithubApiMergeStrategy extends MergeStrategy {
3835
constructor(
3936
git: AuthenticatedGitClient,
40-
private _config: GithubApiMergeStrategyConfig,
37+
private config: GithubApiMergeStrategyConfig,
4138
) {
4239
super(git);
4340
}
@@ -52,12 +49,20 @@ export class GithubApiMergeStrategy extends MergeStrategy {
5249
*/
5350
override async merge(pullRequest: PullRequest): Promise<void> {
5451
const {githubTargetBranch, prNumber, needsCommitMessageFixup, targetBranches} = pullRequest;
55-
const method = this._getMergeActionFromPullRequest(pullRequest);
52+
const method = this.getMergeActionFromPullRequest(pullRequest);
5653
const cherryPickTargetBranches = targetBranches.filter((b) => b !== githubTargetBranch);
5754

55+
// Squash and Merge will create a single commit message and thus we can use the API to merge.
56+
if (
57+
method === 'rebase-with-fixup' &&
58+
(pullRequest.needsCommitMessageFixup || (await this.hasFixupOrSquashCommits(pullRequest)))
59+
) {
60+
return new AutosquashMergeStrategy(this.git).merge(pullRequest);
61+
}
62+
5863
const mergeOptions: OctokitMergeParams = {
5964
pull_number: prNumber,
60-
merge_method: method,
65+
merge_method: method === 'rebase-with-fixup' ? 'rebase' : method,
6166
...this.git.remoteParams,
6267
};
6368

@@ -195,10 +200,7 @@ export class GithubApiMergeStrategy extends MergeStrategy {
195200
* behavior here so that we have a default commit message that can be fixed up.
196201
*/
197202
private async _getDefaultSquashCommitMessage(pullRequest: PullRequest): Promise<string> {
198-
const commits = (await this._getPullRequestCommitMessages(pullRequest)).map((message) => ({
199-
message,
200-
parsed: parseCommitMessage(message),
201-
}));
203+
const commits = await this.getPullRequestCommits(pullRequest);
202204
const messageBase = `${pullRequest.title}${COMMIT_HEADER_SEPARATOR}`;
203205
if (commits.length <= 1) {
204206
return `${messageBase}${commits[0].parsed.body}`;
@@ -207,23 +209,21 @@ export class GithubApiMergeStrategy extends MergeStrategy {
207209
return `${messageBase}${joinedMessages}`;
208210
}
209211

210-
/** Gets all commit messages of commits in the pull request. */
211-
private async _getPullRequestCommitMessages({prNumber}: PullRequest) {
212-
const allCommits = await this.git.github.paginate(this.git.github.pulls.listCommits, {
213-
...this.git.remoteParams,
214-
pull_number: prNumber,
215-
});
216-
return allCommits.map(({commit}) => commit.message);
217-
}
218-
219212
/** Determines the merge action from the given pull request. */
220-
private _getMergeActionFromPullRequest({labels}: PullRequest): GithubApiMergeMethod {
221-
if (this._config.labels) {
222-
const matchingLabel = this._config.labels.find(({pattern}) => labels.includes(pattern));
213+
private getMergeActionFromPullRequest({labels}: PullRequest): GithubApiMergeMethod {
214+
if (this.config.labels) {
215+
const matchingLabel = this.config.labels.find(({pattern}) => labels.includes(pattern));
223216
if (matchingLabel !== undefined) {
224217
return matchingLabel.method;
225218
}
226219
}
227-
return this._config.default;
220+
return this.config.default;
221+
}
222+
223+
/** Checks whether the pull request contains fixup or squash commits. */
224+
private async hasFixupOrSquashCommits(pullRequest: PullRequest): Promise<boolean> {
225+
const commits = await this.getPullRequestCommits(pullRequest);
226+
227+
return commits.some(({parsed: {isFixup, isSquash}}) => isFixup || isSquash);
228228
}
229229
}

ng-dev/pr/merge/strategies/strategy.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9+
import {Commit, parseCommitMessage} from '../../../commit-message/parse.js';
910
import {AuthenticatedGitClient} from '../../../utils/git/authenticated-git-client.js';
1011
import {
1112
FatalMergeToolError,
@@ -200,4 +201,22 @@ export abstract class MergeStrategy {
200201
throw new MergeConflictsFatalError(failedBranches);
201202
}
202203
}
204+
205+
/** Gets all commit messages of commits in the pull request. */
206+
protected async getPullRequestCommits({prNumber}: PullRequest): Promise<
207+
{
208+
message: string;
209+
parsed: Commit;
210+
}[]
211+
> {
212+
const allCommits = await this.git.github.paginate(this.git.github.pulls.listCommits, {
213+
...this.git.remoteParams,
214+
pull_number: prNumber,
215+
});
216+
217+
return allCommits.map(({commit: {message}}) => ({
218+
message,
219+
parsed: parseCommitMessage(message),
220+
}));
221+
}
203222
}

0 commit comments

Comments
 (0)