-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(imagebuilder-alpha): add support for Image Recipe Construct #36092
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ozelalisen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for considering patterns from previous PRs, left first round of comments
| resource: 'image-recipe', | ||
| resourceName: `${this.physicalName}/${imageRecipeVersion}`, | ||
| }); | ||
| this.imageRecipeVersion = cdk.Fn.select(2, cdk.Fn.split('/', imageRecipe.attrArn)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be belong here, it was related to fromImageRecipeAttributes method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah good call. we can actually use .getAtt('Version') here (for some reason, that is not modeled as .attrVersion on the L1)
| * | ||
| * @default - this is false if the Systems Manager agent is pre-installed on the base image. Otherwise, this is true. | ||
| */ | ||
| readonly uninstallSsmAgentAfterBuild?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if on my baseimage there is not any ssmagent, and I set this as true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it behaves the same as the default behavior - the output image will not contain the SSM agent
| public static fromString(value: string): ComponentParameterValue { | ||
| return new ComponentParameterValue([value]); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be multiple values but, only one value can be saved via this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API/CFN input is string list, but only one element is supported for now (to support the simple string data type), so that it can be extended once stringList is supported here
| /** | ||
| * The parameter value for a component parameter | ||
| */ | ||
| export class ComponentParameterValue { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see any purpose with this class, because parameter value can only be list of strings, so we do not need create an additional parameter value class, this pattern is useful, when value is any that can be number, string, bool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - I created it like this so that when Image Builder components support more parameter value data types, we can extend it easily here, rather than having to deprecate the property if we add support for stringLists or booleans for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, it is expected that this might be extended for different types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - that was the intent of making this class
| * @default - no parameters. if the component contains parameters, their default values will be used. otherwise, any | ||
| * required parameters that are not included will result in a build failure | ||
| */ | ||
| readonly parameters?: { [name: string]: ComponentParameterValue }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplify as:
| readonly parameters?: { [name: string]: ComponentParameterValue }; | |
| readonly parameters?: { [name: string]: string[] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep this as ComponentParameterValue. As noted in the other comment, a string list is accepted in API/CFN, but only one element is supported since only the string data type is supported. When stringList, boolean, etc. is supported, ComponentParameterValue would be the better option here, following CDK design guideline for union data types
56b4365 to
f41878c
Compare
Pull request has been modified.
336df49 to
aef71d7
Compare
aef71d7 to
7ca9300
Compare
| /** | ||
| * Indicates whether the recipe is an Image Recipe | ||
| * | ||
| * @internal | ||
| */ | ||
| _isImageRecipe(): this is IImageRecipe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are gonna use this in Image/ImagePipeline constructs. In those constructs, recipe: IRecipeBase will be accepted which can be either an image or container recipe. This method helps us know which recipe, so that we can map it to the correct parameter in the L1 definition. There will be a similar method for IContainerRecipe.
| /** | ||
| * The parameter value for a component parameter | ||
| */ | ||
| export class ComponentParameterValue { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, it is expected that this might be extended for different types?
| /** | ||
| * Indicates whether the recipe is an Image Recipe | ||
| * | ||
| * @internal | ||
| */ | ||
| public _isImageRecipe(): this is IImageRecipe { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is purpose of this?
Issue
aws/aws-cdk-rfcs#789
Reason for this change
This change adds a new alpha module for EC2 Image Builder L2 Constructs (@aws-cdk/aws-imagebuilder-alpha), as outlined in aws/aws-cdk-rfcs#789. This PR specifically implements the ImageRecipe construct.
Description of changes
This change implements the
ImageRecipeconstruct, which is a higher-level construct of CfnImageRecipe.Example
Describe any new or updated permissions being added
N/A - new L2 construct in alpha module
Description of how you validated changes
Validated with unit tests and integration tests. Manually verified generated CFN templates as well.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license