Skip to content

Commit

Permalink
fix(ng-dev/pr): check CLA status rather than label for CLA passing st…
Browse files Browse the repository at this point in the history
…atus on pr merges (#242)

Rather than check if the CLA is signed based on the presense of a label, the cla/google status should be
used.

BREAKING CHANGE:
`claSignedLabel` is not longer used as an attribute on the `PullRequestConfig`

PR Close #242
  • Loading branch information
josephperrott committed Sep 27, 2021
1 parent 6777242 commit 2556d72
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 13 deletions.
5 changes: 1 addition & 4 deletions github-actions/breaking-changes-label/main.js

Large diffs are not rendered by default.

22 changes: 21 additions & 1 deletion ng-dev/pr/common/validation/validations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ import {TargetLabel, TargetLabelName} from '../targeting/target-label';
import {breakingChangeLabel, PullRequestConfig} from '../../config';
import {PullRequestFailure} from './failures';
import {red, warn} from '../../../utils/console';
import {PullRequestFromGithub} from '../fetch-pull-request';
import {
getStatusesForPullRequest,
PullRequestFromGithub,
PullRequestStatus,
} from '../fetch-pull-request';

/**
* Assert the commits provided are allowed to merge to the provided target label,
Expand Down Expand Up @@ -102,3 +106,19 @@ export function assertPendingState(pullRequest: PullRequestFromGithub) {
throw PullRequestFailure.isMerged();
}
}

/**
* Assert the pull request has all necessary CLAs signed.
* @throws {PullRequestFailure} if the pull request is missing a necessary CLA signature.
*/
export function assertSignedCla(pullRequest: PullRequestFromGithub) {
const passing = getStatusesForPullRequest(pullRequest).statuses.some(({name, status}) => {
return name === 'cla/google' && status === PullRequestStatus.PASSING;
});

if (passing) {
return;
}

throw PullRequestFailure.claUnsigned();
}
5 changes: 0 additions & 5 deletions ng-dev/pr/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ export interface PullRequestConfig {
noTargetLabeling?: boolean;
/** Required base commits for given branches. */
requiredBaseCommits?: {[branchName: string]: string};
/** Pattern that matches labels which imply a signed CLA. */
claSignedLabel: string | RegExp;
/** Pattern that matches labels which imply a merge ready pull request. */
mergeReadyLabel: string | RegExp;
/** Label that is applied when special attention from the caretaker is required. */
Expand Down Expand Up @@ -66,9 +64,6 @@ export function assertValidPullRequestConfig<T>(
);
}

if (!config.pullRequest.claSignedLabel) {
errors.push('No CLA signed label configured.');
}
if (!config.pullRequest.mergeReadyLabel) {
errors.push('No merge ready label configured.');
}
Expand Down
5 changes: 2 additions & 3 deletions ng-dev/pr/merge/pull-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {getTargetBranchesForPullRequest} from '../common/targeting/target-label'
import {
assertCorrectBreakingChangeLabeling,
assertPendingState,
assertSignedCla,
} from '../common/validation/validations';
import {
fetchPullRequestFromGithub,
Expand Down Expand Up @@ -65,9 +66,6 @@ export async function loadAndValidatePullRequest(
if (!labels.some((name) => matchesPattern(name, config.pullRequest.mergeReadyLabel))) {
return PullRequestFailure.notMergeReady();
}
if (!labels.some((name) => matchesPattern(name, config.pullRequest.claSignedLabel))) {
return PullRequestFailure.claUnsigned();
}

/** List of parsed commits for all of the commits in the pull request. */
const commitsInPr = prData.commits.nodes.map((n) => parseCommitMessage(n.commit.message));
Expand All @@ -82,6 +80,7 @@ export async function loadAndValidatePullRequest(
);

try {
assertSignedCla(prData);
assertPendingState(prData);
assertCorrectBreakingChangeLabeling(commitsInPr, labels);
} catch (error) {
Expand Down

0 comments on commit 2556d72

Please sign in to comment.