Allow imageId and instanceType to be passed into ASG #213
Conversation
| @@ -40,7 +40,21 @@ describe("The GuAutoScalingGroup", () => { | |||
| }); | |||
| }); | |||
|
|
|||
| test("adds the instanceType parameter", () => { | |||
| test("does not add the AMI parameter if an imageId prop provided", () => { | |||
There was a problem hiding this comment.
I believe that Riff-Raff currently relies on AMI being configured as a parameter in order to automatically update the underlying AMI during a deployment. That's a widely used feature in the department (and ideally we want every application to do this due to the security benefits it brings), so I think we might need to discourage passing it as a prop for now.
There was a problem hiding this comment.
That's a good point. The reason this was added back as an optional prop was to allow for stacks that have multiple ASGs. Currently, that would fail as we'd try to create the same parameter more than once. This would allow a stack to define multiple parameters and pass them into each ASG as required. If we want to go as far as requiring AMI values to be a parameter, we could change the type of imageId to GuAMIParameter to allow for this overriding but prevent the value being hardcoded in the stack.
There was a problem hiding this comment.
The reason this was added back as an optional prop was to allow for stacks that have multiple ASGs.
Hmm, I hadn't thought about that other use-case! Maybe we should still allow this then and use patterns/docs to push 'typical' single-ASG stacks towards using an AMI parameter.
There was a problem hiding this comment.
I've added a suggestion below which might be the best of both worlds. Agreed that the standard EC2 pattern should then just create the parameter for you!
|
I wonder if we should go even further for In addition to the benefits @akash1810 mentioned, using a If we included a default |
src/constructs/autoscaling/asg.ts
Outdated
| @@ -11,6 +11,8 @@ import { GuAmiParameter, GuInstanceTypeParameter } from "../core"; | |||
| // https://www.typescriptlang.org/docs/handbook/utility-types.html#omittype-keys | |||
| export interface GuAutoScalingGroupProps | |||
| extends Omit<AutoScalingGroupProps, "imageId" | "osType" | "machineImage" | "instanceType" | "userData"> { | |||
| instanceType?: InstanceType; | |||
| imageId?: string; | |||
There was a problem hiding this comment.
This would require the prop to be a parameter so we can cater for stacks with multiple ASGs but enforce using parameters for AMIs.
| imageId?: string; | |
| imageId?: GuAMIParameter; |
What does this change?
Currently the
GuAutoscalingGroupcreates parameters for theimageId(AMI) andinstanceTypevalues and omits the props. This PR adds the props back in as optional and only creates the parameters if no value is passed. This maintains the BYO pattern as the default behaviour but allows flexibility where required.Does this change require changes to existing projects or CDK CLI?
No
How to test
New unit tests have been written to cover the change in behaviour.
How can we measure success?
The default behaviour of our constructs makes them easy to use but also allows for flexibility.