Skip to content

Commit edae464

Browse files
authored
chore(prlint): add need-xxx-reviews when building with github action (#35326)
### Issue # (if applicable) Closes #35268. ### Reason for this change When moving the builds to github actions, `need-xxx-review` labels were not added anymore. ### Description of changes Fetch the build status from the github action, remove support for `StatusEvent` Github actions don't use it (only the direct Codebuild integration used it) ### Description of how you validated changes Manually ran the script for a few PR's locally, updated unit tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent fc2fc9d commit edae464

File tree

7 files changed

+66
-144
lines changed

7 files changed

+66
-144
lines changed

.github/workflows/pr-linter.yml

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,10 @@ on:
1414

1515
# Triggered from a separate job when a review is added
1616
workflow_run:
17-
workflows: [PR Linter Trigger]
17+
workflows: ["PR Linter Trigger", "Codebuild PR Build"]
1818
types:
1919
- completed
2020

21-
# Trigger when a status is updated (CodeBuild leads to statuses)
22-
status: {}
23-
2421
# Trigger when a check suite is completed (GitHub actions and CodeCov create checks)
2522
check_suite:
2623
types: [completed]

tools/@aws-cdk/prlint/constants.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11

22
export const DEFAULT_LINTER_LOGIN = 'aws-cdk-automation';
33

4-
export const CODE_BUILD_CONTEXT = 'AWS CodeBuild us-east-1 (AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv)';
4+
export const CODE_BUILD_WORKFLOW_FILE = 'codebuild-pr-build.yml';
55

66
/**
77
* Types of exemption labels in aws-cdk project.

tools/@aws-cdk/prlint/index.ts

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ import * as github from '@actions/github';
22
import { Octokit } from '@octokit/rest';
33
import { StatusEvent, PullRequestEvent, CheckSuiteEvent } from '@octokit/webhooks-definitions/schema';
44
import { PullRequestLinter } from './lint';
5-
import { LinterActions } from './linter-base';
65
import { DEFAULT_LINTER_LOGIN } from './constants';
76

87
/**
@@ -42,23 +41,7 @@ async function run() {
4241
linterLogin: process.env.LINTER_LOGIN || DEFAULT_LINTER_LOGIN,
4342
});
4443

45-
let actions: LinterActions | undefined;
46-
47-
switch (github.context.eventName) {
48-
case 'status':
49-
// Triggering on a 'status' event is solely used to see if the CodeBuild
50-
// job just turned green, and adding certain 'ready for review' labels
51-
// if it does.
52-
const statusPayload = github.context.payload as StatusEvent;
53-
console.log('validating status event');
54-
actions = await prLinter.validateStatusEvent(statusPayload);
55-
break;
56-
57-
default:
58-
// This is the main PR validator action.
59-
actions = await prLinter.validatePullRequestTarget();
60-
break;
61-
}
44+
const actions = await prLinter.validatePullRequestTarget();
6245

6346
if (actions) {
6447
console.log(actions);

tools/@aws-cdk/prlint/lint.ts

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,7 @@ import { findModulePath, moduleStability } from './module';
33
import { breakingModules } from './parser';
44
import { CheckRun, GitHubFile, GitHubPr, Review, sumChanges, summarizeRunConclusions } from "./github";
55
import { TestResult, ValidationCollector } from './results';
6-
import { CODE_BUILD_CONTEXT, CODECOV_CHECKS, Exemption } from './constants';
7-
import { StatusEvent } from '@octokit/webhooks-definitions/schema';
6+
import { CODE_BUILD_WORKFLOW_FILE, CODECOV_CHECKS, Exemption } from './constants';
87
import { LinterActions, mergeLinterActions, PR_FROM_MAIN_ERROR, PullRequestLinterBase } from './linter-base';
98

109
/**
@@ -20,21 +19,15 @@ export class PullRequestLinter extends PullRequestLinterBase {
2019
* @param sha the commit sha to evaluate
2120
*/
2221
private async codeBuildJobSucceeded(sha: string): Promise<boolean> {
23-
const statuses = await this.client.repos.listCommitStatusesForRef({
22+
const statuses = await this.client.actions.listWorkflowRuns({
2423
owner: this.prParams.owner,
2524
repo: this.prParams.repo,
26-
ref: sha,
25+
head_sha: sha,
26+
workflow_id: CODE_BUILD_WORKFLOW_FILE,
2727
});
28-
let status = statuses.data.filter(status => status.context === CODE_BUILD_CONTEXT).map(status => status.state);
29-
console.log("CodeBuild Commit Statuses: ", status);
30-
return statuses.data.some(status => status.context === CODE_BUILD_CONTEXT && status.state === 'success');
31-
}
32-
33-
public async validateStatusEvent(status: StatusEvent): Promise<LinterActions> {
34-
if (status.context === CODE_BUILD_CONTEXT && status.state === 'success') {
35-
return this.assessNeedsReview();
36-
}
37-
return {};
28+
let conclusions = statuses.data.workflow_runs.filter(run => run.status === 'completed').map(run => run.conclusion);
29+
console.log("CodeBuild Commit Conclusions: ", conclusions);
30+
return conclusions.some(conclusion => conclusion === 'success');
3831
}
3932

4033
/**

tools/@aws-cdk/prlint/test/lint.test.ts

Lines changed: 52 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
import * as path from 'path';
22
import { GitHubFile, GitHubLabel, GitHubPr } from '../github';
3-
import { CODE_BUILD_CONTEXT, CODECOV_CHECKS } from '../constants';
3+
import { CODECOV_CHECKS } from '../constants';
44
import { PullRequestLinter } from '../lint';
5-
import { StatusEvent } from '@octokit/webhooks-definitions/schema';
65
import { createOctomock, OctoMock } from './octomock';
76

87
type GitHubFileName = Omit<GitHubFile, 'deletions' | 'additions'>;
@@ -589,13 +588,22 @@ describe('integration tests required on features', () => {
589588
});
590589
});
591590

592-
describe('assess needs review from status event', () => {
591+
describe('assess needs review', () => {
593592
const pr = {
593+
title: 'chore(s3): something',
594594
draft: false,
595595
mergeable_state: 'behind',
596596
number: 1234,
597597
labels: [{ name: 'p2' }],
598598
};
599+
const files = [
600+
{
601+
filename: 'some-test.test.ts',
602+
},
603+
{
604+
filename: 'readme.md',
605+
},
606+
];
599607
beforeEach(() => {
600608
mockListReviews.mockImplementation(() => {
601609
return {
@@ -606,12 +614,8 @@ describe('integration tests required on features', () => {
606614

607615
test('needs a review', async () => {
608616
// WHEN
609-
const prLinter = configureMock(pr);
610-
await legacyValidateStatusEvent(prLinter, {
611-
sha: SHA,
612-
context: CODE_BUILD_CONTEXT,
613-
state: 'success',
614-
} as any);
617+
const prLinter = configureMock(pr, files);
618+
await legacyValidatePullRequestTarget(prLinter);
615619

616620
// THEN
617621
expect(mockAddLabel.mock.calls[0][0]).toEqual({
@@ -626,12 +630,8 @@ describe('integration tests required on features', () => {
626630
test('needs a review and is p1', async () => {
627631
// WHEN
628632
pr.labels = [{ name: 'p1' }];
629-
const prLinter = configureMock(pr);
630-
await legacyValidateStatusEvent(prLinter, {
631-
sha: SHA,
632-
context: CODE_BUILD_CONTEXT,
633-
state: 'success',
634-
} as any);
633+
const prLinter = configureMock(pr, files);
634+
await legacyValidatePullRequestTarget(prLinter);
635635

636636
// THEN
637637
expect(mockAddLabel.mock.calls[0][0]).toEqual({
@@ -657,12 +657,8 @@ describe('integration tests required on features', () => {
657657
];
658658

659659
// WHEN
660-
const prLinter = configureMock(pr);
661-
await legacyValidateStatusEvent(prLinter, {
662-
sha: SHA,
663-
context: CODE_BUILD_CONTEXT,
664-
state: 'success',
665-
} as any);
660+
const prLinter = configureMock(pr, files);
661+
await legacyValidatePullRequestTarget(prLinter);
666662

667663
// THEN
668664
expect(mockRemoveLabel.mock.calls[0][0]).toEqual({
@@ -688,12 +684,8 @@ describe('integration tests required on features', () => {
688684
];
689685

690686
// WHEN
691-
const prLinter = configureMock(pr);
692-
await legacyValidateStatusEvent(prLinter, {
693-
sha: SHA,
694-
context: CODE_BUILD_CONTEXT,
695-
state: 'success',
696-
} as any);
687+
const prLinter = configureMock(pr, files);
688+
await legacyValidatePullRequestTarget(prLinter);
697689

698690
// THEN
699691
expect(mockAddLabel.mock.calls[0][0]).toEqual({
@@ -725,12 +717,8 @@ describe('integration tests required on features', () => {
725717
];
726718

727719
// WHEN
728-
const prLinter = configureMock(pr);
729-
await legacyValidateStatusEvent(prLinter, {
730-
sha: SHA,
731-
context: CODE_BUILD_CONTEXT,
732-
state: 'success',
733-
} as any);
720+
const prLinter = configureMock(pr, files);
721+
await legacyValidatePullRequestTarget(prLinter);
734722

735723
// THEN
736724
expect(mockRemoveLabel.mock.calls[0][0]).toEqual({
@@ -758,12 +746,8 @@ describe('integration tests required on features', () => {
758746
];
759747

760748
// WHEN
761-
const prLinter = configureMock(pr);
762-
await legacyValidateStatusEvent(prLinter, {
763-
sha: SHA,
764-
context: CODE_BUILD_CONTEXT,
765-
state: 'success',
766-
} as any);
749+
const prLinter = configureMock(pr, files);
750+
await legacyValidatePullRequestTarget(prLinter);
767751

768752
// THEN
769753
expect(mockRemoveLabel.mock.calls[0][0]).toEqual({
@@ -791,12 +775,8 @@ describe('integration tests required on features', () => {
791775
];
792776

793777
// WHEN
794-
const prLinter = configureMock(pr);
795-
await legacyValidateStatusEvent(prLinter, {
796-
sha: SHA,
797-
context: CODE_BUILD_CONTEXT,
798-
state: 'success',
799-
} as any);
778+
const prLinter = configureMock(pr, files);
779+
await legacyValidatePullRequestTarget(prLinter);
800780

801781
// THEN
802782
expect(mockRemoveLabel.mock.calls[0][0]).toEqual({
@@ -831,12 +811,8 @@ describe('integration tests required on features', () => {
831811
];
832812

833813
// WHEN
834-
const prLinter = configureMock(pr);
835-
await legacyValidateStatusEvent(prLinter, {
836-
sha: SHA,
837-
context: CODE_BUILD_CONTEXT,
838-
state: 'success',
839-
} as any);
814+
const prLinter = configureMock(pr, files);
815+
await legacyValidatePullRequestTarget(prLinter);
840816

841817
// THEN
842818
expect(mockRemoveLabel.mock.calls[0][0]).toEqual({
@@ -869,12 +845,8 @@ describe('integration tests required on features', () => {
869845
];
870846

871847
// WHEN
872-
const prLinter = configureMock(pr);
873-
await legacyValidateStatusEvent(prLinter, {
874-
sha: SHA,
875-
context: CODE_BUILD_CONTEXT,
876-
state: 'success',
877-
} as any);
848+
const prLinter = configureMock(pr, files);
849+
await legacyValidatePullRequestTarget(prLinter);
878850

879851
// THEN
880852
expect(mockRemoveLabel.mock.calls[0][0]).toEqual({
@@ -899,12 +871,8 @@ describe('integration tests required on features', () => {
899871
(pr as any).labels = [];
900872

901873
// WHEN
902-
const prLinter = configureMock(pr);
903-
await legacyValidateStatusEvent(prLinter, {
904-
sha: SHA,
905-
context: CODE_BUILD_CONTEXT,
906-
state: 'success',
907-
} as any);
874+
const prLinter = configureMock(pr, files);
875+
await legacyValidatePullRequestTarget(prLinter);
908876

909877
// THEN
910878
expect(mockRemoveLabel.mock.calls).toEqual([]);
@@ -927,12 +895,8 @@ describe('integration tests required on features', () => {
927895
];
928896

929897
// WHEN
930-
const prLinter = configureMock(pr);
931-
await legacyValidateStatusEvent(prLinter, {
932-
sha: SHA,
933-
context: CODE_BUILD_CONTEXT,
934-
state: 'success',
935-
} as any);
898+
const prLinter = configureMock(pr, files);
899+
await legacyValidatePullRequestTarget(prLinter);
936900

937901
// THEN
938902
expect(mockRemoveLabel.mock.calls).toEqual([]);
@@ -956,12 +920,8 @@ describe('integration tests required on features', () => {
956920
];
957921

958922
// WHEN
959-
const prLinter = configureMock(pr);
960-
await legacyValidateStatusEvent(prLinter, {
961-
sha: SHA,
962-
context: CODE_BUILD_CONTEXT,
963-
state: 'success',
964-
} as any);
923+
const prLinter = configureMock(pr, files);
924+
await legacyValidatePullRequestTarget(prLinter);
965925

966926
// THEN
967927
expect(mockRemoveLabel.mock.calls[0][0]).toEqual({
@@ -986,12 +946,8 @@ describe('integration tests required on features', () => {
986946
(pr as any).labels = [];
987947

988948
// WHEN
989-
const prLinter = configureMock(pr);
990-
await legacyValidateStatusEvent(prLinter, {
991-
sha: SHA,
992-
context: CODE_BUILD_CONTEXT,
993-
state: 'success',
994-
} as any);
949+
const prLinter = configureMock(pr, files);
950+
await legacyValidatePullRequestTarget(prLinter);
995951

996952
// THEN
997953
expect(mockRemoveLabel.mock.calls).toEqual([]);
@@ -1019,12 +975,8 @@ describe('integration tests required on features', () => {
1019975
];
1020976

1021977
// WHEN
1022-
const prLinter = configureMock(pr);
1023-
await legacyValidateStatusEvent(prLinter, {
1024-
sha: SHA,
1025-
context: CODE_BUILD_CONTEXT,
1026-
state: 'success',
1027-
} as any);
978+
const prLinter = configureMock(pr, files);
979+
await legacyValidatePullRequestTarget(prLinter);
1028980

1029981
// THEN
1030982
expect(mockRemoveLabel.mock.calls).toEqual([]);
@@ -1367,12 +1319,6 @@ function configureMock(pr: Subset<GitHubPr>, prFiles?: GitHubFileName[], existin
13671319
});
13681320
octomock.issues.addLabels = mockAddLabel;
13691321
octomock.issues.removeLabel = mockRemoveLabel;
1370-
octomock.repos.listCommitStatusesForRef.mockImplementation(() => ({
1371-
data: [{
1372-
context: CODE_BUILD_CONTEXT,
1373-
state: 'success',
1374-
}],
1375-
}));
13761322

13771323
// We need to pretend that all CodeCov checks are passing by default, otherwise
13781324
// the linter will complain about these even in tests that aren't testing for this.
@@ -1384,6 +1330,19 @@ function configureMock(pr: Subset<GitHubPr>, prFiles?: GitHubFileName[], existin
13841330
})),
13851331
}));
13861332

1333+
// Mock workflow runs to simulate successful CodeBuild jobs
1334+
octomock.actions.listWorkflowRuns.mockImplementation(() => ({
1335+
data: {
1336+
workflow_runs: [
1337+
{
1338+
head_sha: SHA,
1339+
status: 'completed',
1340+
conclusion: 'success',
1341+
},
1342+
],
1343+
},
1344+
}));
1345+
13871346
const linter = new PullRequestLinter({
13881347
owner: 'aws',
13891348
repo: 'aws-cdk',
@@ -1420,13 +1379,3 @@ async function legacyValidatePullRequestTarget(prLinter: PullRequestLinter) {
14201379
await prLinter.executeActions(actions);
14211380
prLinter.actionsToException(actions);
14221381
}
1423-
1424-
/**
1425-
* Same as for validatePullRequesTarget
1426-
*
1427-
* @deprecated Assert on the contents of `LinterActions` instead.
1428-
*/
1429-
async function legacyValidateStatusEvent(prLinter: PullRequestLinter, statusEvent: StatusEvent) {
1430-
const actions = await prLinter.validateStatusEvent(statusEvent);
1431-
await prLinter.executeActions(actions);
1432-
}

tools/@aws-cdk/prlint/test/linter-base.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ beforeEach(() => {
3131
},
3232
});
3333
octomock.pulls.listReviews.mockReturnValue({ data: [] });
34-
octomock.repos.listCommitStatusesForRef.mockReturnValue({ data: [] });
34+
octomock.actions.listWorkflowRuns.mockReturnValue({ data: [] });
3535
});
3636

3737
test('ignore if dismissing reviews throws a specific "already dismissed" error', async () => {

0 commit comments

Comments
 (0)