Additional Code Linters (#1304)

*dupl threshold that aligns with controller-runtime
* lll as a linter with a line length limit of 250

Signed-off-by: Ken Sipe <kensipe@gmail.com>

.golangci.yml

@@ -9,13 +9,23 @@ linters:
     - scopelint
     - unconvert
     - unparam
+    - interfacer
+    - nakedret
+    - gocyclo
+    - dupl
+    - goconst
+    - lll
 run:
   skip-dirs:
     # autogenerated clientset by client-gen
     - pkg/client
 linters-settings:
   errcheck:
     check-type-assertions: true
+  lll:
+    line-length: 250
+  dupl:
+    threshold: 400
   goimports:
     # Don't use 'github.com/kudobuilder/kudo', it'll result in unreliable output!
     local-prefixes: github.com/kudobuilder

hack/install-golangcilint.sh

@@ -4,6 +4,6 @@ set -o errexit
 set -o nounset
 set -o pipefail
 
-GOLANGCILINT_VERSION=${GOLANGCILINT_VERSION:-v1.22.0}
+GOLANGCILINT_VERSION=${GOLANGCILINT_VERSION:-v1.23.1}
 
 curl -sSfL "https://raw.githubusercontent.com/golangci/golangci-lint/${GOLANGCILINT_VERSION}/install.sh" | sh -s -- -b "$(go env GOPATH)/bin" "${GOLANGCILINT_VERSION}"

pkg/controller/instance/instance_controller.go

@@ -575,7 +575,8 @@ func getPlanToBeExecuted(i *v1beta1.Instance, ov *v1beta1.OperatorVersion) (*str
 		log.Printf("Instance: instance %s/%s was upgraded from %s to %s operatorVersion", i.Namespace, i.Name, instanceSnapshot.OperatorVersion.Name, i.Spec.OperatorVersion.Name)
 		plan := selectPlan([]string{v1beta1.UpgradePlanName, v1beta1.UpdatePlanName, v1beta1.DeployPlanName}, ov)
 		if plan == nil {
-			return nil, &v1beta1.InstanceError{Err: fmt.Errorf("supposed to execute plan because instance %s/%s was upgraded but none of the deploy, upgrade, update plans found in linked operatorVersion", i.Namespace, i.Name), EventName: kudo.String("PlanNotFound")}
+			return nil, &v1beta1.InstanceError{Err: fmt.Errorf("supposed to execute plan because instance %s/%s was upgraded but none of the deploy, upgrade, update plans found in linked operatorVersion", i.Namespace, i.Name),
+				EventName: kudo.String("PlanNotFound")}
 		}
 		return plan, nil
 	}

pkg/engine/renderer/enhancer.go

@@ -83,7 +83,7 @@ func (k *DefaultEnhancer) Apply(templates map[string]string, metadata Metadata)
 	return objs, nil
 }
 
-func setControllerReference(owner v1.Object, object *unstructured.Unstructured, scheme *runtime.Scheme) error {
+func setControllerReference(owner v1.Object, object v1.Object, scheme *runtime.Scheme) error {
 	ownerNs := owner.GetNamespace()
 	if ownerNs != "" {
 		objNs := object.GetNamespace()

pkg/engine/workflow/engine_test.go

@@ -175,7 +175,8 @@ func TestExecutePlan(t *testing.T) {
 			expectedStatus: &v1beta1.PlanStatus{
 				Status: v1beta1.ExecutionInProgress,
 				Name:   "test",
-				Phases: []v1beta1.PhaseStatus{{Name: "phase", Status: v1beta1.ExecutionInProgress, Steps: []v1beta1.StepStatus{{Status: v1beta1.ErrorStatus, Name: "step", Message: "A transient error when executing task test.phase.step.task. Will retry. dummy error"}}}},
+				Phases: []v1beta1.PhaseStatus{{Name: "phase", Status: v1beta1.ExecutionInProgress, Steps: []v1beta1.StepStatus{{Status: v1beta1.ErrorStatus, Name: "step",
+					Message: "A transient error when executing task test.phase.step.task. Will retry. dummy error"}}}},
 			},
 			enhancer: testEnhancer,
 		},
@@ -207,7 +208,8 @@ func TestExecutePlan(t *testing.T) {
 			expectedStatus: &v1beta1.PlanStatus{
 				Status: v1beta1.ExecutionFatalError,
 				Name:   "test",
-				Phases: []v1beta1.PhaseStatus{{Name: "phase", Status: v1beta1.ExecutionFatalError, Steps: []v1beta1.StepStatus{{Status: v1beta1.ExecutionFatalError, Message: "Error during execution: fatal error: default/test-instance failed in test.phase.step.task: dummy error", Name: "step"}}}},
+				Phases: []v1beta1.PhaseStatus{{Name: "phase", Status: v1beta1.ExecutionFatalError, Steps: []v1beta1.StepStatus{{Status: v1beta1.ExecutionFatalError,
+					Message: "Error during execution: fatal error: default/test-instance failed in test.phase.step.task: dummy error", Name: "step"}}}},
 			},
 			wantErr:  true,
 			enhancer: testEnhancer,
@@ -240,7 +242,8 @@ func TestExecutePlan(t *testing.T) {
 			expectedStatus: &v1beta1.PlanStatus{
 				Status: v1beta1.ExecutionFatalError,
 				Name:   "test",
-				Phases: []v1beta1.PhaseStatus{{Name: "phase", Status: v1beta1.ExecutionFatalError, Steps: []v1beta1.StepStatus{{Status: v1beta1.ExecutionFatalError, Message: "default/test-instance fatal error:  missing task test.phase.step.fake-task", Name: "step"}}}},
+				Phases: []v1beta1.PhaseStatus{{Name: "phase", Status: v1beta1.ExecutionFatalError, Steps: []v1beta1.StepStatus{{Status: v1beta1.ExecutionFatalError,
+					Message: "default/test-instance fatal error:  missing task test.phase.step.fake-task", Name: "step"}}}},
 			},
 			wantErr:  true,
 			enhancer: testEnhancer,
@@ -271,7 +274,8 @@ func TestExecutePlan(t *testing.T) {
 			expectedStatus: &v1beta1.PlanStatus{
 				Status: v1beta1.ExecutionFatalError,
 				Name:   "test",
-				Phases: []v1beta1.PhaseStatus{{Name: "phase", Status: v1beta1.ExecutionFatalError, Steps: []v1beta1.StepStatus{{Status: v1beta1.ExecutionFatalError, Name: "step", Message: "default/test-instance fatal error:  failed to build task test.phase.step.task: unknown task kind Unknown"}}}},
+				Phases: []v1beta1.PhaseStatus{{Name: "phase", Status: v1beta1.ExecutionFatalError, Steps: []v1beta1.StepStatus{{Status: v1beta1.ExecutionFatalError, Name: "step",
+					Message: "default/test-instance fatal error:  failed to build task test.phase.step.task: unknown task kind Unknown"}}}},
 			},
 			wantErr:  true,
 			enhancer: testEnhancer,
@@ -555,7 +559,8 @@ func TestExecutePlan(t *testing.T) {
 				Status: v1beta1.ExecutionFatalError,
 				Name:   "test",
 				Phases: []v1beta1.PhaseStatus{
-					{Name: "phaseOne", Status: v1beta1.ExecutionFatalError, Steps: []v1beta1.StepStatus{{Name: "step", Status: v1beta1.ExecutionFatalError, Message: "Error during execution: fatal error: default/test-instance failed in test.phaseOne.step.taskOne: dummy error"}}},
+					{Name: "phaseOne", Status: v1beta1.ExecutionFatalError, Steps: []v1beta1.StepStatus{{Name: "step", Status: v1beta1.ExecutionFatalError,
+						Message: "Error during execution: fatal error: default/test-instance failed in test.phaseOne.step.taskOne: dummy error"}}},
 					{Name: "phaseTwo", Status: v1beta1.ExecutionInProgress, Steps: []v1beta1.StepStatus{{Name: "step", Status: v1beta1.ExecutionInProgress}}},
 				},
 			},
@@ -592,7 +597,8 @@ func TestExecutePlan(t *testing.T) {
 			expectedStatus: &v1beta1.PlanStatus{
 				Status: v1beta1.ExecutionFatalError,
 				Name:   "test",
-				Phases: []v1beta1.PhaseStatus{{Name: "phase", Status: v1beta1.ExecutionFatalError, Steps: []v1beta1.StepStatus{{Status: v1beta1.ExecutionFatalError, Name: "step", Message: "Error during execution: fatal error: default/test-instance failed in test.phase.step.task: dummy error"}}}}},
+				Phases: []v1beta1.PhaseStatus{{Name: "phase", Status: v1beta1.ExecutionFatalError, Steps: []v1beta1.StepStatus{{Status: v1beta1.ExecutionFatalError, Name: "step",
+					Message: "Error during execution: fatal error: default/test-instance failed in test.phase.step.task: dummy error"}}}}},
 			wantErr:  true,
 			enhancer: testEnhancer,
 		},

pkg/kudoctl/cmd/generate/parameter_test.go

@@ -15,7 +15,7 @@ import (
 
 func TestAddParameter(t *testing.T) {
 	goldenFile := "parameter"
-	path := "/opt/zk"
+	path := "/opt/zk" //nolint:goconst
 	fs := afero.NewMemMapFs()
 	files.CopyOperatorToFs(fs, "../../packages/testdata/zk", "/opt")
 

pkg/kudoctl/cmd/upgrade.go

@@ -62,8 +62,10 @@ func newUpgradeCmd(fs afero.Fs) *cobra.Command {
 	upgradeCmd.Flags().StringVar(&options.InstanceName, "instance", "", "The instance name.")
 	upgradeCmd.Flags().StringArrayVarP(&parameters, "parameter", "p", nil, "The parameter name and value separated by '='")
 	upgradeCmd.Flags().StringVar(&options.RepoName, "repo", "", "Name of repository configuration to use. (default defined by context)")
-	upgradeCmd.Flags().StringVar(&options.AppVersion, "app-version", "", "A specific app version in the official repository. When installing from other sources than an official repository, a version from inside operator.yaml will be used. (default to the most recent)")
-	upgradeCmd.Flags().StringVar(&options.OperatorVersion, "operator-version", "", "A specific operator version in the official repository. When installing from other sources than an official repository, a version from inside operator.yaml will be used. (default to the most recent)")
+	upgradeCmd.Flags().StringVar(&options.AppVersion, "app-version", "",
+		"A specific app version in the official repository. When installing from other sources than an official repository, a version from inside operator.yaml will be used. (default to the most recent)")
+	upgradeCmd.Flags().StringVar(&options.OperatorVersion, "operator-version", "",
+		"A specific operator version in the official repository. When installing from other sources than an official repository, a version from inside operator.yaml will be used. (default to the most recent)")
 
 	return upgradeCmd
 }

pkg/kudoctl/kudoinit/setup/wait.go

@@ -49,7 +49,7 @@ func getKUDOPodImage(client corev1.PodsGetter, namespace string) (string, error)
 	return "", errors.New("could not find a KUDO pod")
 }
 
-func getFirstRunningPod(client corev1.PodsGetter, namespace string, selector labels.Selector) (*v1.Pod, error) {
+func getFirstRunningPod(client corev1.PodsGetter, namespace string, selector labels.Selector) (*v1.Pod, error) { //nolint:interfacer
 	options := metav1.ListOptions{LabelSelector: selector.String()}
 	pods, err := client.Pods(namespace).List(options)
 	if err != nil {

pkg/kudoctl/util/kudo/install_test.go

@@ -89,7 +89,7 @@ func Test_InstallPackage(t *testing.T) {
 
 		testResources := resources
 		testResources.OperatorVersion.Spec.Parameters = tt.parameters
-		namespace := "default"
+		namespace := "default" //nolint:goconst
 
 		err := InstallPackage(kc, &testResources, tt.skipInstance, "", namespace, tt.installParameters)
 		if tt.err != "" {

pkg/kudoctl/util/kudo/kudo.go

@@ -306,7 +306,7 @@ func (c *Client) ValidateServerForOperator(operator *v1beta1.Operator) error {
 }
 
 // getKubeVersion returns stringified version of k8s server
-func getKubeVersion(client discovery.DiscoveryInterface) (string, error) {
+func getKubeVersion(client discovery.ServerVersionInterface) (string, error) {
 	v, err := client.ServerVersion()
 	if err != nil {
 		return "", err

pkg/test/case.go

@@ -14,6 +14,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	eventsbeta1 "k8s.io/api/events/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/conversion"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/discovery"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -112,7 +113,7 @@ func (t *Case) CollectEvents(namespace string) {
 	printEvents(events, t.Logger)
 }
 
-func printEvents(events []eventsbeta1.Event, logger testutils.Logger) {
+func printEvents(events []eventsbeta1.Event, logger conversion.DebugLogger) {
 	for _, e := range events {
 		// time type reason kind message
 		logger.Logf("%s\t%s\t%s\t%s", e.ObjectMeta.CreationTimestamp, e.Type, e.Reason, e.Note)

pkg/test/step_test.go

@@ -18,15 +18,18 @@ import (
 	testutils "github.com/kudobuilder/kudo/pkg/test/utils"
 )
 
+const (
+	testNamespace = "world"
+)
+
 // Verify the test state as loaded from disk.
 // Each test provides a path to a set of test steps and their rendered result.
 func TestStepClean(t *testing.T) {
-	namespace := "world"
 
 	pod := testutils.NewPod("hello", "")
 
-	podWithNamespace := testutils.WithNamespace(pod, namespace)
-	pod2WithNamespace := testutils.NewPod("hello2", namespace)
+	podWithNamespace := testutils.WithNamespace(pod, testNamespace)
+	pod2WithNamespace := testutils.NewPod("hello2", testNamespace)
 	pod2WithDiffNamespace := testutils.NewPod("hello2", "different-namespace")
 
 	cl := fake.NewFakeClient(pod, pod2WithNamespace, pod2WithDiffNamespace)
@@ -39,7 +42,7 @@ func TestStepClean(t *testing.T) {
 		DiscoveryClient: func() (discovery.DiscoveryInterface, error) { return testutils.FakeDiscoveryClient(), nil },
 	}
 
-	assert.Nil(t, step.Clean(namespace))
+	assert.Nil(t, step.Clean(testNamespace))
 
 	assert.True(t, k8serrors.IsNotFound(cl.Get(context.TODO(), testutils.ObjectKey(podWithNamespace), podWithNamespace)))
 	assert.Nil(t, cl.Get(context.TODO(), testutils.ObjectKey(pod2WithNamespace), pod2WithNamespace))
@@ -49,7 +52,6 @@ func TestStepClean(t *testing.T) {
 // Verify the test state as loaded from disk.
 // Each test provides a path to a set of test steps and their rendered result.
 func TestStepCreate(t *testing.T) {
-	namespace := "world"
 
 	pod := testutils.NewPod("hello", "")
 	podWithNamespace := testutils.NewPod("hello2", "different-namespace")
@@ -60,7 +62,7 @@ func TestStepCreate(t *testing.T) {
 	}
 	updateToApply := testutils.WithSpec(t, podToUpdate, specToApply)
 
-	cl := fake.NewFakeClient(testutils.WithNamespace(podToUpdate, "world"))
+	cl := fake.NewFakeClient(testutils.WithNamespace(podToUpdate, testNamespace))
 
 	step := Step{
 		Logger: testutils.NewTestLogger(t, ""),
@@ -71,7 +73,7 @@ func TestStepCreate(t *testing.T) {
 		DiscoveryClient: func() (discovery.DiscoveryInterface, error) { return testutils.FakeDiscoveryClient(), nil },
 	}
 
-	assert.Equal(t, []error{}, step.Create(namespace))
+	assert.Equal(t, []error{}, step.Create(testNamespace))
 
 	assert.Nil(t, cl.Get(context.TODO(), testutils.ObjectKey(pod), pod))
 	assert.Nil(t, cl.Get(context.TODO(), testutils.ObjectKey(clusterScopedResource), clusterScopedResource))
@@ -81,17 +83,16 @@ func TestStepCreate(t *testing.T) {
 	assert.Equal(t, specToApply, updatedPod.Object["spec"])
 
 	assert.Nil(t, cl.Get(context.TODO(), testutils.ObjectKey(podWithNamespace), podWithNamespace))
-	actual := testutils.NewPod("hello2", namespace)
+	actual := testutils.NewPod("hello2", testNamespace)
 	assert.True(t, k8serrors.IsNotFound(cl.Get(context.TODO(), testutils.ObjectKey(actual), actual)))
 }
 
 // Verify that the DeleteExisting method properly cleans up resources during a test step.
 func TestStepDeleteExisting(t *testing.T) {
-	namespace := "world"
 
-	podToDelete := testutils.NewPod("delete-me", "world")
+	podToDelete := testutils.NewPod("delete-me", testNamespace)
 	podToDeleteDefaultNS := testutils.NewPod("also-delete-me", "default")
-	podToKeep := testutils.NewPod("keep-me", "world")
+	podToKeep := testutils.NewPod("keep-me", testNamespace)
 
 	cl := fake.NewFakeClient(podToDelete, podToKeep, podToDeleteDefaultNS)
 
@@ -124,7 +125,7 @@ func TestStepDeleteExisting(t *testing.T) {
 	assert.Nil(t, cl.Get(context.TODO(), testutils.ObjectKey(podToDelete), podToDelete))
 	assert.Nil(t, cl.Get(context.TODO(), testutils.ObjectKey(podToDeleteDefaultNS), podToDeleteDefaultNS))
 
-	assert.Nil(t, step.DeleteExisting(namespace))
+	assert.Nil(t, step.DeleteExisting(testNamespace))
 
 	assert.Nil(t, cl.Get(context.TODO(), testutils.ObjectKey(podToKeep), podToKeep))
 	assert.True(t, k8serrors.IsNotFound(cl.Get(context.TODO(), testutils.ObjectKey(podToDelete), podToDelete)))
@@ -170,7 +171,7 @@ func TestCheckResource(t *testing.T) {
 
 		t.Run(test.testName, func(t *testing.T) {
 			fakeDiscovery := testutils.FakeDiscoveryClient()
-			namespace := "world"
+			namespace := testNamespace
 
 			_, _, err := testutils.Namespaced(fakeDiscovery, test.actual, namespace)
 			assert.Nil(t, err)
@@ -220,9 +221,8 @@ func TestCheckResourceAbsent(t *testing.T) {
 
 		t.Run(test.name, func(t *testing.T) {
 			fakeDiscovery := testutils.FakeDiscoveryClient()
-			namespace := "world"
 
-			_, _, err := testutils.Namespaced(fakeDiscovery, test.actual, namespace)
+			_, _, err := testutils.Namespaced(fakeDiscovery, test.actual, testNamespace)
 			assert.Nil(t, err)
 
 			step := Step{
@@ -231,7 +231,7 @@ func TestCheckResourceAbsent(t *testing.T) {
 				DiscoveryClient: func() (discovery.DiscoveryInterface, error) { return fakeDiscovery, nil },
 			}
 
-			error := step.CheckResourceAbsent(test.expected, namespace)
+			error := step.CheckResourceAbsent(test.expected, testNamespace)
 
 			if test.shouldError {
 				assert.NotNil(t, error)
@@ -283,7 +283,7 @@ func TestRun(t *testing.T) {
 				},
 			}, func(t *testing.T, client client.Client) {
 				// mock kubelet to set the pod status
-				assert.Nil(t, client.Update(context.TODO(), testutils.WithStatus(t, testutils.NewPod("hello", "world"), map[string]interface{}{
+				assert.Nil(t, client.Update(context.TODO(), testutils.WithStatus(t, testutils.NewPod("hello", testNamespace), map[string]interface{}{
 					"phase": "Ready",
 				})))
 			},
@@ -311,7 +311,7 @@ func TestRun(t *testing.T) {
 				}()
 			}
 
-			errors := test.Step.Run("world")
+			errors := test.Step.Run(testNamespace)
 
 			if test.shouldError {
 				assert.NotEqual(t, []error{}, errors)

pkg/test/utils/kubernetes.go

@@ -349,11 +349,12 @@ func ConvertUnstructured(in runtime.Object) (runtime.Object, error) {
 	kind := in.GetObjectKind().GroupVersionKind().Kind
 	group := in.GetObjectKind().GroupVersionKind().Group
 
-	if group == "kudo.dev" && kind == "TestStep" {
+	kudoGroup := "kudo.dev"
+	if group == kudoGroup && kind == "TestStep" {
 		converted = &harness.TestStep{}
-	} else if group == "kudo.dev" && kind == "TestAssert" {
+	} else if group == kudoGroup && kind == "TestAssert" {
 		converted = &harness.TestAssert{}
-	} else if group == "kudo.dev" && kind == "TestSuite" {
+	} else if group == kudoGroup && kind == "TestSuite" {
 		converted = &harness.TestSuite{}
 	} else {
 		return in, nil