Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion .github/local-actions/branch-manager/main.js

Large diffs are not rendered by default.

5 changes: 4 additions & 1 deletion .ng-dev/pull-request.mts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import {PullRequestConfig} from '../ng-dev/pr/config/index.js';

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

// Disable target labeling in the dev-infra repo as we don't have
Expand Down
5 changes: 4 additions & 1 deletion ng-dev/pr/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,14 @@

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

// TODO(alanagius): remove `rebase-with-fixup` and replace it's logic with `rebase`.
// This is just temporary to allow testing in the dev-infra repo. Without breaking workflows in other repos.

/**
* Possible merge methods supported by the Github API.
* https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button.
*/
export type GithubApiMergeMethod = 'merge' | 'squash' | 'rebase';
export type GithubApiMergeMethod = 'merge' | 'squash' | 'rebase' | 'rebase-with-fixup';

/** Configuration for the Github API merge strategy. */
export interface GithubApiMergeStrategyConfig {
Expand Down
51 changes: 25 additions & 26 deletions ng-dev/pr/merge/strategies/api-merge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,18 @@

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

import {parseCommitMessage} from '../../../commit-message/parse.js';
import {AuthenticatedGitClient} from '../../../utils/git/authenticated-git-client.js';
import {GithubApiMergeMethod, GithubApiMergeStrategyConfig} from '../../config/index.js';
import {PullRequest} from '../pull-request.js';

import {MergeStrategy} from './strategy.js';
import {isGithubApiError} from '../../../utils/git/github.js';
import {FatalMergeToolError, MergeConflictsFatalError} from '../failures.js';
import {Prompt} from '../../../utils/prompt.js';
import {AutosquashMergeStrategy} from './autosquash-merge.js';

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

type OctokitPullRequestCommitsList =
RestEndpointMethodTypes['pulls']['listCommits']['response']['data'];

/** Separator between commit message header and body. */
const COMMIT_HEADER_SEPARATOR = '\n\n';

Expand All @@ -34,10 +30,10 @@ const COMMIT_HEADER_SEPARATOR = '\n\n';
* target branches using the local Git instance. The benefit is that the Github merged state
* is properly set, but a notable downside is that PRs cannot use fixup or squash commits.
*/
export class GithubApiMergeStrategy extends MergeStrategy {
export class GithubApiMergeStrategy extends AutosquashMergeStrategy {
constructor(
git: AuthenticatedGitClient,
private _config: GithubApiMergeStrategyConfig,
private config: GithubApiMergeStrategyConfig,
) {
super(git);
}
Expand All @@ -52,12 +48,20 @@ export class GithubApiMergeStrategy extends MergeStrategy {
*/
override async merge(pullRequest: PullRequest): Promise<void> {
const {githubTargetBranch, prNumber, needsCommitMessageFixup, targetBranches} = pullRequest;
const method = this._getMergeActionFromPullRequest(pullRequest);
const method = this.getMergeActionFromPullRequest(pullRequest);
const cherryPickTargetBranches = targetBranches.filter((b) => b !== githubTargetBranch);

// Squash and Merge will create a single commit message and thus we can use the API to merge.
if (
method === 'rebase-with-fixup' &&
(pullRequest.needsCommitMessageFixup || (await this.hasFixupOrSquashCommits(pullRequest)))
) {
return super.merge(pullRequest);
}

Comment on lines +54 to +61
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why but I just cannot parse this block to save my life.

I think we don't want to perform the merge immediately if it needs commit message fixing right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comit fixing is handled in

override async merge(pullRequest: PullRequest): Promise<void> {
which this class now extends from.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I didn't realize/connect that it changed base classes.

const mergeOptions: OctokitMergeParams = {
pull_number: prNumber,
merge_method: method,
merge_method: method === 'rebase-with-fixup' ? 'rebase' : method,
...this.git.remoteParams,
};

Expand Down Expand Up @@ -195,10 +199,7 @@ export class GithubApiMergeStrategy extends MergeStrategy {
* behavior here so that we have a default commit message that can be fixed up.
*/
private async _getDefaultSquashCommitMessage(pullRequest: PullRequest): Promise<string> {
const commits = (await this._getPullRequestCommitMessages(pullRequest)).map((message) => ({
message,
parsed: parseCommitMessage(message),
}));
const commits = await this.getPullRequestCommits(pullRequest);
const messageBase = `${pullRequest.title}${COMMIT_HEADER_SEPARATOR}`;
if (commits.length <= 1) {
return `${messageBase}${commits[0].parsed.body}`;
Expand All @@ -207,23 +208,21 @@ export class GithubApiMergeStrategy extends MergeStrategy {
return `${messageBase}${joinedMessages}`;
}

/** Gets all commit messages of commits in the pull request. */
private async _getPullRequestCommitMessages({prNumber}: PullRequest) {
const allCommits = await this.git.github.paginate(this.git.github.pulls.listCommits, {
...this.git.remoteParams,
pull_number: prNumber,
});
return allCommits.map(({commit}) => commit.message);
}

/** Determines the merge action from the given pull request. */
private _getMergeActionFromPullRequest({labels}: PullRequest): GithubApiMergeMethod {
if (this._config.labels) {
const matchingLabel = this._config.labels.find(({pattern}) => labels.includes(pattern));
private getMergeActionFromPullRequest({labels}: PullRequest): GithubApiMergeMethod {
if (this.config.labels) {
const matchingLabel = this.config.labels.find(({pattern}) => labels.includes(pattern));
if (matchingLabel !== undefined) {
return matchingLabel.method;
}
}
return this._config.default;
return this.config.default;
}

/** Checks whether the pull request contains fixup or squash commits. */
private async hasFixupOrSquashCommits(pullRequest: PullRequest): Promise<boolean> {
const commits = await this.getPullRequestCommits(pullRequest);

return commits.some(({parsed: {isFixup, isSquash}}) => isFixup || isSquash);
}
}
19 changes: 19 additions & 0 deletions ng-dev/pr/merge/strategies/strategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Commit, parseCommitMessage} from '../../../commit-message/parse.js';
import {AuthenticatedGitClient} from '../../../utils/git/authenticated-git-client.js';
import {
FatalMergeToolError,
Expand Down Expand Up @@ -200,4 +201,22 @@ export abstract class MergeStrategy {
throw new MergeConflictsFatalError(failedBranches);
}
}

/** Gets all commit messages of commits in the pull request. */
protected async getPullRequestCommits({prNumber}: PullRequest): Promise<
{
message: string;
parsed: Commit;
}[]
> {
const allCommits = await this.git.github.paginate(this.git.github.pulls.listCommits, {
...this.git.remoteParams,
pull_number: prNumber,
});

return allCommits.map(({commit: {message}}) => ({
message,
parsed: parseCommitMessage(message),
}));
}
}
Loading