Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: fix PRlinter dequeuing PRs because of CodeCov #33137

Merged
merged 1 commit into from
Jan 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 8 additions & 16 deletions tools/@aws-cdk/prlint/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,25 +41,17 @@ export interface GitHubFile {
* Returns `success` if they all return a positive result, `failure` if
* one of them failed for some reason, and `waiting` if the result isn't available
* yet.
*
* 'failure' takes precedence over 'waiting' if there's any reason for it.
*/
export function summarizeRunConclusions(conclusions: Array<CheckRun['conclusion'] | undefined>): 'success' | 'failure' | 'waiting' {
for (const concl of conclusions) {
switch (concl) {
case null:
case undefined:
case 'action_required':
return 'waiting';

case 'failure':
case 'cancelled':
case 'timed_out':
return 'failure';
if (conclusions.some(c => ['failure', 'cancelled', 'timed_out'].includes(c ?? ''))) {
return 'failure';
}

case 'neutral':
case 'skipped':
case 'success':
break;
}
if (conclusions.some(c => c === 'action_required' || c === null || c === undefined)) {
return 'waiting';
}

return 'success';
}
21 changes: 20 additions & 1 deletion tools/@aws-cdk/prlint/lint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,26 @@ export class PullRequestLinter extends PullRequestLinterBase {

switch (summary) {
case 'failure': return TestResult.failure('CodeCov is indicating a drop in code coverage');
case 'waiting': return TestResult.failure('Still waiting for CodeCov results (make sure the build is passing first)');
// If we don't know the result of the CodeCov results yet, we pretend that there isn't a problem.
//
// It would be safer to ask for changes until we're confident that CodeCov has passed, but if we do
// that the following sequence of events happens:
//
// 1. PR is ready to be merged (approved, everything passes)
// 2. Mergify enqueues it and merges from main
// 3. CodeCov needs to run again
// 4. PR linter requests changes because CodeCov result is uncertain
// 5. Mergify dequeues the PR because PR linter requests changes
//
// This looks very confusing and noisy, and also will never fix itself, so the PR ends up unmerged.
//
// The better solution would probably be not to do a "Request Changes" review, but leave a comment
// and create a GitHub "status" on the PR to say 'success/pending/failure', and make it required.
// (https://github.com/aws/aws-cdk/issues/33136)
//
// For now, not doing anything with a 'waiting' status is a smaller delta, and the race condition posed by it is
// unlikely to happen given that there are much slower jobs that the merge is blocked on anyway.
case 'waiting': return TestResult.success();
case 'success': return TestResult.success();
}
},
Expand Down
9 changes: 4 additions & 5 deletions tools/@aws-cdk/prlint/test/lint.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1183,7 +1183,7 @@ describe('for any PR', () => {
};

const ARBITRARY_FILES: GitHubFile[] = [{
filename: 'packages/aws-cdk-lib/region-info/build-tools/metadata.ts',
filename: 'README.md',
}];

test('deletes old comments', async () => {
Expand Down Expand Up @@ -1212,7 +1212,8 @@ describe('for any PR', () => {
}));
});

test('missing CodeCov runs lead to a failure', async () => {
test('missing CodeCov run does not lead to request changes', async () => {
// Not ideal, but https://github.com/aws/aws-cdk/issues/33136
// GIVEN
const prLinter = configureMock(ARBITRARY_PR, ARBITRARY_FILES);
prLinter.octomock.checks.listForRef.mockReturnValue({ data: [] });
Expand All @@ -1221,9 +1222,7 @@ describe('for any PR', () => {
const result = await prLinter.validatePullRequestTarget();

// THEN
expect(result.requestChanges?.failures).toContainEqual(
expect.stringContaining('Still waiting for CodeCov results'),
);
expect(result.requestChanges).toBeUndefined();
});

test('failing CodeCov runs lead to a failure', async () => {
Expand Down
Loading