-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(eks): added support for bootstrapSelfManagedAddons
#33597
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.
(This review is outdated)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33597 +/- ##
=======================================
Coverage 82.20% 82.21%
=======================================
Files 119 119
Lines 6862 6876 +14
Branches 1158 1162 +4
=======================================
+ Hits 5641 5653 +12
- Misses 1118 1120 +2
Partials 103 103
Flags with carried forward coverage won't be shown. Click here to find out more.
|
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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.
Hi @mrlikl , thank you for your contribution!
Your commits include lots of integ and snapshots changes that aren't necessary and are out of date. You might need to reset your snapshot and integ test changes, then rebase your changes from main
.
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.
Changes to this file are no longer needed due to the following commit: test(eks): remove eks.KubernetesVersion.V1_24 as default version in i…
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 review is outdated)
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.
As reported by the linter, an integration test would need to be changed or added which would use the new bootstrapSelfManagedAddons
property.
This reverts commit 79be245.
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 have updated the snapshots, but no integ tests (integ-*.ts
) were modified.
I am on it, |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
This reverts commit e7e2c36.
- Remove integ.bucket-deployment-signcontent.js and .d.ts files - These are compiled outputs that should be generated from .ts source - Follows .gitignore rules that exclude *.js files
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 can't do inline comments as the Files Changed tab doesn't work due to the huge amount of files changed. Apologies for inconvenience.
On cluster.ts
:
const compareUndefinedOrTrue = (val1: boolean | undefined, val2: boolean | undefined): boolean => {
// If both are undefined or true, or one is undefined and the other is true, consider them equal
if ((val1 === undefined && val2 === true) || (val1 === true && val2 === undefined) || val1 === val2) {
return true;
}
return false;
};
Replace with
const compareUndefinedOrTrue = (val1: boolean | undefined, val2: boolean | undefined): boolean => {
return (val1 ?? true) == (val2 ?? true)
}
On packages/@aws-cdk-testing/framework-integ/test/aws-s3-deployment/test/integ.bucket-deployment-signcontent.js
:
const app = new cdk.App({
postCliContext: {
'@aws-cdk/aws-lambda:useCdkManagedLogGroup': false,
},
});
Is it necessary to add this option?
On packages/aws-cdk-lib/aws-eks/README.md
, replace:
### Self-Managed AddOns
with
### Self-Managed Add-ons
The changes are in for review |
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.
Thank you!
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). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #30792
Reason for this change
The feature enables support to create clusters without the default networking add-ons
Description of changes
Describe any new or updated permissions being added
Added the prop bootstrapSelfManagedAddons to the cluster and incremented the eks client version. Also validated that existing if bootstrapSelfManagedAddons is undefined to true or vice versa does not replace the cluster as the default is
true
.Description of how you validated changes
Validated the changes against an existing cluster and made sure it is not replaced unless the change is from true to false or vice versa.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license