Skip to content

Commit

Permalink
chore: stop failing the PR linter on changes requested (#32987)
Browse files Browse the repository at this point in the history
Almost every PR immediately looks like it's failing with a red cross, because the PR linter fails if it is requesting changes.

The "Changes Requested" review by itself is enough to prevent a PR from getting merged by the Mergify config, so we don't actually need to fail the PR linter as well.

Instead: the PR linter succeeds if it runs to the end, and it may request changes on the PR. If it fails, then it's because it was unable to do its job for some reason (that should and will still block merging, so we are not accidentally failing open if something is wrong with the linter).

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored Jan 17, 2025
1 parent 680b6ba commit 2afdf25
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 41 deletions.
6 changes: 6 additions & 0 deletions .mergify.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ queue_rules:
- -closed
- "#approved-reviews-by>=1"
- -approved-reviews-by~=author
# This is important! It makes the PR Linter work.
- "#changes-requested-reviews-by=0"
- status-success~=AWS CodeBuild us-east-1
- status-success=validate-pr
Expand All @@ -30,6 +31,7 @@ queue_rules:
- -closed
- "#approved-reviews-by>=1"
- -approved-reviews-by~=author
# This is important! It makes the PR Linter work.
- "#changes-requested-reviews-by=0"
- status-success~=AWS CodeBuild us-east-1
- status-success=validate-pr
Expand Down Expand Up @@ -61,6 +63,7 @@ pull_request_rules:
- author!=dependabot-preview[bot]
- "#approved-reviews-by>=1"
- -approved-reviews-by~=author
# This is important! It makes the PR Linter work.
- "#changes-requested-reviews-by=0"
- status-success~=AWS CodeBuild us-east-1
- status-success=validate-pr
Expand All @@ -81,6 +84,7 @@ pull_request_rules:
- author!=dependabot-preview[bot]
- "#approved-reviews-by>=2"
- -approved-reviews-by~=author
# This is important! It makes the PR Linter work.
- "#changes-requested-reviews-by=0"
- status-success~=AWS CodeBuild us-east-1
- status-success=validate-pr
Expand All @@ -101,6 +105,7 @@ pull_request_rules:
- author!=dependabot-preview[bot]
- "#approved-reviews-by>=1"
- -approved-reviews-by~=author
# This is important! It makes the PR Linter work.
- "#changes-requested-reviews-by=0"
- status-success~=AWS CodeBuild us-east-1
- status-success=validate-pr
Expand Down Expand Up @@ -140,6 +145,7 @@ pull_request_rules:
- -closed
- author~=dependabot
- "#approved-reviews-by>=1"
# This is important! It makes the PR Linter work.
- "#changes-requested-reviews-by=0"
- status-success~=AWS CodeBuild us-east-1
- status-success=validate-pr
73 changes: 32 additions & 41 deletions tools/@aws-cdk/prlint/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import * as core from '@actions/core';
import * as github from '@actions/github';
import { Octokit } from '@octokit/rest';
import { StatusEvent, PullRequestEvent } from '@octokit/webhooks-definitions/schema';
Expand Down Expand Up @@ -33,50 +32,42 @@ async function run() {
throw new Error(`Could not find PR number from event: ${github.context.eventName}`);
}

try {
const prLinter = new PullRequestLinter({
client,
owner,
repo,
number,
// On purpose || instead of ??, also collapse empty string
linterLogin: process.env.LINTER_LOGIN || DEFEAULT_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;
}

if (actions) {
console.log(actions);
const prLinter = new PullRequestLinter({
client,
owner,
repo,
number,
// On purpose || instead of ??, also collapse empty string
linterLogin: process.env.LINTER_LOGIN || DEFEAULT_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;
}

if (github.context.eventName || process.env.FOR_REAL) {
console.log('Carrying out actions');
if (actions) {
console.log(actions);

// Actually running in GitHub actions, do it (otherwise we're just testing)
await prLinter.executeActions(actions);
}
if (github.context.eventName || process.env.FOR_REAL) {
console.log('Carrying out actions');

await prLinter.actionsToException(actions);
// Actually running in GitHub actions, do it (otherwise we're just testing)
await prLinter.executeActions(actions);
}

} catch (error: any) {
// Fail the action
core.setFailed(error.message);
}
}

Expand Down

0 comments on commit 2afdf25

Please sign in to comment.