From 9e5411d955f17d0fcac6bc7eddf43c4c4ac49ff6 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 24 Jan 2025 14:20:25 +0100 Subject: [PATCH] chore: fix PRlinter dequeuing PRs because of CodeCov If we don't know the result of the CodeCov results yet, we used to ask for changes, because it prevents merging while the check might still fail in the future. The following sequence of events happens because of this: 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. See also #33136. --- tools/@aws-cdk/prlint/github.ts | 24 ++++++++---------------- tools/@aws-cdk/prlint/lint.ts | 21 ++++++++++++++++++++- tools/@aws-cdk/prlint/test/lint.test.ts | 9 ++++----- 3 files changed, 32 insertions(+), 22 deletions(-) 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 () => {