-
Notifications
You must be signed in to change notification settings - Fork 264
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
EKS: edit imported clusters, and clusters with exclusively self-managed nodes #11294
EKS: edit imported clusters, and clusters with exclusively self-managed nodes #11294
Conversation
fix networking subnet mode display
…en configured to create a vpc and subnet automatically
f32fb7c
to
d3ae4a9
Compare
pkg/eks/components/CruEKS.vue
Outdated
@@ -409,6 +418,9 @@ export default defineComponent({ | |||
}, | |||
|
|||
async actuallySave(): Promise<void> { | |||
if (!this.isNewOrUnprovisioned && !this.nodeGroups.length) { | |||
delete this.normanCluster.eksConfig.nodeGroups; |
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.normanCluster.eksConfig
is possibly undefined. We can include it in the if condition before delete.
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.
LGTM
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.
Just a comment to add the null check.
Updated to include #11291 - the same syncUpstreamConfig functionality needs to be added to GKE provisioning. No extra logic around node groups needed for GKE though. |
I hit an additional issue with this: due to pre-existing norman api validation it's not possible to add node groups then delete all node groups through the UI. I've separated that out into a different issue, as it needs backend work as well and the other fixes in this PR are required to reproduce the bug. |
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.
LGTM
…ed nodes (rancher#11294) * sync upstream eks spec on edit * allow for nil nodeGroups fix networking subnet mode display * split up syncUpstreamConfig test * update subnets display for imported clusters and clusters that had been configured to create a vpc and subnet automatically * fix ts in eks save function * add syncUpstreamConfig to gke provisioning
Summary
Fixes #11100
Fixes #11104
Fixes #10335
Fixes #11291
Occurred changes and/or fixed issues
This PR updates EKS provisioning to incorporate the same upstream syncing behavior as AKS - see #11120 for some background on why this is needed. This PR adds some fixes to the syncing utility as well as unit tests around the problems I found while working on the PR. There are also some minor fixes around the EKS components to correct console errors shown when editing an imported cluster.
Special notes about EKS:
Areas or cases that should be tested
user should be able to add and remove node groupsEDIT: removing all node groups is causing issues due to backend validation, issue filed here EKS - unable to remove all managed nodegroups from imported EKS cluster that also has self-managed nodes #11336
Checklist