-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(imagebuilder-alpha): add support for Container Recipe Construct #36091
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
packages/@aws-cdk/aws-imagebuilder-alpha/lib/container-recipe.ts
Outdated
Show resolved
Hide resolved
| * @param string The container instance image as a direct string value | ||
| */ | ||
| public static fromString(string: string): BaseContainerImage { | ||
| return new ContainerInstanceImage(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.
Isn't the expected return type BaseContainerImage here?
| // } | ||
|
|
||
| /** | ||
| * The string value of the base image to use in a container recipe |
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.
Can we elaborate what could this string value represent.
| } | ||
|
|
||
| /** | ||
| * The string value of the container instance image to use in a container recipe |
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.
Can we elaborate what could this string value represent.
packages/@aws-cdk/aws-imagebuilder-alpha/lib/container-recipe.ts
Outdated
Show resolved
Hide resolved
| attrs: ContainerRecipeAttributes, | ||
| ): IContainerRecipe { | ||
| if (attrs.containerRecipeArn && (attrs.containerRecipeName || attrs.containerRecipeVersion)) { | ||
| throw new cdk.ValidationError( |
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.
Why is this validation error needed? Can we just not parse the attributes as required?
packages/@aws-cdk/aws-imagebuilder-alpha/lib/container-recipe.ts
Outdated
Show resolved
Hide resolved
| this.instanceBlockDevices.push(...instanceBlockDevices); | ||
| } | ||
|
|
||
| private renderBlockDevices(): CfnContainerRecipe.InstanceBlockDeviceMappingProperty[] | undefined { |
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.
Can we add JSDoc here explaining what this helper does.
| * | ||
| * @param string The base image as a direct string value | ||
| */ | ||
| public static fromString(string: string): BaseContainerImage { |
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.
Param name string is not intuitive, rename this to something meaningful and update doc string accordingly.
039ee95 to
b4532cb
Compare
Pull request has been modified.
| * @param parameter The SSM parameter to use as the container instance image | ||
| */ | ||
| public static fromSsmParameter(parameter: ssm.IStringParameter): ContainerInstanceImage { | ||
| return this.fromSsmParameterName(parameter.parameterArn); |
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 method says parameterName, this should be changed to:
| return this.fromSsmParameterName(parameter.parameterArn); | |
| return this.fromSsmParameterName(parameter.parameterName); |
| } | ||
|
|
||
| /** | ||
| * The string value of the container instance image to use in a container recipe. This can either be an SSM parameter, |
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.
Based on the CFN docs, this should be more explicit for SSM parameter:
| * The string value of the container instance image to use in a container recipe. This can either be an SSM parameter, | |
| * The string value of the container instance image to use in a container recipe. This can either be: | |
| * - an SSM parameter reference, prefixed with `ssm:` and followed by the parameter name or ARN | |
| * - an AMI ID |
|
|
||
| const CONTAINER_RECIPE_SYMBOL = Symbol.for('@aws-cdk/aws-imagebuilder-alpha.ContainerRecipe'); | ||
|
|
||
| const LATEST_VERSION = 'x.x.x'; |
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.
Can we add JSDoc here explaining what this magic value x.x.x represents, and link reference docs.
| resource: 'container-recipe', | ||
| resourceName: `${this.physicalName}/${containerRecipeVersion}`, | ||
| }); | ||
| this.containerRecipeVersion = containerRecipe.getAtt('Version').toString(); |
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.
CFN docs doesn’t define a return value with Version attribute for Fn::GetAtt
| /** | ||
| * The version of the container recipe | ||
| * | ||
| * @attribute |
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.
Why is this marked as an attribute? CFN docs don’t define a Version return value.
| * The dockerfile template used to build the container image. | ||
| * | ||
| * @default - a standard dockerfile template will be generated to pull the base image, perform environment setup, and | ||
| * run all components in the recipe |
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.
nit:
| * run all components in the recipe | |
| * run all components in the recipe |
| * The working directory for use during build and test workflows. | ||
| * | ||
| * @default - the Image Builder default working directory is used. For Linux and macOS builds, this would be /tmp. For | ||
| * Windows builds, this would be C:/ |
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.
nit:
| * Windows builds, this would be C:/ | |
| * Windows builds, this would be C:/ |
| version: containerRecipeVersion, | ||
| description: props.description, | ||
| parentImage: props.baseImage.image, | ||
| containerType: 'DOCKER', |
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.
Can this be extracted to enum to support other containerTypes?
| * | ||
| * @default None | ||
| */ | ||
| readonly components?: ComponentConfiguration[]; |
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 is marked optional, but CFN doc states - "Recipes require a minimum of one build component" ?
|
|
||
| /** | ||
| * The resulting inline string or S3 URL which references the dockerfile data | ||
| */ |
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.
nit: Can also add:
* - For inline dockerfiles, this is the Dockerfile template text
* - For S3-backed dockerfiles, this is the S3 URL
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 ContainerRecipe construct.
Description of changes
This change implements the
ContainerRecipeconstruct, which is a higher-level construct of CfnContainerRecipe.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