Refactor and increase test coverage for creating nodegroups #3728

nikimanoledaki · 2021-05-18T12:53:12Z

Description

Related to #2931

Adds unit test for pkg/actions/nodegroup.create to increase test coverage. Tests failures as well as successfully creating managed nodes, creating unmanaged nodes, and creating nodes with or without all of the CreateOpts.

New Interfaces

The interface KubeProvider has some functions from the ClusterProvider struct that did not previously belong to any interface and were therefore hard to mock.
The interface NodeGroupInitialiser has functions from the NodeGroupService struct.
The interface NodegroupFilter has functions from the struct NodeGroupFilter (StackLister had to be exported in order to be able to mock it).
Mocks were generated for these new interfaces with Counterfeiter and used in the unit tests.

Refactoring the main Create function

Adds 2 setter funcs for NodeManager to mock out 2 of its fields without having to change the code in all the other packages where NodeManager is used.
Moved part of the Create func to a new func UpdateAuthConfigMap that is now part of the KubeProvider interface.
Splits part of the Create function to a new func nodeCreationTasks so this reduces it from ~200 to ~120 LOC.

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 🌟

pkg/actions/nodegroup/create.go

pkg/actions/nodegroup/nodegroup.go

pkg/cfn/manager/create_tasks.go

nikimanoledaki · 2021-05-25T12:00:04Z

@aclevername I implemented your review feedback and then manually created a nodegroup to test the changes - no regression!

aclevername · 2021-05-25T14:19:40Z

pkg/actions/nodegroup/nodegroup.go

@@ -18,6 +18,8 @@ type Manager struct {
 	cfg          *api.ClusterConfig
 	clientSet    *kubernetes.Clientset
 	wait         WaitFunc
+	init         eks.NodeGroupInitialiser


I feel like init is a bit vague, but not sure on a better name 🤷‍♂️

i think initialiser, while verbose, would be clearer. init as we know it in Go does shorthand initialise, but it may be helpful to clarify that we don't mean that kind of init.

aclevername

LGTM! Big PR so worth waiting for 2 accepting reviews before merging IMO 😄

Callisto13

Great work 🎉

I just have a couple of comments on really superficial things.

pkg/ctl/cmdutils/filter/nodegroup_filter.go

pkg/eks/api.go

pkg/eks/nodegroup_service.go

pkg/actions/nodegroup/create_test.go

pkg/ctl/cmdutils/filter/nodegroup_filter.go

pkg/actions/nodegroup/create.go

pkg/ctl/cmdutils/filter/nodegroup_filter.go

Remove KubeProvider from v1alpha5 types Generate KubeProvider mocks to be used by Nodegroup unit tests * Abstracts some of ClusterProvider's functions into a KubeProvider interface in pkg/eks/kubeprovider.go * Generates mocks for KubeProvider in pkg/eks/fakes * Passes KubeProvider to the ClusterProvider struct * Mocks calls to KubeProvider's functions in pkg/actions/nodegroups/create.go and pkg/actions/nodegroups/create_test.go

* Test for failing to load VPC from config * Test when cluster does not support managed nodes

* Test for when cluster is not compatible with ng config * Test for when existing local ng stacks in config file fail to be listed

* Unit test ng creation when failing to validate legacy subnets for ng * Transcribe go pkg tests to ginkgo tests * Test whether ng uses IRSA, test that stack manager does tasks * Test evaluates whether aws-node uses IRSA * Test checks if stack manager fails to do ng tasks * Test ng is created without errors - happy path * Test ng creation with dry run option * Test creating ng will all options * Unit test creating managed nodegroups * Refactored WaitForNodes from ClusterProvider to nodeGroupService * Refactor WaitTimeout to KubeProvider * Refactor UpdateAuthConfigMap to nodeGroupService

* Move ValidateExistingNodeGroupsForCompatibility to nodegroup initialiser interface * Move DoAllNodegroupStackTasks to nodegroup initialiser interface

* Get rid of counterfeiter warning when generating mocks * Update doc comments * Rename unit test names to more descriptive names Co-authored-by: Claudia <claudiaberesford@gmail.com>

Callisto13

nikimanoledaki added kind/improvement technical debt labels May 18, 2021

nikimanoledaki changed the title ~~Refactor and increase test coverage for nodegroup creation~~ Refactor and increase test coverage for creating nodegroups May 19, 2021

nikimanoledaki marked this pull request as ready for review May 19, 2021 14:04

nikimanoledaki requested review from aclevername and cPu1 May 19, 2021 14:57

cPu1 reviewed May 20, 2021

View reviewed changes

pkg/actions/nodegroup/create.go Outdated Show resolved Hide resolved

nikimanoledaki requested a review from cPu1 May 20, 2021 15:26

aclevername reviewed May 21, 2021

View reviewed changes

pkg/actions/nodegroup/create.go Show resolved Hide resolved

aclevername reviewed May 21, 2021

View reviewed changes

pkg/actions/nodegroup/nodegroup.go Outdated Show resolved Hide resolved

aclevername reviewed May 21, 2021

View reviewed changes

pkg/cfn/manager/create_tasks.go Outdated Show resolved Hide resolved

nikimanoledaki requested a review from aclevername May 25, 2021 11:57

aclevername reviewed May 25, 2021

View reviewed changes

aclevername approved these changes May 25, 2021

View reviewed changes

nikimanoledaki force-pushed the test-ng branch 2 times, most recently from 2ccf506 to a932fda Compare May 26, 2021 14:56

Callisto13 force-pushed the test-ng branch from a932fda to 99e732e Compare May 27, 2021 09:12

Callisto13 reviewed May 27, 2021

View reviewed changes

nikimanoledaki mentioned this pull request May 27, 2021

Make the Provider less powerful #2931

Closed

nikimanoledaki force-pushed the test-ng branch 3 times, most recently from e7bfd41 to 6feb6d0 Compare May 27, 2021 13:57

nikimanoledaki added 7 commits May 27, 2021 16:01

Adds integration and unit tests for creating ng

572afbe

Unit testing Create Nodegroup

84be31f

* Test for failing to load VPC from config * Test when cluster does not support managed nodes

Create NodeGroupInitialiser for helper funcs

c843d03

Revert work from branch 'upgrade-ng'

b1f0f2d

Unit test create nodegroups for cluster compatibility, stacklister

b6100d5

* Test for when cluster is not compatible with ng config * Test for when existing local ng stacks in config file fail to be listed

nikimanoledaki and others added 5 commits May 27, 2021 16:01

Move ConfigMap func to KubeProvider

b4d99ef

Refactor kubeProvider to pass to ng Manager struct

1cf078c

Use NodegroupFilter with user config and update its interface

99b29c3

Implement review changes to ng creation PR

f5a4306

* Move ValidateExistingNodeGroupsForCompatibility to nodegroup initialiser interface * Move DoAllNodegroupStackTasks to nodegroup initialiser interface

Implement review feedback

7a59b28

* Get rid of counterfeiter warning when generating mocks * Update doc comments * Rename unit test names to more descriptive names Co-authored-by: Claudia <claudiaberesford@gmail.com>

nikimanoledaki force-pushed the test-ng branch from 6feb6d0 to 7a59b28 Compare May 27, 2021 14:01

nikimanoledaki requested a review from Callisto13 May 27, 2021 14:04

Callisto13 approved these changes May 27, 2021

View reviewed changes

nikimanoledaki merged commit 516eabd into main May 27, 2021

nikimanoledaki deleted the test-ng branch May 27, 2021 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and increase test coverage for creating nodegroups #3728

Refactor and increase test coverage for creating nodegroups #3728

nikimanoledaki commented May 18, 2021 •

edited

Loading

nikimanoledaki commented May 25, 2021

aclevername May 25, 2021

Callisto13 May 27, 2021

aclevername left a comment

Callisto13 left a comment

Callisto13 left a comment

Refactor and increase test coverage for creating nodegroups #3728

Refactor and increase test coverage for creating nodegroups #3728

Conversation

nikimanoledaki commented May 18, 2021 • edited Loading

Description

Checklist

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

nikimanoledaki commented May 25, 2021

aclevername May 25, 2021

Choose a reason for hiding this comment

Callisto13 May 27, 2021

Choose a reason for hiding this comment

aclevername left a comment

Choose a reason for hiding this comment

Callisto13 left a comment

Choose a reason for hiding this comment

Callisto13 left a comment

Choose a reason for hiding this comment

nikimanoledaki commented May 18, 2021 •

edited

Loading