Enable the unparam linter, and fix outstanding issues.

dlorenc · tekton-robot · commit 6c5ff05ae93e · 2019-10-09T17:44:17.000-05:00
This linter checks for unused function parameters and return values.
We had quite a few of these, but the fixes were easy.
diff --git a/.golangci.yml b/.golangci.yml
@@ -14,3 +14,4 @@ linters:
   - goimports
   - gosec
   - gocritic
+  - unparam
diff --git a/pkg/apis/pipeline/v1alpha1/build_gcs_resource.go b/pkg/apis/pipeline/v1alpha1/build_gcs_resource.go
@@ -143,10 +143,7 @@ func (s *BuildGCSResource) GetInputTaskModifier(ts *TaskSpec, sourcePath string)
 			Args:    args,
 		}}}
 
-	volumes, err := getStorageVolumeSpec(s, *ts)
-	if err != nil {
-		return nil, err
-	}
+	volumes := getStorageVolumeSpec(s, *ts)
 
 	return &InternalTaskModifier{
 		StepsToPrepend: steps,
diff --git a/pkg/apis/pipeline/v1alpha1/gcs_resource.go b/pkg/apis/pipeline/v1alpha1/gcs_resource.go
@@ -121,10 +121,7 @@ func (s *GCSResource) GetOutputTaskModifier(ts *TaskSpec, path string) (TaskModi
 		Env:          envVars},
 	}
 
-	volumes, err := getStorageVolumeSpec(s, *ts)
-	if err != nil {
-		return nil, err
-	}
+	volumes := getStorageVolumeSpec(s, *ts)
 
 	return &InternalTaskModifier{
 		StepsToAppend: []Step{step},
@@ -156,10 +153,7 @@ func (s *GCSResource) GetInputTaskModifier(ts *TaskSpec, path string) (TaskModif
 			VolumeMounts: secretVolumeMount,
 		}}}
 
-	volumes, err := getStorageVolumeSpec(s, *ts)
-	if err != nil {
-		return nil, err
-	}
+	volumes := getStorageVolumeSpec(s, *ts)
 
 	return &InternalTaskModifier{
 		StepsToPrepend: steps,
diff --git a/pkg/apis/pipeline/v1alpha1/storage_resource.go b/pkg/apis/pipeline/v1alpha1/storage_resource.go
@@ -59,7 +59,7 @@ func NewStorageResource(images pipeline.Images, r *PipelineResource) (PipelineSt
 	return nil, xerrors.Errorf("StoreResource: Cannot create a storage resource without type %s in spec", r.Name)
 }
 
-func getStorageVolumeSpec(s PipelineStorageResourceInterface, spec TaskSpec) ([]corev1.Volume, error) {
+func getStorageVolumeSpec(s PipelineStorageResourceInterface, spec TaskSpec) []corev1.Volume {
 	var storageVol []corev1.Volume
 	mountedSecrets := map[string]string{}
 
@@ -85,5 +85,5 @@ func getStorageVolumeSpec(s PipelineStorageResourceInterface, spec TaskSpec) ([]
 			mountedSecrets[volName] = ""
 		}
 	}
-	return storageVol, nil
+	return storageVol
 }
diff --git a/pkg/git/git.go b/pkg/git/git.go
@@ -26,13 +26,13 @@ import (
 	"golang.org/x/xerrors"
 )
 
-func run(logger *zap.SugaredLogger, cmd string, args ...string) error {
-	c := exec.Command(cmd, args...)
+func run(logger *zap.SugaredLogger, args ...string) error {
+	c := exec.Command("git", args...)
 	var output bytes.Buffer
 	c.Stderr = &output
 	c.Stdout = &output
 	if err := c.Run(); err != nil {
-		logger.Errorf("Error running %v %v: %v\n%v", cmd, args, err, output.String())
+		logger.Errorf("Error running git %v: %v\n%v", args, err, output.String())
 		return err
 	}
 	return nil
@@ -70,29 +70,29 @@ func Fetch(logger *zap.SugaredLogger, revision, path, url string) error {
 		revision = "master"
 	}
 	if path != "" {
-		if err := run(logger, "git", "init", path); err != nil {
+		if err := run(logger, "init", path); err != nil {
 			return err
 		}
 		if err := os.Chdir(path); err != nil {
 			return xerrors.Errorf("Failed to change directory with path %s; err: %w", path, err)
 		}
-	} else if err := run(logger, "git", "init"); err != nil {
+	} else if err := run(logger, "init"); err != nil {
 		return err
 	}
 	trimmedURL := strings.TrimSpace(url)
-	if err := run(logger, "git", "remote", "add", "origin", trimmedURL); err != nil {
+	if err := run(logger, "remote", "add", "origin", trimmedURL); err != nil {
 		return err
 	}
-	if err := run(logger, "git", "fetch", "--depth=1", "--recurse-submodules=yes", "origin", revision); err != nil {
+	if err := run(logger, "fetch", "--depth=1", "--recurse-submodules=yes", "origin", revision); err != nil {
 		// Fetch can fail if an old commitid was used so try git pull, performing regardless of error
 		// as no guarantee that the same error is returned by all git servers gitlab, github etc...
-		if err := run(logger, "git", "pull", "--recurse-submodules=yes", "origin"); err != nil {
+		if err := run(logger, "pull", "--recurse-submodules=yes", "origin"); err != nil {
 			logger.Warnf("Failed to pull origin : %s", err)
 		}
-		if err := run(logger, "git", "checkout", revision); err != nil {
+		if err := run(logger, "checkout", revision); err != nil {
 			return err
 		}
-	} else if err := run(logger, "git", "reset", "--hard", "FETCH_HEAD"); err != nil {
+	} else if err := run(logger, "reset", "--hard", "FETCH_HEAD"); err != nil {
 		return err
 	}
 	logger.Infof("Successfully cloned %s @ %s in path %s", trimmedURL, revision, path)
diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go
@@ -1779,7 +1779,11 @@ func TestResolvedConditionCheck_WithResources(t *testing.T) {
 	getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil }
 	getTaskRun := func(name string) (*v1alpha1.TaskRun, error) { return nil, nil }
 	getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return nil, xerrors.New("should not get called") }
-	getCondition := func(name string) (*v1alpha1.Condition, error) {
+
+	// This err result is required to satisfy the type alias on this function, but it triggers
+	// a false positive in the linter: https://github.com/mvdan/unparam/issues/40
+	// nolint: unparam
+	getCondition := func(_ string) (*v1alpha1.Condition, error) {
 		return condition, nil
 	}
 
diff --git a/pkg/reconciler/taskrun/resources/input_resource_test.go b/pkg/reconciler/taskrun/resources/input_resource_test.go
@@ -81,7 +81,7 @@ var (
 	}
 )
 
-func setUp(t *testing.T) {
+func setUp() {
 	logger, _ = logging.NewLogger("", "")
 
 	rs := []*v1alpha1.PipelineResource{{
@@ -740,7 +740,7 @@ func TestAddResourceToTask(t *testing.T) {
 		},
 	}} {
 		t.Run(c.desc, func(t *testing.T) {
-			setUp(t)
+			setUp()
 			names.TestingSeed()
 			fakekubeclient := fakek8s.NewSimpleClientset()
 			got, err := AddInputResource(fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, mockResolveTaskResources(c.taskRun), logger)
@@ -923,7 +923,7 @@ func TestStorageInputResource(t *testing.T) {
 	}} {
 		t.Run(c.desc, func(t *testing.T) {
 			names.TestingSeed()
-			setUp(t)
+			setUp()
 			fakekubeclient := fakek8s.NewSimpleClientset()
 			got, err := AddInputResource(fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, mockResolveTaskResources(c.taskRun), logger)
 			if (err != nil) != c.wantErr {
@@ -1043,7 +1043,7 @@ func TestAddStepsToTaskWithBucketFromConfigMap(t *testing.T) {
 		},
 	}} {
 		t.Run(c.desc, func(t *testing.T) {
-			setUp(t)
+			setUp()
 			fakekubeclient := fakek8s.NewSimpleClientset(
 				&corev1.ConfigMap{
 					ObjectMeta: metav1.ObjectMeta{
diff --git a/pkg/reconciler/taskrun/resources/output_resource_test.go b/pkg/reconciler/taskrun/resources/output_resource_test.go
@@ -32,7 +32,7 @@ var (
 	outputResources map[string]v1alpha1.PipelineResourceInterface
 )
 
-func outputResourceSetup(t *testing.T) {
+func outputResourceSetup() {
 	logger, _ = logging.NewLogger("", "")
 
 	rs := []*v1alpha1.PipelineResource{{
@@ -787,7 +787,7 @@ func TestValidOutputResources(t *testing.T) {
 	}} {
 		t.Run(c.name, func(t *testing.T) {
 			names.TestingSeed()
-			outputResourceSetup(t)
+			outputResourceSetup()
 			fakekubeclient := fakek8s.NewSimpleClientset()
 			got, err := AddOutputResources(fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, resolveOutputResources(c.taskRun), logger)
 			if err != nil {
@@ -992,7 +992,7 @@ func TestValidOutputResourcesWithBucketStorage(t *testing.T) {
 		}}},
 	}} {
 		t.Run(c.name, func(t *testing.T) {
-			outputResourceSetup(t)
+			outputResourceSetup()
 			names.TestingSeed()
 			fakekubeclient := fakek8s.NewSimpleClientset(
 				&corev1.ConfigMap{
@@ -1151,7 +1151,7 @@ func TestInvalidOutputResources(t *testing.T) {
 		wantErr: true,
 	}} {
 		t.Run(c.desc, func(t *testing.T) {
-			outputResourceSetup(t)
+			outputResourceSetup()
 			fakekubeclient := fakek8s.NewSimpleClientset()
 			_, err := AddOutputResources(fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, resolveOutputResources(c.taskRun), logger)
 			if (err != nil) != c.wantErr {
diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go
@@ -312,7 +312,7 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error
 
 	status.SortTaskRunStepOrder(tr.Status.Steps, taskSpec.Steps)
 
-	updateTaskRunResourceResult(tr, pod, c.resourceLister, c.KubeClientSet, c.Logger)
+	updateTaskRunResourceResult(tr, pod, c.resourceLister, c.Logger)
 
 	after := tr.Status.GetCondition(apis.ConditionSucceeded)
 
@@ -358,7 +358,7 @@ func (c *Reconciler) handlePodCreationError(tr *v1alpha1.TaskRun, err error) {
 	c.Logger.Errorf("Failed to create build pod for task %q: %v", tr.Name, err)
 }
 
-func updateTaskRunResourceResult(taskRun *v1alpha1.TaskRun, pod *corev1.Pod, resourceLister listers.PipelineResourceLister, kubeclient kubernetes.Interface, logger *zap.SugaredLogger) {
+func updateTaskRunResourceResult(taskRun *v1alpha1.TaskRun, pod *corev1.Pod, resourceLister listers.PipelineResourceLister, logger *zap.SugaredLogger) {
 	if resources.TaskRunHasOutputImageResource(resourceLister.PipelineResources(taskRun.Namespace).Get, taskRun) && taskRun.IsSuccessful() {
 		for _, cs := range pod.Status.ContainerStatuses {
 			if strings.HasPrefix(cs.Name, imageDigestExporterContainerName) {
diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go
@@ -1395,10 +1395,10 @@ func TestReconcile_SortTaskRunStatusSteps(t *testing.T) {
 	if err := testAssets.Controller.Reconciler.Reconcile(context.Background(), getRunName(taskRun)); err != nil {
 		t.Errorf("expected no error reconciling valid TaskRun but got %v", err)
 	}
-	verify_TaskRunStatusStep(t, taskRun, taskMultipleSteps)
+	verify_TaskRunStatusStep(t, taskRun)
 }
 
-func verify_TaskRunStatusStep(t *testing.T, taskRun *v1alpha1.TaskRun, task *v1alpha1.Task) {
+func verify_TaskRunStatusStep(t *testing.T, taskRun *v1alpha1.TaskRun) {
 	actualStepOrder := []string{}
 	for _, state := range taskRun.Status.Steps {
 		actualStepOrder = append(actualStepOrder, state.Name)
diff --git a/test/cluster_resource_test.go b/test/cluster_resource_test.go
@@ -57,7 +57,7 @@ func TestClusterResource(t *testing.T) {
 	}
 
 	t.Logf("Creating Task %s", taskName)
-	if _, err := c.TaskClient.Create(getClusterResourceTask(namespace, taskName, resourceName, configName)); err != nil {
+	if _, err := c.TaskClient.Create(getClusterResourceTask(namespace, taskName, configName)); err != nil {
 		t.Fatalf("Failed to create Task `%s`: %s", taskName, err)
 	}
 
@@ -97,7 +97,7 @@ func getClusterResourceTaskSecret(namespace, name string) *corev1.Secret {
 	}
 }
 
-func getClusterResourceTask(namespace, name, resName, configName string) *v1alpha1.Task {
+func getClusterResourceTask(namespace, name, configName string) *v1alpha1.Task {
 	return tb.Task(name, namespace, tb.TaskSpec(
 		tb.TaskInputs(tb.InputsResource("target-cluster", v1alpha1.PipelineResourceTypeCluster)),
 		tb.TaskVolume("config-vol", tb.VolumeSource(corev1.VolumeSource{
diff --git a/test/git_checkout_test.go b/test/git_checkout_test.go
@@ -55,7 +55,7 @@ func TestGitPipelineRun(t *testing.T) {
 		}
 
 		t.Logf("Creating Task %s", gitTestTaskName)
-		if _, err := c.TaskClient.Create(getGitCheckTask(namespace, t)); err != nil {
+		if _, err := c.TaskClient.Create(getGitCheckTask(namespace)); err != nil {
 			t.Fatalf("Failed to create Task `%s`: %s", gitTestTaskName, err)
 		}
 
@@ -91,7 +91,7 @@ func TestGitPipelineRunFail(t *testing.T) {
 	}
 
 	t.Logf("Creating Task %s", gitTestTaskName)
-	if _, err := c.TaskClient.Create(getGitCheckTask(namespace, t)); err != nil {
+	if _, err := c.TaskClient.Create(getGitCheckTask(namespace)); err != nil {
 		t.Fatalf("Failed to create Task `%s`: %s", gitTestTaskName, err)
 	}
 
@@ -152,7 +152,7 @@ func getGitPipelineResource(namespace, revision string) *v1alpha1.PipelineResour
 	))
 }
 
-func getGitCheckTask(namespace string, t *testing.T) *v1alpha1.Task {
+func getGitCheckTask(namespace string) *v1alpha1.Task {
 	return tb.Task(gitTestTaskName, namespace, tb.TaskSpec(
 		tb.TaskInputs(tb.InputsResource("gitsource", v1alpha1.PipelineResourceTypeGit)),
 		tb.Step("git", "alpine/git", tb.StepArgs("--git-dir=/workspace/gitsource/.git", "show")),
diff --git a/test/init_test.go b/test/init_test.go
@@ -145,7 +145,7 @@ func TestMain(m *testing.M) {
 
 func getCRDYaml(cs *clients, ns string) ([]byte, error) {
 	var output []byte
-	printOrAdd := func(kind, name string, i interface{}) {
+	printOrAdd := func(i interface{}) {
 		bs, err := yaml.Marshal(i)
 		if err != nil {
 			return
@@ -159,46 +159,46 @@ func getCRDYaml(cs *clients, ns string) ([]byte, error) {
 		return nil, xerrors.Errorf("could not get pipeline: %w", err)
 	}
 	for _, i := range ps.Items {
-		printOrAdd("Pipeline", i.Name, i)
+		printOrAdd(i)
 	}
 
 	prs, err := cs.PipelineResourceClient.List(metav1.ListOptions{})
 	if err != nil {
 		return nil, xerrors.Errorf("could not get pipelinerun resource: %w", err)
 	}
 	for _, i := range prs.Items {
-		printOrAdd("PipelineResource", i.Name, i)
+		printOrAdd(i)
 	}
 
 	prrs, err := cs.PipelineRunClient.List(metav1.ListOptions{})
 	if err != nil {
 		return nil, xerrors.Errorf("could not get pipelinerun: %w", err)
 	}
 	for _, i := range prrs.Items {
-		printOrAdd("PipelineRun", i.Name, i)
+		printOrAdd(i)
 	}
 
 	ts, err := cs.TaskClient.List(metav1.ListOptions{})
 	if err != nil {
 		return nil, xerrors.Errorf("could not get tasks: %w", err)
 	}
 	for _, i := range ts.Items {
-		printOrAdd("Task", i.Name, i)
+		printOrAdd(i)
 	}
 	trs, err := cs.TaskRunClient.List(metav1.ListOptions{})
 	if err != nil {
 		return nil, xerrors.Errorf("could not get taskrun: %w", err)
 	}
 	for _, i := range trs.Items {
-		printOrAdd("TaskRun", i.Name, i)
+		printOrAdd(i)
 	}
 
 	pods, err := cs.KubeClient.Kube.CoreV1().Pods(ns).List(metav1.ListOptions{})
 	if err != nil {
 		return nil, xerrors.Errorf("could not get pods: %w", err)
 	}
 	for _, i := range pods.Items {
-		printOrAdd("Pod", i.Name, i)
+		printOrAdd(i)
 	}
 
 	return output, nil

Original file line number	Diff line number	Diff line change
`@@ -59,7 +59,7 @@ func NewStorageResource(images pipeline.Images, r *PipelineResource) (PipelineSt`
`59`	`59`	`return nil, xerrors.Errorf("StoreResource: Cannot create a storage resource without type %s in spec", r.Name)`
`60`	`60`	`}`
`61`	`61`
`62`		`-func getStorageVolumeSpec(s PipelineStorageResourceInterface, spec TaskSpec) ([]corev1.Volume, error) {`
	`62`	`+func getStorageVolumeSpec(s PipelineStorageResourceInterface, spec TaskSpec) []corev1.Volume {`
`63`	`63`	`var storageVol []corev1.Volume`
`64`	`64`	`mountedSecrets := map[string]string{}`
`65`	`65`
`@@ -85,5 +85,5 @@ func getStorageVolumeSpec(s PipelineStorageResourceInterface, spec TaskSpec) ([]`
`85`	`85`	`mountedSecrets[volName] = ""`
`86`	`86`	`}`
`87`	`87`	`}`
`88`		`- return storageVol, nil`
	`88`	`+ return storageVol`
`89`	`89`	`}`
Original file line number	Diff line number	Diff line change
`@@ -26,13 +26,13 @@ import (`
`26`	`26`	`"golang.org/x/xerrors"`
`27`	`27`	`)`
`28`	`28`
`29`		`-func run(logger *zap.SugaredLogger, cmd string, args ...string) error {`
`30`		`- c := exec.Command(cmd, args...)`
	`29`	`+func run(logger *zap.SugaredLogger, args ...string) error {`
	`30`	`+ c := exec.Command("git", args...)`
`31`	`31`	`var output bytes.Buffer`
`32`	`32`	`c.Stderr = &output`
`33`	`33`	`c.Stdout = &output`
`34`	`34`	`if err := c.Run(); err != nil {`
`35`		`- logger.Errorf("Error running %v %v: %v\n%v", cmd, args, err, output.String())`
	`35`	`+ logger.Errorf("Error running git %v: %v\n%v", args, err, output.String())`
`36`	`36`	`return err`
`37`	`37`	`}`
`38`	`38`	`return nil`
`@@ -70,29 +70,29 @@ func Fetch(logger *zap.SugaredLogger, revision, path, url string) error {`
`70`	`70`	`revision = "master"`
`71`	`71`	`}`
`72`	`72`	`if path != "" {`
`73`		`- if err := run(logger, "git", "init", path); err != nil {`
	`73`	`+ if err := run(logger, "init", path); err != nil {`
`74`	`74`	`return err`
`75`	`75`	`}`
`76`	`76`	`if err := os.Chdir(path); err != nil {`
`77`	`77`	`return xerrors.Errorf("Failed to change directory with path %s; err: %w", path, err)`
`78`	`78`	`}`
`79`		`- } else if err := run(logger, "git", "init"); err != nil {`
	`79`	`+ } else if err := run(logger, "init"); err != nil {`
`80`	`80`	`return err`
`81`	`81`	`}`
`82`	`82`	`trimmedURL := strings.TrimSpace(url)`
`83`		`- if err := run(logger, "git", "remote", "add", "origin", trimmedURL); err != nil {`
	`83`	`+ if err := run(logger, "remote", "add", "origin", trimmedURL); err != nil {`
`84`	`84`	`return err`
`85`	`85`	`}`
`86`		`- if err := run(logger, "git", "fetch", "--depth=1", "--recurse-submodules=yes", "origin", revision); err != nil {`
	`86`	`+ if err := run(logger, "fetch", "--depth=1", "--recurse-submodules=yes", "origin", revision); err != nil {`
`87`	`87`	`// Fetch can fail if an old commitid was used so try git pull, performing regardless of error`
`88`	`88`	`// as no guarantee that the same error is returned by all git servers gitlab, github etc...`
`89`		`- if err := run(logger, "git", "pull", "--recurse-submodules=yes", "origin"); err != nil {`
	`89`	`+ if err := run(logger, "pull", "--recurse-submodules=yes", "origin"); err != nil {`
`90`	`90`	`logger.Warnf("Failed to pull origin : %s", err)`
`91`	`91`	`}`
`92`		`- if err := run(logger, "git", "checkout", revision); err != nil {`
	`92`	`+ if err := run(logger, "checkout", revision); err != nil {`
`93`	`93`	`return err`
`94`	`94`	`}`
`95`		`- } else if err := run(logger, "git", "reset", "--hard", "FETCH_HEAD"); err != nil {`
	`95`	`+ } else if err := run(logger, "reset", "--hard", "FETCH_HEAD"); err != nil {`
`96`	`96`	`return err`
`97`	`97`	`}`
`98`	`98`	`logger.Infof("Successfully cloned %s @ %s in path %s", trimmedURL, revision, path)`
Original file line number	Diff line number	Diff line change
`@@ -1395,10 +1395,10 @@ func TestReconcile_SortTaskRunStatusSteps(t *testing.T) {`
`1395`	`1395`	`if err := testAssets.Controller.Reconciler.Reconcile(context.Background(), getRunName(taskRun)); err != nil {`
`1396`	`1396`	`t.Errorf("expected no error reconciling valid TaskRun but got %v", err)`
`1397`	`1397`	`}`
`1398`		`- verify_TaskRunStatusStep(t, taskRun, taskMultipleSteps)`
	`1398`	`+ verify_TaskRunStatusStep(t, taskRun)`
`1399`	`1399`	`}`
`1400`	`1400`
`1401`		`-func verify_TaskRunStatusStep(t testing.T, taskRun v1alpha1.TaskRun, task *v1alpha1.Task) {`
	`1401`	`+func verify_TaskRunStatusStep(t testing.T, taskRun v1alpha1.TaskRun) {`
`1402`	`1402`	`actualStepOrder := []string{}`
`1403`	`1403`	`for _, state := range taskRun.Status.Steps {`
`1404`	`1404`	`actualStepOrder = append(actualStepOrder, state.Name)`