Enable "gocritic" in CI, and fix associated errors.

"gocritic" is "the most opinionated go linter". I was expecting to be terrified
by the number of errors it would report when run, but it was surprisngly reasonable.

.golangci.yml

@@ -13,3 +13,4 @@ linters:
   - gofmt
   - goimports
   - gosec
+  - gocritic

cmd/entrypoint/main.go

@@ -50,7 +50,7 @@ func main() {
 		PostWriter:      &RealPostWriter{},
 	}
 	if err := e.Go(); err != nil {
-		switch err.(type) {
+		switch t := err.(type) {
 		case skipError:
 			os.Exit(0)
 		case *exec.ExitError:
@@ -60,7 +60,7 @@ func main() {
 			// WaitStatus is defined for both Unix and Windows and
 			// in both cases has an ExitStatus() method with the
 			// same signature.
-			if status, ok := err.(*exec.ExitError).Sys().(syscall.WaitStatus); ok {
+			if status, ok := t.Sys().(syscall.WaitStatus); ok {
 				os.Exit(status.ExitStatus())
 			}
 			log.Fatalf("Error executing command (ExitError): %v", err)

examples/taskruns/gitlab.yaml

@@ -0,0 +1,58 @@
+apiVersion: tekton.dev/v1alpha1
+kind: TaskRun
+metadata:
+  name: gitlab-pr
+  namespace: default
+spec:
+  inputs:
+    resources:
+    - name: pr
+      resourceSpec:
+        params:
+        - name: url
+          value: https://gitlab.com/dlorenc1/testtekton/merge_requests/1
+        secrets:
+        - fieldName: authToken
+          secretKey: gitlab-token
+          secretName: gitlabtoken
+        type: pullRequest
+  outputs:
+    resources:
+    - name: pr
+      resourceSpec:
+        params:
+        - name: url
+          value: https://gitlab.com/dlorenc1/testtekton/merge_requests/1
+        secrets:
+        - fieldName: authToken
+          secretKey: gitlab-token
+          secretName: gitlabtoken
+        type: pullRequest
+  taskSpec:
+    inputs:
+      resources:
+      - name: pr
+        outputImageDir: ""
+        targetPath: ""
+        type: pullRequest
+    outputs:
+      resources:
+      - name: pr
+        type: pullRequest
+    steps:
+    - name: find
+      image: python
+      command:
+      - sh
+      args:
+      - -c
+      - |
+        find /workspace -type f | xargs cat
+    - name: comment
+      image: python
+      command:
+      - sh
+      args:
+      - -c
+      - |
+        echo "looks great to me" > /workspace/pr/comments/new && touch /workspace/pr/labels/lgtm

pkg/apis/pipeline/v1alpha1/cloud_event_resource.go

@@ -40,8 +40,7 @@ func NewCloudEventResource(r *PipelineResource) (*CloudEventResource, error) {
 	var targetURISpecified bool
 
 	for _, param := range r.Spec.Params {
-		switch {
-		case strings.EqualFold(param.Name, "TargetURI"):
+		if strings.EqualFold(param.Name, "TargetURI") {
 			targetURI = param.Value
 			if param.Value != "" {
 				targetURISpecified = true

pkg/apis/pipeline/v1alpha1/pull_request_resource.go

@@ -60,8 +60,7 @@ func NewPullRequestResource(r *PipelineResource) (*PullRequestResource, error) {
 		Secrets: r.Spec.SecretParams,
 	}
 	for _, param := range r.Spec.Params {
-		switch {
-		case strings.EqualFold(param.Name, "URL"):
+		if strings.EqualFold(param.Name, "URL") {
 			prResource.URL = param.Value
 		}
 	}
@@ -112,8 +111,7 @@ func (s *PullRequestResource) getSteps(mode string, sourcePath string) []Step {
 
 	evs := []corev1.EnvVar{}
 	for _, sec := range s.Secrets {
-		switch {
-		case strings.EqualFold(sec.FieldName, githubTokenEnv):
+		if strings.EqualFold(sec.FieldName, githubTokenEnv) {
 			ev := corev1.EnvVar{
 				Name: strings.ToUpper(sec.FieldName),
 				ValueFrom: &corev1.EnvVarSource{

pkg/git/git.go

@@ -76,10 +76,8 @@ func Fetch(logger *zap.SugaredLogger, revision, path, url string) error {
 		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 {
-			return err
-		}
+	} else if err := run(logger, "git", "init"); err != nil {
+		return err
 	}
 	trimmedURL := strings.TrimSpace(url)
 	if err := run(logger, "git", "remote", "add", "origin", trimmedURL); err != nil {
@@ -94,10 +92,8 @@ func Fetch(logger *zap.SugaredLogger, revision, path, url string) error {
 		if err := run(logger, "git", "checkout", revision); err != nil {
 			return err
 		}
-	} else {
-		if err := run(logger, "git", "reset", "--hard", "FETCH_HEAD"); err != nil {
-			return err
-		}
+	} else if err := run(logger, "git", "reset", "--hard", "FETCH_HEAD"); err != nil {
+		return err
 	}
 	logger.Infof("Successfully cloned %s @ %s in path %s", trimmedURL, revision, path)
 	return nil

pkg/reconciler/taskrun/resources/cloudevent/cloud_event_controller.go

@@ -77,7 +77,7 @@ func SendCloudEvents(tr *v1alpha1.TaskRun, ceclient CEClient, logger *zap.Sugare
 		}
 		_, err := SendTaskRunCloudEvent(cloudEventDelivery.Target, tr, logger, ceclient)
 		eventStatus.SentAt = &metav1.Time{Time: time.Now()}
-		eventStatus.RetryCount = eventStatus.RetryCount + 1
+		eventStatus.RetryCount++
 		if err != nil {
 			merr = multierror.Append(merr, err)
 			eventStatus.Condition = v1alpha1.CloudEventConditionFailed

pkg/reconciler/taskrun/resources/cloudevent/cloudevent.go

@@ -114,13 +114,14 @@ func SendTaskRunCloudEvent(sinkURI string, taskRun *v1alpha1.TaskRun, logger *za
 	eventID := taskRun.ObjectMeta.Name
 	taskRunStatus := taskRun.Status.GetCondition(apis.ConditionSucceeded)
 	var eventType TektonEventType
-	if taskRunStatus.IsUnknown() {
+	switch {
+	case taskRunStatus.IsUnknown():
 		eventType = TektonTaskRunUnknownV1
-	} else if taskRunStatus.IsFalse() {
+	case taskRunStatus.IsFalse():
 		eventType = TektonTaskRunFailedV1
-	} else if taskRunStatus.IsTrue() {
+	case taskRunStatus.IsTrue():
 		eventType = TektonTaskRunSuccessfulV1
-	} else {
+	default:
 		return event, fmt.Errorf("Unknown condition for in TaskRun.Status %s", taskRunStatus.Status)
 	}
 	eventSourceURI := taskRun.ObjectMeta.SelfLink

pkg/reconciler/taskrun/resources/input_resource_test.go

@@ -1021,18 +1021,19 @@ func mockResolveTaskResources(taskRun *v1alpha1.TaskRun) map[string]v1alpha1.Pip
 	resolved := make(map[string]v1alpha1.PipelineResourceInterface)
 	for _, r := range taskRun.Spec.Inputs.Resources {
 		var i v1alpha1.PipelineResourceInterface
-		if name := r.ResourceRef.Name; name != "" {
-			i = inputResourceInterfaces[name]
+		switch {
+		case r.ResourceRef.Name != "":
+			i = inputResourceInterfaces[r.ResourceRef.Name]
 			resolved[r.Name] = i
-		} else if r.ResourceSpec != nil {
+		case r.ResourceSpec != nil:
 			i, _ = v1alpha1.ResourceFromType(&v1alpha1.PipelineResource{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: r.Name,
 				},
 				Spec: *r.ResourceSpec,
 			})
 			resolved[r.Name] = i
-		} else {
+		default:
 			resolved[r.Name] = nil
 		}
 	}

pkg/reconciler/taskrun/resources/taskresourceresolution_test.go

@@ -108,28 +108,21 @@ func TestResolveTaskRun(t *testing.T) {
 		r, ok := rtr.Inputs["repoToBuildFrom"]
 		if !ok {
 			t.Errorf("Expected value present in map for `repoToBuildFrom' but it was missing")
-		} else {
-			if r.Name != "git-repo" {
-				t.Errorf("Expected to use resource `git-repo` for `repoToBuildFrom` but used %s", r.Name)
-			}
+		} else if r.Name != "git-repo" {
+			t.Errorf("Expected to use resource `git-repo` for `repoToBuildFrom` but used %s", r.Name)
 		}
 		r, ok = rtr.Inputs["clusterToUse"]
 		if !ok {
 			t.Errorf("Expected value present in map for `clusterToUse' but it was missing")
-		} else {
-			if r.Name != "k8s-cluster" {
-				t.Errorf("Expected to use resource `k8s-cluster` for `clusterToUse` but used %s", r.Name)
-			}
+		} else if r.Name != "k8s-cluster" {
+			t.Errorf("Expected to use resource `k8s-cluster` for `clusterToUse` but used %s", r.Name)
 		}
 		r, ok = rtr.Inputs["clusterspecToUse"]
 		if !ok {
 			t.Errorf("Expected value present in map for `clusterspecToUse' but it was missing")
-		} else {
-			if r.Spec.Type != v1alpha1.PipelineResourceTypeCluster {
-				t.Errorf("Expected to use resource to be of type `cluster` for `clusterspecToUse` but got %s", r.Spec.Type)
-			}
+		} else if r.Spec.Type != v1alpha1.PipelineResourceTypeCluster {
+			t.Errorf("Expected to use resource to be of type `cluster` for `clusterspecToUse` but got %s", r.Spec.Type)
 		}
-
 	} else {
 		t.Errorf("Expected 2 resolved inputs but instead had: %v", rtr.Inputs)
 	}
@@ -138,26 +131,20 @@ func TestResolveTaskRun(t *testing.T) {
 		r, ok := rtr.Outputs["imageToBuild"]
 		if !ok {
 			t.Errorf("Expected value present in map for `imageToBuild' but it was missing")
-		} else {
-			if r.Name != "image" {
-				t.Errorf("Expected to use resource `image` for `imageToBuild` but used %s", r.Name)
-			}
+		} else if r.Name != "image" {
+			t.Errorf("Expected to use resource `image` for `imageToBuild` but used %s", r.Name)
 		}
 		r, ok = rtr.Outputs["gitRepoToUpdate"]
 		if !ok {
 			t.Errorf("Expected value present in map for `gitRepoToUpdate' but it was missing")
-		} else {
-			if r.Name != "another-git-repo" {
-				t.Errorf("Expected to use resource `another-git-repo` for `gitRepoToUpdate` but used %s", r.Name)
-			}
+		} else if r.Name != "another-git-repo" {
+			t.Errorf("Expected to use resource `another-git-repo` for `gitRepoToUpdate` but used %s", r.Name)
 		}
 		r, ok = rtr.Outputs["gitspecToUse"]
 		if !ok {
 			t.Errorf("Expected value present in map for `gitspecToUse' but it was missing")
-		} else {
-			if r.Spec.Type != v1alpha1.PipelineResourceTypeGit {
-				t.Errorf("Expected to use resource type `git` for but got %s", r.Spec.Type)
-			}
+		} else if r.Spec.Type != v1alpha1.PipelineResourceTypeGit {
+			t.Errorf("Expected to use resource type `git` for but got %s", r.Spec.Type)
 		}
 	} else {
 		t.Errorf("Expected 2 resolved outputs but instead had: %v", rtr.Outputs)