From db4d04928004638fa6b87dc71d1544deacb42355 Mon Sep 17 00:00:00 2001 From: MatthewPopat-NHS Date: Wed, 29 Apr 2026 14:25:07 +0000 Subject: [PATCH 1/3] Added support for cleaning up arbitrary PR stacks --- .../src/stacks/deleteUnusedStacks.ts | 2 +- .../tests/stacks/deleteUnusedStacks.test.ts | 42 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts b/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts index d20dea22..2c465100 100644 --- a/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts +++ b/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts @@ -189,7 +189,7 @@ async function getHostedZoneInfo( } async function isClosedPullRequest(stackName: string, baseStackName: string, repoName: string): Promise { - const match = new RegExp(String.raw`^${baseStackName}-pr-(?\d+)(-sandbox)?$`).exec(stackName) + const match = new RegExp(String.raw`^${baseStackName}-pr-(?\d+)(-[\w-]+)?$`).exec(stackName) if (!match?.groups?.pullRequestId) { return false } diff --git a/packages/cdkConstructs/tests/stacks/deleteUnusedStacks.test.ts b/packages/cdkConstructs/tests/stacks/deleteUnusedStacks.test.ts index ecb88239..5a583dde 100644 --- a/packages/cdkConstructs/tests/stacks/deleteUnusedStacks.test.ts +++ b/packages/cdkConstructs/tests/stacks/deleteUnusedStacks.test.ts @@ -858,6 +858,48 @@ describe("stack deletion", () => { }) }) + test("deletes PR stacks with multiple suffixes", async () => { + const now = new Date() + const twoDaysAgo = new Date(now.getTime() - 2 * 24 * 60 * 60 * 1000) + + mockListStacksSend.mockReturnValue({ + StackSummaries: [ + { + StackName: `${baseStackName}-pr-123-sandbox`, + StackStatus: "CREATE_COMPLETE", + CreationTime: twoDaysAgo + }, + { + StackName: `${baseStackName}-pr-123-front-door`, + StackStatus: "CREATE_COMPLETE", + CreationTime: twoDaysAgo + }, + { + StackName: `${baseStackName}-pr-123-stateful`, + StackStatus: "CREATE_COMPLETE", + CreationTime: twoDaysAgo + } + ] + }) + + mockGetPRState.mockImplementation((url: string) => { + if (url.endsWith("/repos/NHSDigital/eps-cdk-utils/pulls/123")) { + return "closed" + } + throw new Error(`Unexpected URL: ${url}`) + }) + + const promise = deleteUnusedPrStacks(baseStackName, repoName, hostedZoneName) + await vi.runAllTimersAsync() + await promise + + // One delete stack call for the PR stack + expect(mockDeleteStackSend).toHaveBeenCalledTimes(3) + expect(mockDeleteStackSend).toHaveBeenCalledWith({StackName: `${baseStackName}-pr-123-sandbox`}) + expect(mockDeleteStackSend).toHaveBeenCalledWith({StackName: `${baseStackName}-pr-123-front-door`}) + expect(mockDeleteStackSend).toHaveBeenCalledWith({StackName: `${baseStackName}-pr-123-stateful`}) + }) + test("does not delete open PR stacks", async () => { const now = new Date() const twoDaysAgo = new Date(now.getTime() - 2 * 24 * 60 * 60 * 1000) From babf5368f3bcbff52fbbc440dbf5da82b04770fe Mon Sep 17 00:00:00 2001 From: MatthewPopat-NHS Date: Wed, 29 Apr 2026 14:50:28 +0000 Subject: [PATCH 2/3] Added PR state memoization --- .../src/stacks/deleteUnusedStacks.ts | 49 +++++++++++++------ .../tests/stacks/deleteUnusedStacks.test.ts | 5 +- 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts b/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts index 2c465100..4c370e91 100644 --- a/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts +++ b/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts @@ -83,6 +83,7 @@ export async function deleteUnusedPrStacks( const cloudFormationClient = new CloudFormationClient({}) const route53Client = new Route53Client({}) const {hostedZoneId, cnameRecords} = await getHostedZoneInfo(route53Client, hostedZoneName) + const pullRequestStateCache = new Map() console.log("checking cloudformation stacks") @@ -94,7 +95,7 @@ export async function deleteUnusedPrStacks( } const stackName = stack.StackName - if (!(await isClosedPullRequest(stackName, baseStackName, repoName))) { + if (!(await isClosedPullRequest(stackName, baseStackName, repoName, pullRequestStateCache))) { continue } @@ -188,34 +189,50 @@ async function getHostedZoneInfo( return {hostedZoneId, cnameRecords} } -async function isClosedPullRequest(stackName: string, baseStackName: string, repoName: string): Promise { +async function isClosedPullRequest( + stackName: string, + baseStackName: string, + repoName: string, + pullRequestStateCache: Map +): Promise { const match = new RegExp(String.raw`^${baseStackName}-pr-(?\d+)(-[\w-]+)?$`).exec(stackName) if (!match?.groups?.pullRequestId) { return false } const pullRequestId = match.groups.pullRequestId - console.log(`Checking pull request id ${pullRequestId}`) - const url = `https://api.github.com/repos/NHSDigital/${repoName}/pulls/${pullRequestId}` + let pullRequestState = pullRequestStateCache.get(pullRequestId) + if (pullRequestState === undefined) { - const headers: Record = { - Accept: "application/vnd.github+json", - Authorization: `Bearer ${process.env.GITHUB_TOKEN}` - } + console.log(`Checking pull request id ${pullRequestId}`) + const url = `https://api.github.com/repos/NHSDigital/${repoName}/pulls/${pullRequestId}` - const response = await fetch(url, {headers}) - if (!response.ok) { - console.log(`Failed to fetch PR ${pullRequestId}: ${response.status} ${await response.text()}`) - return false + const headers: Record = { + Accept: "application/vnd.github+json", + Authorization: `Bearer ${process.env.GITHUB_TOKEN}` + } + + const response = await fetch(url, {headers}) + if (!response.ok) { + console.log(`Failed to fetch PR ${pullRequestId}: ${response.status} ${await response.text()}`) + // To avoid accidentally deleting stacks due to transient API failures, we treat errors as non-closed state + // but do not cache the result to allow for retries on subsequent runs + return false + } + + const data = (await response.json()) as {state?: string} + pullRequestState = data.state + if (pullRequestState) { + pullRequestStateCache.set(pullRequestId, pullRequestState) + } } - const data = (await response.json()) as {state?: string} - if (data.state !== "closed") { - console.log(`not going to delete stack ${stackName} as PR state is ${data.state}`) + if (pullRequestState !== "closed") { + console.log(`not going to delete stack ${stackName} as PR state is ${pullRequestState}`) return false } - console.log(`** going to delete stack ${stackName} as PR state is ${data.state} **`) + console.log(`** going to delete stack ${stackName} as PR state is ${pullRequestState} **`) return true } diff --git a/packages/cdkConstructs/tests/stacks/deleteUnusedStacks.test.ts b/packages/cdkConstructs/tests/stacks/deleteUnusedStacks.test.ts index 5a583dde..b832d157 100644 --- a/packages/cdkConstructs/tests/stacks/deleteUnusedStacks.test.ts +++ b/packages/cdkConstructs/tests/stacks/deleteUnusedStacks.test.ts @@ -893,11 +893,14 @@ describe("stack deletion", () => { await vi.runAllTimersAsync() await promise - // One delete stack call for the PR stack + // One delete stack call per PR stack expect(mockDeleteStackSend).toHaveBeenCalledTimes(3) expect(mockDeleteStackSend).toHaveBeenCalledWith({StackName: `${baseStackName}-pr-123-sandbox`}) expect(mockDeleteStackSend).toHaveBeenCalledWith({StackName: `${baseStackName}-pr-123-front-door`}) expect(mockDeleteStackSend).toHaveBeenCalledWith({StackName: `${baseStackName}-pr-123-stateful`}) + + // PR state should only be fetched once per PR, not once per stack + expect(mockGetPRState).toHaveBeenCalledTimes(1) }) test("does not delete open PR stacks", async () => { From 2a034d2ffb5c63ad960dc906cf299c11d241f244 Mon Sep 17 00:00:00 2001 From: MatthewPopat-NHS <95283781+MatthewPopat-NHS@users.noreply.github.com> Date: Wed, 29 Apr 2026 16:00:30 +0100 Subject: [PATCH 3/3] fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts b/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts index 4c370e91..e2660f95 100644 --- a/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts +++ b/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts @@ -216,7 +216,7 @@ async function isClosedPullRequest( if (!response.ok) { console.log(`Failed to fetch PR ${pullRequestId}: ${response.status} ${await response.text()}`) // To avoid accidentally deleting stacks due to transient API failures, we treat errors as non-closed state - // but do not cache the result to allow for retries on subsequent runs + // and do not cache the result, allowing another fetch attempt later in this run if needed. return false }