Skip to content

Commit

Permalink
feat(ng-dev/pr): add dry-run flag to pr merge tooling (#733)
Browse files Browse the repository at this point in the history
Add a --dry-run flag to allow for checking if a PR is currently mergable into its target branches
in the current state.

PR Close #733
  • Loading branch information
josephperrott authored and devversion committed Jul 26, 2022
1 parent 64e801f commit 934abb0
Show file tree
Hide file tree
Showing 7 changed files with 35 additions and 16 deletions.
13 changes: 10 additions & 3 deletions ng-dev/pr/merge/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

import {Argv, Arguments, CommandModule} from 'yargs';
import {addDryRunFlag} from '../../utils/dry-run.js';

import {addGithubTokenOption} from '../../utils/git/github-yargs.js';

Expand All @@ -18,11 +19,12 @@ export interface MergeCommandOptions {
pr: number;
branchPrompt: boolean;
forceManualBranches: boolean;
dryRun: boolean;
}

/** Builds the command. */
function builder(argv: Argv) {
return addGithubTokenOption(argv)
return addDryRunFlag(addGithubTokenOption(argv))
.help()
.strict()
.positional('pr', {
Expand All @@ -43,8 +45,13 @@ function builder(argv: Argv) {
}

/** Handles the command. */
async function handler({pr, branchPrompt, forceManualBranches}: Arguments<MergeCommandOptions>) {
await mergePullRequest(pr, {branchPrompt, forceManualBranches});
async function handler({
pr,
branchPrompt,
forceManualBranches,
dryRun,
}: Arguments<MergeCommandOptions>) {
await mergePullRequest(pr, {branchPrompt, forceManualBranches, dryRun});
}

/** yargs command module describing the command. */
Expand Down
2 changes: 1 addition & 1 deletion ng-dev/pr/merge/failures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export class FatalMergeToolError extends Error {
}
}

/** Error class that can be thrown the user aborted the merge manually. */
/** Error class that can be thrown when the user aborted the merge manually. */
export class UserAbortedMergeToolError extends Error {}

export class MismatchedTargetBranchFatalError extends FatalMergeToolError {
Expand Down
2 changes: 1 addition & 1 deletion ng-dev/pr/merge/merge-pull-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ export async function mergePullRequest(prNumber: number, flags: PullRequestMerge
): Promise<boolean> {
try {
await tool.merge(prNumber, validationConfig);
Log.info(green(`Successfully merged the pull request: #${prNumber}`));
return true;
} catch (e) {
// Catch errors to the Github API for invalid requests. We want to
Expand All @@ -63,6 +62,7 @@ export async function mergePullRequest(prNumber: number, flags: PullRequestMerge
Log.warn('Manually aborted merging..');
return false;
}

if (e instanceof FatalMergeToolError) {
Log.error(`Could not merge the specified pull request. Error:`);
Log.error(` -> ${bold(e.message)}`);
Expand Down
14 changes: 11 additions & 3 deletions ng-dev/pr/merge/merge-tool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@
*/

import {AuthenticatedGitClient} from '../../utils/git/authenticated-git-client.js';
import {GitCommandError} from '../../utils/git/git-client.js';
import semver from 'semver';
import {prompt} from 'inquirer';
import {Log, red, yellow} from '../../utils/logging.js';
import {green, Log, red, yellow} from '../../utils/logging.js';

import {PullRequestConfig} from '../config/index.js';
import {
getCaretakerNotePromptMessage,
getTargetedBranchesConfirmationPromptMessage,
getTargetedBranchesMessage,
} from './messages.js';
import {loadAndValidatePullRequest, PullRequest} from './pull-request.js';
import {GithubApiMergeStrategy} from './strategies/api-merge.js';
Expand All @@ -34,11 +34,13 @@ import {PullRequestValidationConfig} from '../common/validation/validation-confi
export interface PullRequestMergeFlags {
branchPrompt: boolean;
forceManualBranches: boolean;
dryRun: boolean;
}

const defaultPullRequestMergeFlags: PullRequestMergeFlags = {
branchPrompt: true,
forceManualBranches: false,
dryRun: false,
};

/**
Expand All @@ -64,7 +66,6 @@ export class MergeTool {
* @param validationConfig Pull request validation config. Can be modified to skip
* certain non-fatal validations.
*/

async merge(prNumber: number, validationConfig: PullRequestValidationConfig): Promise<void> {
if (this.git.hasUncommittedChanges()) {
throw new FatalMergeToolError(
Expand Down Expand Up @@ -139,6 +140,12 @@ export class MergeTool {
// Check for conflicts between the pull request and target branches.
await strategy.check(pullRequest);

if (this.flags.dryRun) {
Log.info(getTargetedBranchesMessage(pullRequest));
Log.info(green(` ✓ Mergeablility of pull request confirmed, exiting dry run.`));
return;
}

if (
// In cases where manual branch targeting is used, the user already confirmed.
!this.flags.forceManualBranches &&
Expand All @@ -150,6 +157,7 @@ export class MergeTool {

// Perform the merge and pass-through potential failures.
await strategy.merge(pullRequest);
Log.info(green(` ✓ Successfully merged the pull request: #${prNumber}`));
} finally {
// Switch back to the previous branch. We need to do this before deleting the temporary
// branches because we cannot delete branches which are currently checked out.
Expand Down
8 changes: 6 additions & 2 deletions ng-dev/pr/merge/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ export function getCaretakerNotePromptMessage(pullRequest: PullRequest): string
}

export function getTargetedBranchesConfirmationPromptMessage(pullRequest: PullRequest): string {
const targetBranchListAsString = pullRequest.targetBranches.map((b) => ` - ${b}\n`).join('');
return `Pull request #${pullRequest.prNumber} will merge into:\n${targetBranchListAsString}\nDo you want to proceed merging?`;
return `${getTargetedBranchesMessage(pullRequest)}}\nDo you want to proceed merging?`;
}

export function getTargetedBranchesMessage(pullRequest: PullRequest): string {
const targetBranchListAsString = pullRequest.targetBranches.map((b) => ` - ${b}`).join('\n');
return `Pull request #${pullRequest.prNumber} will merge into:\n${targetBranchListAsString}`;
}
8 changes: 4 additions & 4 deletions ng-dev/pr/merge/pull-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {ActiveReleaseTrains} from '../../release/versioning/active-release-train
import {PullRequestValidationConfig} from '../common/validation/validation-config.js';
import {assertValidPullRequest} from '../common/validation/validate-pull-request.js';
import {TEMP_PR_HEAD_BRANCH} from './strategies/strategy.js';
import {getRepositoryGitUrl} from '../../utils/git/github-urls.js';

/** Interface that describes a pull request. */
export interface PullRequest {
Expand All @@ -33,8 +34,6 @@ export interface PullRequest {
targetBranches: string[];
/** Branch that the PR targets in the Github UI. */
githubTargetBranch: string;
/** List of branches this PR must be cherry picked into. */
cherryPickTargetBranches: string[];
/** Count of commits in this pull request. */
commitCount: number;
/** Optional SHA that this pull request needs to be based on. */
Expand Down Expand Up @@ -112,6 +111,8 @@ export async function loadAndValidatePullRequest(
!!config.pullRequest.caretakerNoteLabel &&
labels.includes(config.pullRequest.caretakerNoteLabel);

git.runGraceful(['fetch', getRepositoryGitUrl(config.github), `pull/${prNumber}/head`]);

/** Number of commits in the pull request. */
const commitCount = prData.commits.totalCount;
/**
Expand All @@ -123,7 +124,7 @@ export async function loadAndValidatePullRequest(
* change. We avoid this issue around this by parsing the base revision so that we are able
* to reference a specific SHA before a autosquash rebase could be performed.
*/
const baseSha = git.run(['rev-parse', `${TEMP_PR_HEAD_BRANCH}~${commitCount}`]).stdout.trim();
const baseSha = git.run(['rev-parse', `${prData.headRefOid}~${commitCount}`]).stdout.trim();
/* Git revision range that matches the pull request commits. */
const revisionRange = `${baseSha}..${TEMP_PR_HEAD_BRANCH}`;

Expand All @@ -139,7 +140,6 @@ export async function loadAndValidatePullRequest(
revisionRange,
hasCaretakerNote,
targetBranches: target.branches,
cherryPickTargetBranches: target.branches.filter((b) => b !== githubTargetBranch),
title: prData.title,
};
}
4 changes: 2 additions & 2 deletions ng-dev/pr/merge/strategies/api-merge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ export class GithubApiMergeStrategy extends MergeStrategy {
* @throws {FatalMergeToolError} A fatal error if the merge could not be performed.
*/
override async merge(pullRequest: PullRequest): Promise<void> {
const {githubTargetBranch, prNumber, needsCommitMessageFixup, cherryPickTargetBranches} =
pullRequest;
const {githubTargetBranch, prNumber, needsCommitMessageFixup, targetBranches} = pullRequest;
const method = this._getMergeActionFromPullRequest(pullRequest);
const cherryPickTargetBranches = targetBranches.filter((b) => b !== githubTargetBranch);

const mergeOptions: OctokitMergeParams = {
pull_number: prNumber,
Expand Down

0 comments on commit 934abb0

Please sign in to comment.