Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scale managed nodegroup with --name flag #4383

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

nikimanoledaki
Copy link
Contributor

@nikimanoledaki nikimanoledaki commented Oct 29, 2021

Description

Fixes bug in #2655

The issue was that when a user was doing eksctl scale nodegroup --config-file x --name y, eksctl was not looking in the config file for managed nodegroups. This only worked for unmanaged nodegroups.

Iterating over managedNodeGroups as well and then returning the nodegroup base (which is common to both types of ng) fixes this issue.

A separate PR will address scaling all nodegroups in the config file if the name flag is not provided.

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 馃く

  • Backfilled missing tests for code in same general area 馃帀
  • Refactored something and made the world a better place 馃専

@@ -166,4 +166,39 @@ var _ = Describe("scale node group config file loader", func() {
err: fmt.Errorf("at least one of minimum, maximum and desired nodes must be set"),
}),
)

Describe("for managed nodegroups", func() {
Context("when using a config file", func() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could Context could be a When and then just write using a config file? But I don't have a strong opinion on that.

Copy link
Contributor

@Skarlso Skarlso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Well done Niki! :) 馃帀

@@ -44,14 +46,28 @@ func ClusterHasInstanceType(cfg *ClusterConfig, hasType func(string) bool) bool
return false
}

// HasNodegroup returns true if this clusterConfig contains a managed or un-managed nodegroup with the given name
func (c *ClusterConfig) FindNodegroup(name string) *NodeGroup {
// FindNodegroup returns true if this clusterConfig contains a managed or un-managed nodegroup with the given name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment needs to be updated to match the signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, updated it!

@nikimanoledaki nikimanoledaki merged commit 18e4c0d into eksctl-io:main Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants