feat(ecs): enable Kernel 5.10 support on Amazon Linux 2 #29809
Conversation
github-actions
bot
added
p2
beginning-contributor
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. 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.
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. 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.
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Konosh93
changed the title
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. 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.
Konosh93
changed the title
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
added
the
pr/needs-community-review
There was a problem hiding this comment.
Thanks 👍
I left some suggestions for adjustments.
Also, does the recommended here refer to the kernel version? 🤔
If so, we should remove it when kernel is specified.
aws-cdk-automation
removed
pr/needs-community-review
I am not sure what Both of the following commands return a valid AMI from SSM and |
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
@Mergifyio update |
❌ Mergify doesn't have permission to updateFor security reasons, Mergify can't update this pull request. Try updating locally. |
|
@TheRealAmazonKendra Can we go ahead and merge this change, or is it missing something? |
|
@TheRealAmazonKendra Do you have any other suggestions to add to this PR? |
| * | ||
| * @default none, uses the recommended kernel version | ||
| */ | ||
| readonly kernel?: string; |
There was a problem hiding this comment.
As @TheRealAmazonKendra said, this is a deprecated interface, so we shouldn't be adding a new property to this...except, there's a note that says remove @deprecated above in V2. @rix0rrr I'm thinking we should un-deprecate this property, WDYT?
This should not be a string value, it should be an enum. We should not force users to call the .toString() on the enum, they should just pass it directly.
The enum provides type discoverability; if the kernel property is only a string type, there's nothing the IDE can do to tell you that you should really be passing the result of a toString() call. Instead, with the enum, it tells you what type is expected, so the user discovers what they need to pass much more easily.
| /** | ||
| * Kernel version used. | ||
| * Only supported for Amazon Linux 2. | ||
| * | ||
| * @default none, uses the recommended kernel version | ||
| */ | ||
| readonly kernel?: ec2.AmazonLinux2Kernel; |
There was a problem hiding this comment.
Discussed offline, this should not be undeprecated...sorry about that, I was wrong. This entire props should be deprecated and replaced with the linked EcsOptimizedImage instead.
There was a problem hiding this comment.
@comcalvi I replied to your comment here. Can you have a look?
This entire props should be deprecated and replaced with the linked EcsOptimizedImage instead.
EcsOptimizedImage is a class and its private constructor is still using the deprecated EcsOptimizedAmiProps. There are no other Props interfaces for EcsOptimizedImage constructor.
Is it time to remove @deprecated already?
There was a problem hiding this comment.
It looks like we deprecated the props and decided to create a bunch of factory methods that take an Options instead of props. I think your original solution was actually correct; leave these as deprecated (since they should be marked internal, and if we remove the deprecation we'll never remember to fix this) and just add the new property.
@TheRealAmazonKendra is correct about this, in principle, since a deprecated marker implies that these props have been directly replaced by something else. That is not what is going on here. Instead, these props are supposed to be hidden away behind a set of factory methods. So, even if we had not missed these when we did v2, the optimal would still be what you've done here; add kernel to these props.
Sorry for the churn on this one, please leave the @deprecated and add the new property to those deprecated props.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
Issue
#29800
Reason for this change
Allow using recommended kernel version instead of 4.14 which is marked as EOL Jan 2024, by opting in for the new kernel.
Description of changes
amazonLinux2in favor of a newamazonLinux2WithKernel510.Description of how you validated changes
Unit and integration tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license