diff --git a/.github/workflows/pr-linter.yml b/.github/workflows/pr-linter.yml index 1d60432ae9b13..3b830e1078c4a 100644 --- a/.github/workflows/pr-linter.yml +++ b/.github/workflows/pr-linter.yml @@ -14,13 +14,10 @@ on: # Triggered from a separate job when a review is added workflow_run: - workflows: [PR Linter Trigger] + workflows: ["PR Linter Trigger", "Codebuild PR Build"] types: - completed - # Trigger when a status is updated (CodeBuild leads to statuses) - status: {} - # Trigger when a check suite is completed (GitHub actions and CodeCov create checks) check_suite: types: [completed] diff --git a/tools/@aws-cdk/prlint/constants.ts b/tools/@aws-cdk/prlint/constants.ts index 4aee5e4c1a5a5..fc0de8ed66d38 100644 --- a/tools/@aws-cdk/prlint/constants.ts +++ b/tools/@aws-cdk/prlint/constants.ts @@ -1,7 +1,7 @@ export const DEFAULT_LINTER_LOGIN = 'aws-cdk-automation'; -export const CODE_BUILD_CONTEXT = 'AWS CodeBuild us-east-1 (AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv)'; +export const CODE_BUILD_WORKFLOW_FILE = 'codebuild-pr-build.yml'; /** * Types of exemption labels in aws-cdk project. diff --git a/tools/@aws-cdk/prlint/index.ts b/tools/@aws-cdk/prlint/index.ts index 00f502b0fb2f8..0d3fb12fa3d2e 100644 --- a/tools/@aws-cdk/prlint/index.ts +++ b/tools/@aws-cdk/prlint/index.ts @@ -2,7 +2,6 @@ import * as github from '@actions/github'; import { Octokit } from '@octokit/rest'; import { StatusEvent, PullRequestEvent, CheckSuiteEvent } from '@octokit/webhooks-definitions/schema'; import { PullRequestLinter } from './lint'; -import { LinterActions } from './linter-base'; import { DEFAULT_LINTER_LOGIN } from './constants'; /** @@ -42,23 +41,7 @@ async function run() { linterLogin: process.env.LINTER_LOGIN || DEFAULT_LINTER_LOGIN, }); - let actions: LinterActions | undefined; - - switch (github.context.eventName) { - case 'status': - // Triggering on a 'status' event is solely used to see if the CodeBuild - // job just turned green, and adding certain 'ready for review' labels - // if it does. - const statusPayload = github.context.payload as StatusEvent; - console.log('validating status event'); - actions = await prLinter.validateStatusEvent(statusPayload); - break; - - default: - // This is the main PR validator action. - actions = await prLinter.validatePullRequestTarget(); - break; - } + const actions = await prLinter.validatePullRequestTarget(); if (actions) { console.log(actions); diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index 13dab9b6b51cc..aebcb05bf939f 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -3,8 +3,7 @@ import { findModulePath, moduleStability } from './module'; import { breakingModules } from './parser'; import { CheckRun, GitHubFile, GitHubPr, Review, sumChanges, summarizeRunConclusions } from "./github"; import { TestResult, ValidationCollector } from './results'; -import { CODE_BUILD_CONTEXT, CODECOV_CHECKS, Exemption } from './constants'; -import { StatusEvent } from '@octokit/webhooks-definitions/schema'; +import { CODE_BUILD_WORKFLOW_FILE, CODECOV_CHECKS, Exemption } from './constants'; import { LinterActions, mergeLinterActions, PR_FROM_MAIN_ERROR, PullRequestLinterBase } from './linter-base'; /** @@ -20,21 +19,15 @@ export class PullRequestLinter extends PullRequestLinterBase { * @param sha the commit sha to evaluate */ private async codeBuildJobSucceeded(sha: string): Promise { - const statuses = await this.client.repos.listCommitStatusesForRef({ + const statuses = await this.client.actions.listWorkflowRuns({ owner: this.prParams.owner, repo: this.prParams.repo, - ref: sha, + head_sha: sha, + workflow_id: CODE_BUILD_WORKFLOW_FILE, }); - let status = statuses.data.filter(status => status.context === CODE_BUILD_CONTEXT).map(status => status.state); - console.log("CodeBuild Commit Statuses: ", status); - return statuses.data.some(status => status.context === CODE_BUILD_CONTEXT && status.state === 'success'); - } - - public async validateStatusEvent(status: StatusEvent): Promise { - if (status.context === CODE_BUILD_CONTEXT && status.state === 'success') { - return this.assessNeedsReview(); - } - return {}; + let conclusions = statuses.data.workflow_runs.filter(run => run.status === 'completed').map(run => run.conclusion); + console.log("CodeBuild Commit Conclusions: ", conclusions); + return conclusions.some(conclusion => conclusion === 'success'); } /** diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index f9fb9055398ce..3971c09ca75ed 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -1,8 +1,7 @@ import * as path from 'path'; import { GitHubFile, GitHubLabel, GitHubPr } from '../github'; -import { CODE_BUILD_CONTEXT, CODECOV_CHECKS } from '../constants'; +import { CODECOV_CHECKS } from '../constants'; import { PullRequestLinter } from '../lint'; -import { StatusEvent } from '@octokit/webhooks-definitions/schema'; import { createOctomock, OctoMock } from './octomock'; type GitHubFileName = Omit; @@ -589,13 +588,22 @@ describe('integration tests required on features', () => { }); }); - describe('assess needs review from status event', () => { + describe('assess needs review', () => { const pr = { + title: 'chore(s3): something', draft: false, mergeable_state: 'behind', number: 1234, labels: [{ name: 'p2' }], }; + const files = [ + { + filename: 'some-test.test.ts', + }, + { + filename: 'readme.md', + }, + ]; beforeEach(() => { mockListReviews.mockImplementation(() => { return { @@ -606,12 +614,8 @@ describe('integration tests required on features', () => { test('needs a review', async () => { // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockAddLabel.mock.calls[0][0]).toEqual({ @@ -626,12 +630,8 @@ describe('integration tests required on features', () => { test('needs a review and is p1', async () => { // WHEN pr.labels = [{ name: 'p1' }]; - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockAddLabel.mock.calls[0][0]).toEqual({ @@ -657,12 +657,8 @@ describe('integration tests required on features', () => { ]; // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockRemoveLabel.mock.calls[0][0]).toEqual({ @@ -688,12 +684,8 @@ describe('integration tests required on features', () => { ]; // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockAddLabel.mock.calls[0][0]).toEqual({ @@ -725,12 +717,8 @@ describe('integration tests required on features', () => { ]; // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockRemoveLabel.mock.calls[0][0]).toEqual({ @@ -758,12 +746,8 @@ describe('integration tests required on features', () => { ]; // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockRemoveLabel.mock.calls[0][0]).toEqual({ @@ -791,12 +775,8 @@ describe('integration tests required on features', () => { ]; // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockRemoveLabel.mock.calls[0][0]).toEqual({ @@ -831,12 +811,8 @@ describe('integration tests required on features', () => { ]; // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockRemoveLabel.mock.calls[0][0]).toEqual({ @@ -869,12 +845,8 @@ describe('integration tests required on features', () => { ]; // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockRemoveLabel.mock.calls[0][0]).toEqual({ @@ -899,12 +871,8 @@ describe('integration tests required on features', () => { (pr as any).labels = []; // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockRemoveLabel.mock.calls).toEqual([]); @@ -927,12 +895,8 @@ describe('integration tests required on features', () => { ]; // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockRemoveLabel.mock.calls).toEqual([]); @@ -956,12 +920,8 @@ describe('integration tests required on features', () => { ]; // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockRemoveLabel.mock.calls[0][0]).toEqual({ @@ -986,12 +946,8 @@ describe('integration tests required on features', () => { (pr as any).labels = []; // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockRemoveLabel.mock.calls).toEqual([]); @@ -1019,12 +975,8 @@ describe('integration tests required on features', () => { ]; // WHEN - const prLinter = configureMock(pr); - await legacyValidateStatusEvent(prLinter, { - sha: SHA, - context: CODE_BUILD_CONTEXT, - state: 'success', - } as any); + const prLinter = configureMock(pr, files); + await legacyValidatePullRequestTarget(prLinter); // THEN expect(mockRemoveLabel.mock.calls).toEqual([]); @@ -1367,12 +1319,6 @@ function configureMock(pr: Subset, prFiles?: GitHubFileName[], existin }); octomock.issues.addLabels = mockAddLabel; octomock.issues.removeLabel = mockRemoveLabel; - octomock.repos.listCommitStatusesForRef.mockImplementation(() => ({ - data: [{ - context: CODE_BUILD_CONTEXT, - state: 'success', - }], - })); // We need to pretend that all CodeCov checks are passing by default, otherwise // the linter will complain about these even in tests that aren't testing for this. @@ -1384,6 +1330,19 @@ function configureMock(pr: Subset, prFiles?: GitHubFileName[], existin })), })); + // Mock workflow runs to simulate successful CodeBuild jobs + octomock.actions.listWorkflowRuns.mockImplementation(() => ({ + data: { + workflow_runs: [ + { + head_sha: SHA, + status: 'completed', + conclusion: 'success', + }, + ], + }, + })); + const linter = new PullRequestLinter({ owner: 'aws', repo: 'aws-cdk', @@ -1420,13 +1379,3 @@ async function legacyValidatePullRequestTarget(prLinter: PullRequestLinter) { await prLinter.executeActions(actions); prLinter.actionsToException(actions); } - -/** - * Same as for validatePullRequesTarget - * - * @deprecated Assert on the contents of `LinterActions` instead. - */ -async function legacyValidateStatusEvent(prLinter: PullRequestLinter, statusEvent: StatusEvent) { - const actions = await prLinter.validateStatusEvent(statusEvent); - await prLinter.executeActions(actions); -} diff --git a/tools/@aws-cdk/prlint/test/linter-base.test.ts b/tools/@aws-cdk/prlint/test/linter-base.test.ts index 950a6559c4916..6e8329592d099 100644 --- a/tools/@aws-cdk/prlint/test/linter-base.test.ts +++ b/tools/@aws-cdk/prlint/test/linter-base.test.ts @@ -31,7 +31,7 @@ beforeEach(() => { }, }); octomock.pulls.listReviews.mockReturnValue({ data: [] }); - octomock.repos.listCommitStatusesForRef.mockReturnValue({ data: [] }); + octomock.actions.listWorkflowRuns.mockReturnValue({ data: [] }); }); test('ignore if dismissing reviews throws a specific "already dismissed" error', async () => { diff --git a/tools/@aws-cdk/prlint/test/octomock.ts b/tools/@aws-cdk/prlint/test/octomock.ts index d24e2578be079..5ce4cb55693e3 100644 --- a/tools/@aws-cdk/prlint/test/octomock.ts +++ b/tools/@aws-cdk/prlint/test/octomock.ts @@ -23,12 +23,12 @@ export function createOctomock() { search: { issuesAndPullRequests: jest.fn(), }, - repos: { - listCommitStatusesForRef: jest.fn(), - }, checks: { listForRef: jest.fn(), }, + actions: { + listWorkflowRuns: jest.fn(), + }, paginate: async (method: any, args: any) => { return (await method(args)).data; }, }; }