diff --git a/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts b/packages/cdkConstructs/src/stacks/deleteUnusedStacks.ts index d20dea22..e2660f95 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 { - const match = new RegExp(String.raw`^${baseStackName}-pr-(?\d+)(-sandbox)?$`).exec(stackName) +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 + // and do not cache the result, allowing another fetch attempt later in this run if needed. + 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 ecb88239..b832d157 100644 --- a/packages/cdkConstructs/tests/stacks/deleteUnusedStacks.test.ts +++ b/packages/cdkConstructs/tests/stacks/deleteUnusedStacks.test.ts @@ -858,6 +858,51 @@ 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 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 () => { const now = new Date() const twoDaysAgo = new Date(now.getTime() - 2 * 24 * 60 * 60 * 1000)