diff --git a/tools/@aws-cdk/prlint/github.ts b/tools/@aws-cdk/prlint/github.ts index 8e162cba46ae5..a906e2d985914 100644 --- a/tools/@aws-cdk/prlint/github.ts +++ b/tools/@aws-cdk/prlint/github.ts @@ -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): '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'; } \ No newline at end of file diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index b7779ed3794a7..d44f8134fec6e 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -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(); } }, diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index d26299b3090bf..748d3581fb545 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -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 () => { @@ -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: [] }); @@ -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 () => {