fix(eks): neuron plugin not added with addNodegroupCapacity()#27909
fix(eks): neuron plugin not added with addNodegroupCapacity()#27909mergify[bot] merged 5 commits intoaws:mainfrom
Conversation
aws-cdk-automation
left a comment
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.
b19674e to
0583de9
Compare
Clarification Request❌ Features must contain a change to a README file. The neuron plugin was not mentioned previously in the aws-eks README file, so I didn't know where to document this semi-new behaviour. Should I add a note that the plugin is added when using both |
lpizzinidev
left a comment
There was a problem hiding this comment.
Thanks for the contribution 👍
I think that this is more a fix than a feature, can please you update the title following the guidelines?
Also, an integration test is needed.
You can use this as a template and check the documentation for its execution.
09c205c to
11f7b06
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
lpizzinidev
left a comment
There was a problem hiding this comment.
Thanks, looks good 👍
Can you please update the title to fix(eks): neuron plugin not added with addNodegroupCapacity?
It should describe the bug (not the solution).
should I change the commit itself too, or you'll squash merge with using the PR topic? |
|
@gdamjan |
| aws-eks/test/integ.eks-cluster.js | ||
| aws-eks/test/integ.eks-helm-asset.js | ||
| aws-eks/test/integ.eks-inference.js | ||
| aws-eks/test/integ.eks-inference-nodegroup.js |
There was a problem hiding this comment.
| aws-eks/test/integ.eks-inference-nodegroup.js |
Do not think we need to update this file.
There was a problem hiding this comment.
afaik, the integration tests were tying to run the .ts file in that case and failing. I can try again.
I would add something like this: just above the what do you think? |
Previously, the neuron plugin was only added in addAutoScalingGroupCapacity() when the instanceType was some of the INFERENTIA types. Let's also add the plugin if at least one (some/any) of the instanceTypes in addNodegroupCapacity() is of the INFERENTIA types. added integration tests for the addNodegroupCapacity() feature too, and snapshot test Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
11f7b06 to
bcbfccf
Compare
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Previously, the neuron plugin is already added in the addAutoScalingGroupCapacity() method, when the instanceType was some of the INFERENTIA types.
Let's also add the plugin if at least one (some/any) of the instanceTypes in addNodegroupCapacity() is of the INFERENTIA types.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license