Skip to content

Commit

Permalink
fix(ng-dev/pr): rename MergeConfig to PullRequestConfig, discover the…
Browse files Browse the repository at this point in the history
… attribute at pullRequest instead of merge (#237)

Rather than using MergeConfig from merge in the ng-dev config object, we use PullRequestConfig from pullRequest
as this config applies to all `pr` commands.

Fixes #203

BREAKING CHANGE:
`MergeConfig` has been renamed to `PullRequestConfig` and is now accessed via `pullRequest` on the provided
ng-dev config.

PR Close #237
  • Loading branch information
josephperrott committed Sep 20, 2021
1 parent 10e9993 commit c3f5729
Show file tree
Hide file tree
Showing 8 changed files with 48 additions and 45 deletions.
20 changes: 10 additions & 10 deletions github-actions/breaking-changes-label/main.js

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions ng-dev/pr/check-target-branches/check-target-branches.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@
import {assertValidGithubConfig, getConfig, GithubConfig} from '../../utils/config';
import {info} from '../../utils/console';
import {GitClient} from '../../utils/git/git-client';
import {assertValidMergeConfig, MergeConfig} from '../config';
import {assertValidPullRequestConfig, PullRequestConfig} from '../config';
import {getTargetBranchesForPullRequest} from '../common/targeting/target-label';

async function getTargetBranchesForPr(
prNumber: number,
config: {github: GithubConfig; merge: MergeConfig},
config: {github: GithubConfig; pullRequest: PullRequestConfig},
) {
/** Repo owner and name for the github repository. */
const {owner, name: repo} = config.github;
Expand All @@ -40,9 +40,9 @@ async function getTargetBranchesForPr(
export async function printTargetBranchesForPr(prNumber: number) {
const config = getConfig();
assertValidGithubConfig(config);
assertValidMergeConfig(config);
assertValidPullRequestConfig(config);

if (config.merge.noTargetLabeling) {
if (config.pullRequest.noTargetLabeling) {
info(`PR #${prNumber} will merge into: ${config.github.mainBranchName}`);
return;
}
Expand Down
12 changes: 6 additions & 6 deletions ng-dev/pr/common/targeting/target-label.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {MergeConfig} from '../../config';
import {PullRequestConfig} from '../../config';
import {getTargetLabelsForActiveReleaseTrains} from './labels';
import {GithubConfig} from '../../../utils/config';
import {Commit} from '../../../commit-message/parse';
Expand Down Expand Up @@ -65,7 +65,7 @@ export class InvalidTargetLabelError {

/** Gets the target label from the specified pull request labels. */
export async function getMatchingTargetLabelForPullRequest(
config: Pick<MergeConfig, 'noTargetLabeling'>,
config: Pick<PullRequestConfig, 'noTargetLabeling'>,
labelsOnPullRequest: string[],
allTargetLabels: TargetLabel[],
): Promise<TargetLabel> {
Expand Down Expand Up @@ -97,12 +97,12 @@ export async function getMatchingTargetLabelForPullRequest(
/** Get the branches the pull request should be merged into. */
export async function getTargetBranchesForPullRequest(
api: GithubClient,
config: {merge: MergeConfig; github: GithubConfig},
config: {pullRequest: PullRequestConfig; github: GithubConfig},
labelsOnPullRequest: string[],
githubTargetBranch: string,
commits: Commit[],
): Promise<string[]> {
if (config.merge.noTargetLabeling) {
if (config.pullRequest.noTargetLabeling) {
return [config.github.mainBranchName];
}

Expand All @@ -113,13 +113,13 @@ export async function getTargetBranchesForPullRequest(
try {
const targetLabels = await getTargetLabelsForActiveReleaseTrains(api, config);
const matchingLabel = await getMatchingTargetLabelForPullRequest(
config.merge,
config.pullRequest,
labelsOnPullRequest,
targetLabels,
);
const targetBranches = await getBranchesFromTargetLabel(matchingLabel, githubTargetBranch);

assertChangesAllowForTargetLabel(commits, matchingLabel, config.merge);
assertChangesAllowForTargetLabel(commits, matchingLabel, config.pullRequest);

return targetBranches;
} catch (error) {
Expand Down
4 changes: 2 additions & 2 deletions ng-dev/pr/common/validation/validations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import {Commit} from '../../../commit-message/parse';
import {TargetLabel, TargetLabelName} from '../targeting/target-label';
import {breakingChangeLabel, MergeConfig} from '../../config';
import {breakingChangeLabel, PullRequestConfig} from '../../config';
import {PullRequestFailure} from './failures';
import {red, warn} from '../../../utils/console';
import {RawPullRequest} from '../fetch-pull-request';
Expand All @@ -21,7 +21,7 @@ import {RawPullRequest} from '../fetch-pull-request';
export function assertChangesAllowForTargetLabel(
commits: Commit[],
label: TargetLabel,
config: MergeConfig,
config: PullRequestConfig,
) {
/**
* List of commit scopes which are exempted from target label content requirements. i.e. no `feat`
Expand Down
22 changes: 12 additions & 10 deletions ng-dev/pr/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export interface GithubApiMergeStrategyConfig {
}

/** Configuration for the merge script. */
export interface MergeConfig {
export interface PullRequestConfig {
/**
* Configuration for the upstream remote. All of these options are optional as
* defaults are provided by the common dev-infra github configuration.
Expand Down Expand Up @@ -56,26 +56,28 @@ export interface MergeConfig {
}

/** Loads and validates the merge configuration. */
export function assertValidMergeConfig<T>(
config: T & Partial<{merge: MergeConfig}>,
): asserts config is T & {merge: MergeConfig} {
export function assertValidPullRequestConfig<T>(
config: T & Partial<{pullRequest: PullRequestConfig}>,
): asserts config is T & {pullRequest: PullRequestConfig} {
const errors: string[] = [];
if (config.merge === undefined) {
throw new ConfigValidationError('No merge configuration found. Set the `merge` configuration.');
if (config.pullRequest === undefined) {
throw new ConfigValidationError(
'No pullRequest configuration found. Set the `pullRequest` configuration.',
);
}

if (!config.merge.claSignedLabel) {
if (!config.pullRequest.claSignedLabel) {
errors.push('No CLA signed label configured.');
}
if (!config.merge.mergeReadyLabel) {
if (!config.pullRequest.mergeReadyLabel) {
errors.push('No merge ready label configured.');
}
if (config.merge.githubApiMerge === undefined) {
if (config.pullRequest.githubApiMerge === undefined) {
errors.push('No explicit choice of merge strategy. Please set `githubApiMerge`.');
}

if (errors.length) {
throw new ConfigValidationError('Invalid `merge` configuration', errors);
throw new ConfigValidationError('Invalid `pullRequest` configuration', errors);
}
}

Expand Down
4 changes: 2 additions & 2 deletions ng-dev/pr/merge/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {AuthenticatedGitClient} from '../../utils/git/authenticated-git-client';
import {GithubApiRequestError} from '../../utils/git/github';
import {GITHUB_TOKEN_GENERATE_URL} from '../../utils/git/github-urls';

import {assertValidMergeConfig} from '../config';
import {assertValidPullRequestConfig} from '../config';
import {MergeResult, MergeStatus, PullRequestMergeTask, PullRequestMergeTaskFlags} from './task';

/**
Expand Down Expand Up @@ -137,7 +137,7 @@ async function createPullRequestMergeTask(flags: PullRequestMergeTaskFlags) {
try {
const config = getConfig();
assertValidGithubConfig(config);
assertValidMergeConfig(config);
assertValidPullRequestConfig(config);
/** The singleton instance of the authenticated git client. */
const git = AuthenticatedGitClient.get();

Expand Down
15 changes: 8 additions & 7 deletions ng-dev/pr/merge/pull-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ export async function loadAndValidatePullRequest(

const labels = prData.labels.nodes.map((l) => l.name);

if (!labels.some((name) => matchesPattern(name, config.merge.mergeReadyLabel))) {
if (!labels.some((name) => matchesPattern(name, config.pullRequest.mergeReadyLabel))) {
return PullRequestFailure.notMergeReady();
}
if (!labels.some((name) => matchesPattern(name, config.merge.claSignedLabel))) {
if (!labels.some((name) => matchesPattern(name, config.pullRequest.claSignedLabel))) {
return PullRequestFailure.claUnsigned();
}

Expand Down Expand Up @@ -99,13 +99,14 @@ export async function loadAndValidatePullRequest(
}

const requiredBaseSha =
config.merge.requiredBaseCommits && config.merge.requiredBaseCommits[githubTargetBranch];
config.pullRequest.requiredBaseCommits &&
config.pullRequest.requiredBaseCommits[githubTargetBranch];
const needsCommitMessageFixup =
!!config.merge.commitMessageFixupLabel &&
labels.some((name) => matchesPattern(name, config.merge.commitMessageFixupLabel));
!!config.pullRequest.commitMessageFixupLabel &&
labels.some((name) => matchesPattern(name, config.pullRequest.commitMessageFixupLabel));
const hasCaretakerNote =
!!config.merge.caretakerNoteLabel &&
labels.some((name) => matchesPattern(name, config.merge.caretakerNoteLabel!));
!!config.pullRequest.caretakerNoteLabel &&
labels.some((name) => matchesPattern(name, config.pullRequest.caretakerNoteLabel!));

return {
url: prData.url,
Expand Down
8 changes: 4 additions & 4 deletions ng-dev/pr/merge/task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {promptConfirm} from '../../utils/console';
import {AuthenticatedGitClient} from '../../utils/git/authenticated-git-client';
import {GitCommandError} from '../../utils/git/git-client';

import {MergeConfig} from '../config';
import {PullRequestConfig} from '../config';
import {PullRequestFailure} from '../common/validation/failures';
import {
getCaretakerNotePromptMessage,
Expand Down Expand Up @@ -57,7 +57,7 @@ export class PullRequestMergeTask {
private flags: PullRequestMergeTaskFlags;

constructor(
public config: {merge: MergeConfig; github: GithubConfig},
public config: {pullRequest: PullRequestConfig; github: GithubConfig},
public git: AuthenticatedGitClient,
flags: Partial<PullRequestMergeTaskFlags>,
) {
Expand Down Expand Up @@ -130,8 +130,8 @@ export class PullRequestMergeTask {
return {status: MergeStatus.USER_ABORTED};
}

const strategy = this.config.merge.githubApiMerge
? new GithubApiMergeStrategy(this.git, this.config.merge.githubApiMerge)
const strategy = this.config.pullRequest.githubApiMerge
? new GithubApiMergeStrategy(this.git, this.config.pullRequest.githubApiMerge)
: new AutosquashMergeStrategy(this.git);

// Branch or revision that is currently checked out so that we can switch back to
Expand Down

0 comments on commit c3f5729

Please sign in to comment.