[Spike] Investigate how to improve unit test coverage for 'ctl' package

[TiberiuGC]

Unit test coverage for the ctl package mostly consists of checking correct usage of the commands e.g. all required flags are specified, the command throws validation errors if incorrect flags or flags that shouldn't be combined are being provided, etc. (i.e. only validation related bits). While the ctl package is usually not doing much more than that, there are exceptions. E.g. the create iamserviceaccount command sets up a filter to decide whether to override or not existing service accounts.

https://github.com/weaveworks/eksctl/blob/27219282eeb35e2062ba8440dbf724cdd034099d/pkg/ctl/create/iamserviceaccount.go#L113-L118

Our tests should ideally cover such pieces of functionality as-well. Most of this logic however is being written at some point after initialising the cluster provider.

https://github.com/weaveworks/eksctl/blob/27219282eeb35e2062ba8440dbf724cdd034099d/pkg/ctl/create/iamserviceaccount.go#L83-L86

We should investigate if there's a reasonable way to mock the provider and then increase test coverage, separately, for each relevant command.

As part of this spike we should also determine if there still are commands completely missing unit test coverage.

Timebox: 1-2 days

[TiberiuGC]

Posting findings here:

What is the main deterrent?

Most of the commands in the CTL package hardcode the cluster provider by using the function call below.


ctl, err := cmd.NewProviderForExistingCluster(ctx)
if err != nil {
    return err
}

However, very little of the provider’s functionality is actually being used in the CTL package. See a list below:

ctl.CanOperate
ctl.NewOpenIDConnectManager
ctl.NewStdClientSet
ctl.NewRawClient
ctl.AWSProvider.EKS
ctl.AWSProvider.AWSConfig
ctl.AWSProvider.EC2

Moreover, some of the functions (first two) are not even part of any interfaces that the provider consists of, making it mode difficult to mock them in a clean way.

How to overcome it?

Option 1

Simply pass the cluster provider as a function argument in the following template

eksctlCommand() {
	…
	cmd.CobraCommand.RunE = func(_ *cobra.Command, args []string) error {
        	…
		return doEksctlCommand(…, clusterProvider)
    }
}


Afterwards we can use existing mocks as in - FakeKubeProvider, MockProvider and/or configure ProviderStatus for ctl.CanOperate calls. Additionally we will probably need another interface as part of the clusterProvider just for ctl.NewOpenIDConnectManager calls.

Option 2

Similar to option 1 in terms of template, but additionally create an interface that amasses the functions calls referenced above, and mock them using counterfeiter.

In addition to the above work, each command usually relies on a manager struct inside actions package for doing the actual work. We will need interfaces and mocks for these managers as-well.

What functionality should be tested?

First thing that comes to mind and is a common factor for many commands is unit testing the cluster loaders (i.e. the logic for reading cluster config files) e.g.

eksctl/pkg/ctl/create/nodegroup.go

Lines 43 to 45 in 1807c5e

	if err := cmdutils.NewCreateNodeGroupLoader(cmd, ng, ngFilter, options.CreateNGOptions, options.CreateManagedNGOptions).Load(); err != nil {
	return errors.Wrap(err, "couldn't create node group filter from command line options")
	}

Another relevant bit is the outputs of get commands, in all their various formats. e.g.

eksctl/pkg/ctl/get/nodegroup.go

Lines 96 to 114 in 1807c5e

	printer, err := printers.NewPrinter(params.output)
	if err != nil {
	return err
	}
	if params.output == printers.TableType {
	// Empty summary implies no nodegroups
	// We only error if the output is table, since if the output
	// is yaml or json we should return an empty object.
	if len(summaries) == 0 {
	if ng.Name == "" {
	return errors.Errorf("No nodegroups found")
	}
	return errors.Errorf("nodegroup with name %v not found", ng.Name)
	}
	addSummaryTableColumns(printer.(*printers.TablePrinter))
	}
	return printer.PrintObjWithKind("nodegroups", summaries, cmd.CobraCommand.OutOrStdout())

Otherwise, each command differs in functionality and should be evaluated individually. If there are places where there’s too much logic inside the ctl package, mixing CLI logic with business related logic, the latter should be moved to e.g. actions package and tested accordingly. Most relevant example is cluster creation, however this is already quite thoroughly tested and may only need some refactoring, so probably out of the scope of this ticket.

Next steps

Decide on which option to go for and split the work into tickets based on logical area (e.g. addons related commands, nodegroup related commands, etc.)