-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(ecs-patterns): add missing NLB task image options #30775
base: main
Are you sure you want to change the base?
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 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.
Exemption Request: The current coverage of ECS pattern tasks isn't great, both for ALB and NLB. I don't think it's pertinent to add a couple of tests for these props, it would be a lot more pertinent to redo the entire classes Regarding the README, I don't think these props need to be listed |
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 @nmussy for the contribution. It would be good to have unit tests and integ test for these properties similar to ALB EC2 and Fargate services. It is ok not to update README for these props, but would recommend to have the testcases.
healthCheck: props.healthCheck, | ||
logging: logDriver, | ||
environment: taskImageOptions.environment, | ||
secrets: taskImageOptions.secrets, | ||
dockerLabels: taskImageOptions.dockerLabels, | ||
command: taskImageOptions.command, | ||
entryPoint: taskImageOptions.entryPoint, |
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 see similar properties available in ApplicationLoadBalancedFargateService
and since we are adding the same to NetworkLoadBalancedFargateService
, I would recommend we have some unit test that covers these properties. We could have some testcases similar to these in ApplicationLoadBalancedFargateService
.
command: taskImageOptions.command, | ||
entryPoint: taskImageOptions.entryPoint, |
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.
Similarly we should have some unit tests for these commands in NetworkLoadBalancedEc2Service
. We have some similar test cases for ApplicationLoadBalancedEc2Service
.
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.
@godwingrs22 Seems like coverage for |
The pull request linter fails with the following errors:
PRs must pass status checks before we can provide a meaningful review. If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing ✅ A exemption request has been requested. Please wait for a maintainer's review. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #30775 +/- ##
=======================================
Coverage 81.40% 81.40%
=======================================
Files 223 223
Lines 13727 13727
Branches 2411 2411
=======================================
Hits 11175 11175
Misses 2274 2274
Partials 278 278
Flags with carried forward coverage won't be shown. Click here to find out more.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
Closes #30760.
Reason for this change
The ECS patterns L3 allows to create a load balanced Fargate or EC2 task, and to customize their run options. ALB task patterns implemented additional properties that NLB didn't.
Description of changes
command
,entryPoint
props toNetworkLoadBalancedFargateService
andNetworkLoadBalancedEc2Service
healthCheck
prop toNetworkLoadBalancedFargateService
healthCheck
s are only exposed for Fargate targetsDescription of how you validated changes
No unit or integ were added/updated
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license