-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(aws-ecs): add windows core and kernel optimized ecs amis #34274
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
Conversation
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 pull request linter fails with the following errors:
❌ The title scope of the pull request should omit 'aws-' from the name of modified packages. Use 'ecs' instead of 'aws-ecs'.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed, add Clarification Request
to 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 @ykethan for submitting this request, left few comments to address
this.amiParameterName = constructSsmParameterPath({ | ||
windowsVersion: this.windowsVersion, | ||
generation: this.generation, | ||
hwType: this.hwType || AmiHardwareType.STANDARD, |
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 don't think we were setting a default value for this.hwtype
before, is my understanding correct
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.
you are right, we only set this on EcsOptimizedAmi class
aws-cdk/packages/aws-cdk-lib/aws-ecs/lib/amis.ts
Lines 118 to 122 in 6b441b7
/** | |
* Constructs a new instance of the EcsOptimizedAmi class. | |
*/ | |
constructor(props?: EcsOptimizedAmiProps) { | |
this.hwType = (props && props.hardwareType) || AmiHardwareType.STANDARD; |
But in EcsOptimizedImage class dont set a default value during initialization
aws-cdk/packages/aws-cdk-lib/aws-ecs/lib/amis.ts
Lines 257 to 261 in 6b441b7
/** | |
* Constructs a new instance of the EcsOptimizedAmi class. | |
*/ | |
private constructor(props: EcsOptimizedAmiProps) { | |
this.hwType = props && props.hardwareType; |
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.
okay a little bit confused, can we remove this default that we're adding here or its needed ?
hwType: this.hwType ?? AmiHardwareType.STANDARD,
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 default is needed in this case. Let me explain:
In the original code, we didn't have an explicit default for this.hwType
in the EcsOptimizedImage
class constructor - it simply assigns this.hwType = props && props.hardwareType
which could leave it as undefined.
When we previously constructed the SSM parameter path, we had a series of conditionals:
+ (this.hwType === AmiHardwareType.GPU ? 'gpu/' : '')
+ (this.hwType === AmiHardwareType.ARM ? 'arm64/' : '')
+ (this.hwType === AmiHardwareType.NEURON ? 'inf/' : '')
With these conditionals, if this.hwType
was undefined, none of these would match, which functionally behaves like using STANDARD
(since no hardware-specific path segment would be added).
Now with the new shared utility function constructSsmParameterPath()
, we need to provide an explicit hwType value. Using this.hwType ?? AmiHardwareType.STANDARD
ensures we maintain the same behavior as before - if this.hwType
is undefined, we default to STANDARD
.
So while we're adding an explicit default here, it's not changing the actual behavior - it's just making the implicit default from the original code explicit for the utility function.
Let me know if this makes sense or if you'd prefer to handle it differently.
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.
okay got it, if I understand correctly, the AmiHardwaretype check was to add specific suffix like 'gpu/' or 'arm64', since there is no special case handling for this.hwType === AmiHardwareType.STANDARD its same as being undefined.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
Closes #.
Reason for this change
ECS packages currently does not have all the AMI's from the documentation:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-optimized_AMI.html
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-optimized_windows_AMI.html
Description of changes
adds
For Windows Server Core variants:
For kernel-specific Amazon Linux 2 variants:
Describe any new or updated permissions being added
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license