-
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
fix(aws-eks): nodegroupName attribute correctly assigned #22910
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.
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Good catch! I have just a couple comments inline. Additionally, I think that we're going to need this under a feature flag. Fixing this seems to be a destructive change.
@@ -197,6 +197,8 @@ class EksClusterStack extends Stack { | |||
minSize: 1, | |||
// reusing the default capacity nodegroup instance role when available | |||
nodeRole: this.cluster.defaultCapacity ? this.cluster.defaultCapacity.role : undefined, | |||
// custom nodegropuName | |||
nodegroupName: 'extra-ng', |
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.
Let's add an assert to this test that checks that this is correctly set.
@@ -569,6 +569,7 @@ | |||
"./aws-opensearchservice": "./aws-opensearchservice/index.js", | |||
"./aws-opsworks": "./aws-opsworks/index.js", | |||
"./aws-opsworkscm": "./aws-opsworkscm/index.js", | |||
"./aws-organizations": "./aws-organizations/index.js", |
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 file shouldn't be included in this commit.
@pahud given the age of this one and the merge conflicts, I'm going to close this. Please feel free to re-open it if you plan to continue working on it! |
### Issue # (if applicable) As #22910 and #25278 were closed for staleness. I am opening a new PR to fix #22442. To avoid breaking change, this PR introduces a new feature flag for the bug fix. Closes #22442 ### The Code for verification ```ts import { App, Stack, aws_eks as eks, aws_ec2 as ec2, CfnOutput, cx_api, } from 'aws-cdk-lib'; const app = new App(); const env = { region: process.env.CDK_DEFAULT_REGION, account: process.env.CDK_DEFAULT_ACCOUNT }; const stack = new Stack(app, 'my-test-stack2', { env }); stack.node.setContext(cx_api.EKS_NODEGROUP_NAME, true); const vpc = ec2.Vpc.fromLookup(stack, 'Vpc', { isDefault: true }); const cluster = new eks.Cluster(stack, 'Cluster', { vpc, version: eks.KubernetesVersion.V1_29, defaultCapacity: 0, }); const ng1 = new eks.Nodegroup(stack, 'ng-1', { cluster, }); const ng2 = cluster.addNodegroupCapacity('ng-2', { nodegroupName: 'custom-name', }); new CfnOutput(stack, 'ng1-name', { value: ng1.nodegroupName }); new CfnOutput(stack, 'ng2-name', { value: ng2.nodegroupName }); ``` The output is like ``` Outputs: my-test-stack2.ng1name = ng19F1EA947-sSziNMzPVkik my-test-stack2.ng2name = custom-name ``` Console: <img width="315" alt="image" src="https://github.com/aws/aws-cdk/assets/278432/019255bd-7d05-43ad-8624-baf5b6eea725"> ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR fixes the
nodegroupName
attribute was incorrectly assigned.I am not sure if this will cause breaking changes. Please kindly advise.
Fixes: #22442
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license