From 12f812ab79fc1c7d62fde0d395171faf16fa3f3c Mon Sep 17 00:00:00 2001 From: akash1810 Date: Thu, 6 Nov 2025 09:04:37 +0000 Subject: [PATCH 1/3] refactor: Simplify singleton check `Construct` is a shared base type; use that to simplify some code. --- .../patterns/__snapshots__/ec2-app.test.ts.snap | 2 ++ src/experimental/patterns/ec2-app.ts | 17 +++++++++-------- src/utils/singleton.ts | 8 ++++---- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/experimental/patterns/__snapshots__/ec2-app.test.ts.snap b/src/experimental/patterns/__snapshots__/ec2-app.test.ts.snap index 0a90642955..1946db5efd 100644 --- a/src/experimental/patterns/__snapshots__/ec2-app.test.ts.snap +++ b/src/experimental/patterns/__snapshots__/ec2-app.test.ts.snap @@ -26,6 +26,8 @@ exports[`The GuEc2AppExperimental pattern matches the snapshot 1`] = ` "GuApplicationTargetGroup", "GuHttpsApplicationListener", "GuRiffRaffDeploymentIdParameter", + "GuAutoScalingRollingUpdateTimeoutExperimental", + "GuHorizontallyScalingDeploymentPropertiesExperimental", ], "gu:cdk:version": "TEST", }, diff --git a/src/experimental/patterns/ec2-app.ts b/src/experimental/patterns/ec2-app.ts index c8b0b9be9e..283c28009f 100644 --- a/src/experimental/patterns/ec2-app.ts +++ b/src/experimental/patterns/ec2-app.ts @@ -1,9 +1,11 @@ import type { IAspect } from "aws-cdk-lib"; +import { Stack } from "aws-cdk-lib"; import { Aspects, CfnParameter, Duration, Tags } from "aws-cdk-lib"; import { CfnAutoScalingGroup, CfnScalingPolicy, ScalingProcess, UpdatePolicy } from "aws-cdk-lib/aws-autoscaling"; import type { CfnPolicy } from "aws-cdk-lib/aws-iam"; import { Effect, Policy, PolicyStatement } from "aws-cdk-lib/aws-iam"; import type { IConstruct } from "constructs"; +import { Construct } from "constructs"; import { MetadataKeys } from "../../constants"; import { GuAutoScalingGroup } from "../../constructs/autoscaling"; import type { GuStack } from "../../constructs/core"; @@ -42,12 +44,11 @@ export const RollingUpdateDurations: AutoScalingRollingUpdateDurations = { * * TODO Expose the healthcheck grace period as a property on {@link GuEc2App} and remove this `Aspect`. */ -export class GuAutoScalingRollingUpdateTimeoutExperimental implements IAspect { - public readonly stack: GuStack; +export class GuAutoScalingRollingUpdateTimeoutExperimental extends Construct implements IAspect { private static instance: GuAutoScalingRollingUpdateTimeoutExperimental | undefined; private constructor(scope: GuStack) { - this.stack = scope; + super(scope, GuAutoScalingRollingUpdateTimeoutExperimental.name); } public static getInstance(stack: GuStack): GuAutoScalingRollingUpdateTimeoutExperimental { @@ -112,13 +113,12 @@ export class GuAutoScalingRollingUpdateTimeoutExperimental implements IAspect { * * @see https://github.com/guardian/testing-asg-rolling-update */ -export class GuHorizontallyScalingDeploymentPropertiesExperimental implements IAspect { - public readonly stack: GuStack; +export class GuHorizontallyScalingDeploymentPropertiesExperimental extends Construct implements IAspect { public readonly asgToParamMap: Map; private static instance: GuHorizontallyScalingDeploymentPropertiesExperimental | undefined; private constructor(scope: GuStack) { - this.stack = scope; + super(scope, GuHorizontallyScalingDeploymentPropertiesExperimental.name); this.asgToParamMap = new Map(); } @@ -131,7 +131,8 @@ export class GuHorizontallyScalingDeploymentPropertiesExperimental implements IA } public visit(construct: IConstruct) { - if (construct instanceof CfnScalingPolicy && construct.stack.stackName === this.stack.stackName) { + const stack = Stack.of(this); + if (construct instanceof CfnScalingPolicy && construct.stack === stack) { const { node } = construct; const { scopes, path } = node; @@ -174,7 +175,7 @@ export class GuHorizontallyScalingDeploymentPropertiesExperimental implements IA const cfnParameterName = getAsgRollingUpdateCfnParameterName(autoScalingGroup); this.asgToParamMap.set( asgNodeId, - new CfnParameter(this.stack, cfnParameterName, { + new CfnParameter(stack, cfnParameterName, { type: "Number", }), ); diff --git a/src/utils/singleton.ts b/src/utils/singleton.ts index a46067b06c..2f2686db58 100644 --- a/src/utils/singleton.ts +++ b/src/utils/singleton.ts @@ -1,4 +1,5 @@ -import type { Stack } from "aws-cdk-lib/core/lib/stack"; +import { Stack } from "aws-cdk-lib"; +import type { Construct } from "constructs"; import type { GuStack } from "../constructs/core"; /** @@ -35,7 +36,6 @@ import type { GuStack } from "../constructs/core"; * * @see https://github.com/aws/aws-cdk/blob/0ea4b19afd639541e5f1d7c1783032ee480c307e/packages/%40aws-cdk/core/lib/private/refs.ts#L47-L50 */ -export const isSingletonPresentInStack = (stack: GuStack, maybeSingletonInstance?: { stack: Stack }): boolean => { - // destructured `maybeSingletonInstance` to support `CfnElement`s (aka parameters) and `Resource`s, which do not share a type - return maybeSingletonInstance ? maybeSingletonInstance.stack.node === stack.node : false; +export const isSingletonPresentInStack = (stack: GuStack, maybeSingletonInstance?: Construct): boolean => { + return maybeSingletonInstance ? Stack.of(maybeSingletonInstance) === stack : false; }; From 89e5e3843c675eddd7cafed3753300b0a4c14c17 Mon Sep 17 00:00:00 2001 From: akash1810 Date: Wed, 29 Oct 2025 07:56:20 +0000 Subject: [PATCH 2/3] test: Check `minInstancesInServiceParameters` is added with multiple stacks in an `App` --- src/riff-raff-yaml-file/index.test.ts | 109 ++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/src/riff-raff-yaml-file/index.test.ts b/src/riff-raff-yaml-file/index.test.ts index 1fdfd0b742..f57a4a56fb 100644 --- a/src/riff-raff-yaml-file/index.test.ts +++ b/src/riff-raff-yaml-file/index.test.ts @@ -1525,4 +1525,113 @@ describe("The RiffRaffYamlFile class", () => { " `); }); + + it("Should include minInstancesInServiceParameters when GuEc2AppExperimental has a scaling policy (multiple stacks)", () => { + const app = new App({ outdir: "/tmp/cdk.out" }); + + class MyApplicationStack extends GuStack { + public readonly asg: GuAutoScalingGroup; + + // eslint-disable-next-line custom-rules/valid-constructors -- unit testing + constructor(app: App, id: string, props: GuStackProps) { + super(app, id, props); + + const appName = "my-app"; + + const { autoScalingGroup } = new GuEc2AppExperimental(this, { + app: appName, + instanceType: InstanceType.of(InstanceClass.T4G, InstanceSize.MICRO), + access: { scope: AccessScope.PUBLIC }, + userData: { + distributable: { + fileName: `${appName}.deb`, + executionStatement: `dpkg -i /${appName}/${appName}.deb`, + }, + }, + certificateProps: { + domainName: "rip.gu.com", + }, + monitoringConfiguration: { noMonitoring: true }, + instanceMetricGranularity: "5Minute", + scaling: { + minimumInstances: 1, + }, + applicationPort: 9000, + imageRecipe: "arm64-bionic-java11-deploy-infrastructure", + buildIdentifier: "TEST", + }); + + new CfnScalingPolicy(autoScalingGroup, "ScaleOut", { + autoScalingGroupName: autoScalingGroup.autoScalingGroupName, + policyType: "SimpleScaling", + adjustmentType: "ChangeInCapacity", + scalingAdjustment: 1, + }); + + this.asg = autoScalingGroup; + } + } + + new MyApplicationStack(app, "my-stack-CODE", { + stack: "test", + stage: "CODE", + env: { region: "eu-west-1" }, + }); + + new MyApplicationStack(app, "my-stack-PROD", { + stack: "test", + stage: "PROD", + env: { region: "eu-west-1" }, + }); + + // Ensure the Aspects are invoked... + app.synth(); + + // ...so that the CFN Parameters are added to the template, to then be processed by the `RiffRaffYamlFile` + const actual = new RiffRaffYamlFile(app).toYAML(); + + expect(actual).toMatchInlineSnapshot(` + "allowedStages: + - CODE + - PROD + deployments: + asg-upload-eu-west-1-test-my-app: + type: autoscaling + actions: + - uploadArtifacts + regions: + - eu-west-1 + stacks: + - test + app: my-app + parameters: + bucketSsmLookup: true + prefixApp: true + contentDirectory: my-app + cfn-eu-west-1-test-my-application-stack: + type: cloud-formation + regions: + - eu-west-1 + stacks: + - test + app: my-application-stack + contentDirectory: /tmp/cdk.out + parameters: + templateStagePaths: + CODE: my-stack-CODE.template.json + PROD: my-stack-PROD.template.json + amiParametersToTags: + AMIMyapp: + BuiltBy: amigo + AmigoStage: PROD + Recipe: arm64-bionic-java11-deploy-infrastructure + Encrypted: 'true' + minInstancesInServiceParameters: + MinInstancesInServiceFormyapp: + App: my-app + dependencies: + - asg-upload-eu-west-1-test-my-app + " + `); + }); }); From e931d1d27122c89563abe21b0491b50f392228e1 Mon Sep 17 00:00:00 2001 From: akash1810 Date: Wed, 29 Oct 2025 07:57:05 +0000 Subject: [PATCH 3/3] fix(riff-raff-generator): Correctly add `minInstancesInServiceParameters` parameter `GuHorizontallyScalingDeploymentPropertiesExperimental` is implemented as a singleton; only one instance is ever possible. In a scenario where an `App` has multiple `Stack`s, we have the following timeline: 1. ??? --- .../deployments/update-parameters.ts | 23 +++++++++++ src/riff-raff-yaml-file/index.ts | 41 +++++++++++-------- src/riff-raff-yaml-file/types.ts | 2 + 3 files changed, 49 insertions(+), 17 deletions(-) create mode 100644 src/riff-raff-yaml-file/deployments/update-parameters.ts diff --git a/src/riff-raff-yaml-file/deployments/update-parameters.ts b/src/riff-raff-yaml-file/deployments/update-parameters.ts new file mode 100644 index 0000000000..47c699b916 --- /dev/null +++ b/src/riff-raff-yaml-file/deployments/update-parameters.ts @@ -0,0 +1,23 @@ +import type { RiffRaffDeployment, RiffRaffDeploymentParameters, RiffRaffDeployments } from "../types"; + +/** + * Mutate the parameters of a Riff-Raff deployment. + */ +export function updateDeploymentParameters( + deployments: RiffRaffDeployments, + deployment: RiffRaffDeployment, + additionalParameters: RiffRaffDeploymentParameters, +) { + const currentDeployment = deployments.get(deployment.name); + if (!currentDeployment) { + throw new Error(`Unable to find deployment ${deployment.name}`); + } + + deployments.set(deployment.name, { + ...currentDeployment, + parameters: { + ...currentDeployment.parameters, + ...additionalParameters, + }, + }); +} diff --git a/src/riff-raff-yaml-file/index.ts b/src/riff-raff-yaml-file/index.ts index 80ec355158..51cb39f8fb 100644 --- a/src/riff-raff-yaml-file/index.ts +++ b/src/riff-raff-yaml-file/index.ts @@ -1,6 +1,7 @@ import { writeFileSync } from "fs"; import path from "path"; import type { App } from "aws-cdk-lib"; +import { Aspects } from "aws-cdk-lib"; import { Token } from "aws-cdk-lib"; import type { CfnAutoScalingGroup } from "aws-cdk-lib/aws-autoscaling"; import { dump } from "js-yaml"; @@ -15,6 +16,7 @@ import { getMinInstancesInServiceParameters, } from "./deployments/cloudformation"; import { updateLambdaDeployment, uploadLambdaArtifact } from "./deployments/lambda"; +import { updateDeploymentParameters } from "./deployments/update-parameters"; import { groupByClassNameStackRegionStage } from "./group-by"; import type { GroupedCdkStacks, @@ -22,6 +24,7 @@ import type { RiffRaffDeployment, RiffRaffDeploymentName, RiffRaffDeploymentProps, + RiffRaffDeployments, RiffRaffYaml, StackTag, StageTag, @@ -219,7 +222,7 @@ export class RiffRaffYamlFile { this.outdir = app.outdir; - const deployments = new Map(); + const deployments: RiffRaffDeployments = new Map(); const groupedStacks: GroupedCdkStacks = groupByClassNameStackRegionStage(this.allCdkStacks); @@ -274,25 +277,29 @@ export class RiffRaffYamlFile { deployments.set(asgDeployment.name, asgDeployment.props); }); - const amiParametersToTags = getAmiParameters(autoscalingGroups); - - const minInServiceParamMap = - GuHorizontallyScalingDeploymentPropertiesExperimental.getInstance(stack).asgToParamMap; - const minInServiceAsgs = autoscalingGroups.filter((asg) => minInServiceParamMap.has(asg.node.id)); - const minInstancesInServiceParameters = getMinInstancesInServiceParameters(minInServiceAsgs); + // only add the `amiParametersToTags` property if there are some + if (autoscalingGroups.length > 0) { + updateDeploymentParameters(deployments, cfnDeployment, { + amiParametersToTags: getAmiParameters(autoscalingGroups), + }); + } - deployments.set(cfnDeployment.name, { - ...cfnDeployment.props, - parameters: { - ...cfnDeployment.props.parameters, + const maybeScalingDeploymentPropertiesAspect = Aspects.of(stack).all.find( + (_): _ is GuHorizontallyScalingDeploymentPropertiesExperimental => + _ instanceof GuHorizontallyScalingDeploymentPropertiesExperimental, + ); - // only add the `amiParametersToTags` property if there are some - ...(autoscalingGroups.length > 0 && { amiParametersToTags }), + if (maybeScalingDeploymentPropertiesAspect) { + const { asgToParamMap } = maybeScalingDeploymentPropertiesAspect; + const minInServiceAsgs = autoscalingGroups.filter((asg) => asgToParamMap.has(asg.node.id)); - // only add the `minInstancesInServiceParameters` property if there are some - ...(minInServiceAsgs.length > 0 && { minInstancesInServiceParameters }), - }, - }); + // only add the `minInstancesInServiceParameters` property if there are some + if (minInServiceAsgs.length > 0) { + updateDeploymentParameters(deployments, cfnDeployment, { + minInstancesInServiceParameters: getMinInstancesInServiceParameters(minInServiceAsgs), + }); + } + } }); }); }); diff --git a/src/riff-raff-yaml-file/types.ts b/src/riff-raff-yaml-file/types.ts index e946385672..1845f068b5 100644 --- a/src/riff-raff-yaml-file/types.ts +++ b/src/riff-raff-yaml-file/types.ts @@ -43,3 +43,5 @@ export type GroupedCdkStacks = Record< ClassName, Record>> >; + +export type RiffRaffDeployments = Map;