Skip to content
Open
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
3 changes: 2 additions & 1 deletion ng-dev/pr/common/validation/assert-passing-ci.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,11 @@ class Validation extends PullRequestValidation {
if (statuses.find((status) => status.name === 'lint') === undefined) {
throw this._createError(
'Pull request is missing expected status checks. Check the pull request for pending workflows',
false,
);
}
if (combinedStatus === PullRequestStatus.PENDING) {
throw this._createError('Pull request has pending status checks.');
throw this._createError('Pull request has pending status checks.', false);
}
if (combinedStatus === PullRequestStatus.FAILING) {
throw this._createError('Pull request has failing status checks.');
Expand Down
126 changes: 89 additions & 37 deletions ng-dev/pr/common/validation/validate-pull-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import {parseCommitMessage} from '../../../commit-message/parse.js';
import {ActiveReleaseTrains} from '../../../release/versioning/active-release-trains.js';
import {NgDevConfig, GithubConfig} from '../../../utils/config.js';
import {PullRequestConfig, PullRequestValidationConfig} from '../../config/index.js';
import {PullRequestFromGithub} from '../fetch-pull-request.js';
import {fetchPullRequestFromGithub, PullRequestFromGithub} from '../fetch-pull-request.js';
import {Log} from '../../../utils/logging.js';
import {Spinner} from '../../../utils/spinner.js';
import {PullRequestTarget} from '../targeting/target-label.js';
import {changesAllowForTargetLabelValidation} from './assert-allowed-target-label.js';
import {breakingChangeInfoValidation} from './assert-breaking-change-info.js';
Expand All @@ -33,7 +35,7 @@ import {AuthenticatedGitClient} from '../../../utils/git/authenticated-git-clien
* Active release trains may be available for additional checks or not.
*/
export async function assertValidPullRequest(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NIT: Maybe split the validations into a seperate functions, something like to make the code more readable.

/**
 * Runs all valiations that the given pull request is valid, returning a list of all failing
 * validations.
 *
 * Active release trains may be available for additional checks or not.
 */
async function runValidations(
  pullRequest: PullRequestFromGithub,
  validationConfig: PullRequestValidationConfig,
  ngDevConfig: NgDevConfig<{pullRequest: PullRequestConfig; github: GithubConfig}>,
  activeReleaseTrains: ActiveReleaseTrains | null,
  target: PullRequestTarget,
  gitClient: AuthenticatedGitClient,
): Promise<PullRequestValidationFailure[]> {
  const labels = pullRequest.labels.nodes.map((l) => l.name);
  const commitsInPr = pullRequest.commits.nodes.map((n) => {
    return parseCommitMessage(n.commit.message);
  });

  const validationPromises = [
    minimumReviewsValidation.run(validationConfig, pullRequest),
    completedReviewsValidation.run(validationConfig, pullRequest),
    mergeReadyValidation.run(validationConfig, pullRequest),
    signedClaValidation.run(validationConfig, pullRequest),
    pendingStateValidation.run(validationConfig, pullRequest),
    breakingChangeInfoValidation.run(validationConfig, commitsInPr, labels),
    passingCiValidation.run(validationConfig, pullRequest),
    enforcedStatusesValidation.run(validationConfig, pullRequest, ngDevConfig.pullRequest),
    isolatedSeparateFilesValidation.run(
      validationConfig,
      ngDevConfig,
      pullRequest.number,
      gitClient,
    ),
    enforceTestedValidation.run(validationConfig, pullRequest, gitClient),
  ];

  if (activeReleaseTrains !== null) {
    validationPromises.push(
      changesAllowForTargetLabelValidation.run(
        validationConfig,
        commitsInPr,
        target.label,
        ngDevConfig.pullRequest,
        activeReleaseTrains,
        labels,
        pullRequest,
      ),
    );
  }

  const results = await Promise.all(validationPromises);
  return results.filter((result): result is PullRequestValidationFailure => result !== null);
}

export async function assertValidPullRequest(
  originalPullRequest: PullRequestFromGithub,
  validationConfig: PullRequestValidationConfig,
  ngDevConfig: NgDevConfig<{
    pullRequest: PullRequestConfig;
    github: GithubConfig;
  }>,
  activeReleaseTrains: ActiveReleaseTrains | null,
  target: PullRequestTarget,
  gitClient: AuthenticatedGitClient,
): Promise<PullRequestValidationFailure[]> {
  let pullRequest = originalPullRequest;
  let spinner: Spinner | undefined;
  const maxAttempts = 60;

  for (let attempts = 0; attempts <= maxAttempts; attempts++) {
    const failures = await runValidations(
      pullRequest,
      validationConfig,
      ngDevConfig,
      activeReleaseTrains,
      target,
      gitClient,
    );

    const finalFailures = failures.filter((f) => f.isFinal);
    const nonFinalFailures = failures.filter((f) => !f.isFinal);

    const shouldWaitForPending =
      nonFinalFailures.length > 0 &&
      finalFailures.length === 0 &&
      validationConfig.waitIfPending;

    if (!shouldWaitForPending) {
      if (spinner) {
        spinner.complete();
      }
      return failures;
    }

    if (attempts === maxAttempts) {
      if (spinner) {
        spinner.complete();
      }
      Log.error(
        `Timed out waiting for non-final validations to complete after ${maxAttempts} minutes.`,
      );
      return failures;
    }

    const names = nonFinalFailures.map((f) => f.validationName).join(', ');
    const verb = nonFinalFailures.length === 1 ? 'is' : 'are';
    const spinnerText = `[${names}] ${verb} not final. Waiting for completion (attempt ${attempts + 1}/${maxAttempts})...`;

    if (!spinner) {
      spinner = new Spinner(spinnerText);
    } else {
      spinner.update(spinnerText);
    }

    await new Promise((resolve) => setTimeout(resolve, 60000)); // Wait 1 minute
    const freshPr = await fetchPullRequestFromGithub(gitClient, originalPullRequest.number);
    if (!freshPr) {
      throw new Error('Failed to re-fetch pull request data');
    }
    pullRequest = freshPr;
  }

  throw new Error('Unreachable');
}

pullRequest: PullRequestFromGithub,
originalPullRequest: PullRequestFromGithub,
validationConfig: PullRequestValidationConfig,
ngDevConfig: NgDevConfig<{
pullRequest: PullRequestConfig;
Expand All @@ -43,48 +45,98 @@ export async function assertValidPullRequest(
target: PullRequestTarget,
gitClient: AuthenticatedGitClient,
): Promise<PullRequestValidationFailure[]> {
const labels = pullRequest.labels.nodes.map((l) => l.name);
const commitsInPr = pullRequest.commits.nodes.map((n) => {
return parseCommitMessage(n.commit.message);
});
let pullRequest = originalPullRequest;
let spinner: Spinner | undefined;
let attempts = 0;
const maxAttempts = 60;

const validationResults = [
minimumReviewsValidation.run(validationConfig, pullRequest),
completedReviewsValidation.run(validationConfig, pullRequest),
mergeReadyValidation.run(validationConfig, pullRequest),
signedClaValidation.run(validationConfig, pullRequest),
pendingStateValidation.run(validationConfig, pullRequest),
breakingChangeInfoValidation.run(validationConfig, commitsInPr, labels),
passingCiValidation.run(validationConfig, pullRequest),
enforcedStatusesValidation.run(validationConfig, pullRequest, ngDevConfig.pullRequest),
isolatedSeparateFilesValidation.run(
validationConfig,
ngDevConfig,
pullRequest.number,
gitClient,
),
enforceTestedValidation.run(validationConfig, pullRequest, gitClient),
];
while (true) {
const labels = pullRequest.labels.nodes.map((l) => l.name);
const commitsInPr = pullRequest.commits.nodes.map((n) => {
return parseCommitMessage(n.commit.message);
});

if (activeReleaseTrains !== null) {
validationResults.push(
changesAllowForTargetLabelValidation.run(
const validationResults = [
minimumReviewsValidation.run(validationConfig, pullRequest),
completedReviewsValidation.run(validationConfig, pullRequest),
mergeReadyValidation.run(validationConfig, pullRequest),
signedClaValidation.run(validationConfig, pullRequest),
pendingStateValidation.run(validationConfig, pullRequest),
breakingChangeInfoValidation.run(validationConfig, commitsInPr, labels),
passingCiValidation.run(validationConfig, pullRequest),
enforcedStatusesValidation.run(validationConfig, pullRequest, ngDevConfig.pullRequest),
isolatedSeparateFilesValidation.run(
validationConfig,
commitsInPr,
target.label,
ngDevConfig.pullRequest,
activeReleaseTrains,
labels,
pullRequest,
ngDevConfig,
pullRequest.number,
gitClient,
),
);
}
enforceTestedValidation.run(validationConfig, pullRequest, gitClient),
];

if (activeReleaseTrains !== null) {
validationResults.push(
changesAllowForTargetLabelValidation.run(
validationConfig,
commitsInPr,
target.label,
ngDevConfig.pullRequest,
activeReleaseTrains,
labels,
pullRequest,
),
);
}

return await Promise.all(validationResults).then((results) => {
return results.filter(
const results = await Promise.all(validationResults);
const failures = results.filter(
<(result: null | PullRequestValidationFailure) => result is PullRequestValidationFailure>(
((result) => result !== null)
),
);
});

const finalFailures = failures.filter((f) => f.isFinal);
const nonFinalFailures = failures.filter((f) => !f.isFinal);

if (
nonFinalFailures.length > 0 &&
finalFailures.length === 0 &&
validationConfig.waitIfPending
) {
if (attempts >= maxAttempts) {
if (spinner) {
spinner.complete();
}
Log.error(
`Timed out waiting for non-final validations to complete after ${maxAttempts} minutes.`,
);
return failures;
}
attempts++;

const names = nonFinalFailures.map((f) => f.validationName).join(', ');
const verb = nonFinalFailures.length === 1 ? 'is' : 'are';
const spinnerText = `[${names}] ${verb} not final. Waiting for completion (attempt ${attempts}/${maxAttempts})...`;

if (!spinner) {
spinner = new Spinner(spinnerText);
} else {
spinner.update(spinnerText);
}

await new Promise((resolve) => setTimeout(resolve, 60000)); // Wait 1 minute
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NIT

Suggested change
await new Promise((resolve) => setTimeout(resolve, 60000)); // Wait 1 minute
import {setTimeout} from 'node:timers/promises';
await setTimeout(60_000); // Wait 1 minute

const freshPr = await fetchPullRequestFromGithub(gitClient, originalPullRequest.number);
if (!freshPr) {
throw new Error('Failed to re-fetch pull request data');
}
pullRequest = freshPr;
continue;
}

if (spinner) {
spinner.complete();
}

return failures;
}
}
8 changes: 6 additions & 2 deletions ng-dev/pr/common/validation/validation-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ export function createPullRequestValidationConfig(
}

/** Type describing a helper function for validations to create a validation failure. */
export type PullRequestValidationErrorCreateFn = (message: string) => PullRequestValidationFailure;
export type PullRequestValidationErrorCreateFn = (
message: string,
isFinal?: boolean,
) => PullRequestValidationFailure;

/**
* Base class for pull request validations, providing helpers for the validation errors,
Expand Down Expand Up @@ -62,7 +65,8 @@ export function createPullRequestValidation<T extends PullRequestValidation>(
if (validationConfig[name]) {
const validation = new (getValidationCtor())(
name,
(message) => new PullRequestValidationFailure(message, name, canBeForceIgnored),
(message, isFinal = true) =>
new PullRequestValidationFailure(message, name, canBeForceIgnored, isFinal),
);
try {
await validation.assert(...args);
Expand Down
2 changes: 2 additions & 0 deletions ng-dev/pr/common/validation/validation-failure.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,7 @@ export class PullRequestValidationFailure {
public readonly validationName: keyof PullRequestValidationConfig,
/** Validation config name for the failure. */
public readonly canBeForceIgnored: boolean,
/** Whether the failure is final and will not change without action from a human. */
public readonly isFinal: boolean = true,
) {}
}
1 change: 1 addition & 0 deletions ng-dev/pr/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,5 @@ export function assertValidPullRequestConfig<T extends NgDevConfig>(

export interface PullRequestValidationConfig {
[key: `assert${string}`]: boolean;
waitIfPending?: boolean;
}
15 changes: 14 additions & 1 deletion ng-dev/pr/merge/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export interface MergeCommandOptions {
forceManualBranches: boolean;
dryRun: boolean;
ignorePendingReviews: boolean;
waitForValidations: boolean;
}

/** Builds the command. */
Expand Down Expand Up @@ -45,6 +46,11 @@ async function builder(argv: Argv) {
type: 'boolean',
default: false,
description: 'Bypass the check for pending reviews on the pull request',
})
.option('wait-for-validations' as 'waitForValidations', {
type: 'boolean',
default: false,
description: 'Wait for pending validations to complete before merging.',
});
}

Expand All @@ -55,8 +61,15 @@ async function handler({
forceManualBranches,
dryRun,
ignorePendingReviews,
waitForValidations,
}: Arguments<MergeCommandOptions>) {
await mergePullRequest(pr, {branchPrompt, forceManualBranches, dryRun, ignorePendingReviews});
await mergePullRequest(pr, {
branchPrompt,
forceManualBranches,
dryRun,
ignorePendingReviews,
waitForValidations,
});
}

/** yargs command module describing the command. */
Expand Down
3 changes: 3 additions & 0 deletions ng-dev/pr/merge/merge-tool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,15 @@ export interface PullRequestMergeFlags {
forceManualBranches: boolean;
dryRun: boolean;
ignorePendingReviews: boolean;
waitForValidations: boolean;
}

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

/**
Expand Down Expand Up @@ -82,6 +84,7 @@ export class MergeTool {
const validationConfig = createPullRequestValidationConfig({
...this.config.pullRequest.validators,
...partialValidationConfig,
waitIfPending: this.flags.waitForValidations,
});

if (this.git.hasUncommittedChanges()) {
Expand Down
Loading