Add golangci-lilnt to CI (#1794)

This introduces a linter to PRs to help with code reviews and code hygiene. I've also gone ahead and fixed (or ignored) the existing lints.

I've only setup the default linters right now. There are many more options that are documented at https://golangci-lint.run/.

The GitHub Action should add appropriate annotations to the lint job for the PR. Contributors can also lint locally using `make lint`.

.github/workflows/golangci-lint.yaml

@@ -0,0 +1,23 @@
+name: golangci-lint
+on:
+  push:
+    branches:
+      - master
+  pull_request:
+permissions:
+  contents: read
+  pull-requests: read
+jobs:
+  golangci:
+    name: lint
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/setup-go@v3
+        with:
+          go-version: 1.19
+      - uses: actions/checkout@v3
+      - name: golangci-lint
+        uses: golangci/golangci-lint-action@v3
+        with:
+          only-new-issues: true
+          version: v1.49.0

.golangci.yaml

@@ -0,0 +1,17 @@
+run:
+  timeout: 3m
+output:
+  format: github-actions
+linters-settings:
+  errcheck:
+    exclude-functions:
+      - (net/http.ResponseWriter).Write
+      - (*net/http.Server).Shutdown
+      - (*github.com/actions-runner-controller/actions-runner-controller/simulator.VisibleRunnerGroups).Add
+      - (*github.com/actions-runner-controller/actions-runner-controller/testing.Kind).Stop
+issues:
+  exclude-rules:
+    - path: controllers/suite_test.go
+      linters:
+        - staticcheck
+      text: "SA1019"

CONTRIBUTING.md

@@ -155,3 +155,7 @@ GINKGO_FOCUS='[It] should create a new Runner resource from the specified templa
 #### Helm Version Bumps
 
 In general we ask you not to bump the version in your PR, the maintainers in general manage the publishing of a new chart.
+
+#### Running linters locally
+
+To run the `golangci-lint` tool locally, we recommend you use `make lint` to run the tool using a Docker container matching the CI version.

Makefile

@@ -51,6 +51,9 @@ endif
 
 all: manager
 
+lint:
+	docker run --rm -v $(PWD):/app -w /app golangci/golangci-lint:v1.49.0 golangci-lint run
+
 GO_TEST_ARGS ?= -short
 
 # Run tests

api/v1alpha1/runner_types.go

@@ -334,11 +334,7 @@ func (r Runner) IsRegisterable() bool {
 	}
 
 	now := metav1.Now()
-	if r.Status.Registration.ExpiresAt.Before(&now) {
-		return false
-	}
-
-	return true
+	return !r.Status.Registration.ExpiresAt.Before(&now)
 }
 
 // +kubebuilder:object:root=true

controllers/horizontal_runner_autoscaler_batch_scale.go

@@ -79,7 +79,6 @@ func (s *batchScaler) Add(st *ScaleTarget) {
 				for {
 					select {
 					case <-after:
-						after = nil
 						break batch
 					case st := <-s.queue:
 						nsName := types.NamespacedName{

controllers/horizontal_runner_autoscaler_webhook.go

@@ -20,7 +20,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"io/ioutil"
+	"io"
 	"net/http"
 	"strings"
 	"sync"
@@ -77,8 +77,6 @@ type HorizontalRunnerAutoscalerGitHubWebhook struct {
 
 	worker     *worker
 	workerInit sync.Once
-	workerStart sync.Once
-	batchCh     chan *ScaleTarget
 }
 
 func (autoscaler *HorizontalRunnerAutoscalerGitHubWebhook) Reconcile(_ context.Context, request reconcile.Request) (reconcile.Result, error) {
@@ -133,7 +131,7 @@ func (autoscaler *HorizontalRunnerAutoscalerGitHubWebhook) Handle(w http.Respons
 			return
 		}
 	} else {
-		payload, err = ioutil.ReadAll(r.Body)
+		payload, err = io.ReadAll(r.Body)
 		if err != nil {
 			autoscaler.Log.Error(err, "error reading request body")
 

controllers/horizontal_runner_autoscaler_webhook_test.go

@@ -5,7 +5,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -504,7 +503,7 @@ func testServerWithInitObjs(t *testing.T, eventType string, event interface{}, w
 
 	hraWebhook := &HorizontalRunnerAutoscalerGitHubWebhook{}
 
-	client := fake.NewFakeClientWithScheme(sc, initObjs...)
+	client := fake.NewClientBuilder().WithScheme(sc).WithRuntimeObjects(initObjs...).Build()
 
 	logs := installTestLogger(hraWebhook)
 
@@ -537,7 +536,7 @@ func testServerWithInitObjs(t *testing.T, eventType string, event interface{}, w
 		t.Error("status:", resp.StatusCode)
 	}
 
-	respBody, err := ioutil.ReadAll(resp.Body)
+	respBody, err := io.ReadAll(resp.Body)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -575,7 +574,7 @@ func sendWebhook(server *httptest.Server, eventType string, event interface{}) (
 			"X-GitHub-Event": {eventType},
 			"Content-Type":   {"application/json"},
 		},
-		Body: ioutil.NopCloser(bytes.NewBuffer(reqBody)),
+		Body: io.NopCloser(bytes.NewBuffer(reqBody)),
 	}
 
 	return http.DefaultClient.Do(req)
@@ -607,7 +606,7 @@ func (l *testLogSink) Info(_ int, msg string, kvs ...interface{}) {
 	fmt.Fprintf(l.writer, "\n")
 }
 
-func (_ *testLogSink) Enabled(level int) bool {
+func (*testLogSink) Enabled(level int) bool {
 	return true
 }
 

controllers/metrics/runnerset.go

@@ -10,12 +10,6 @@ const (
 	rsNamespace = "namespace"
 )
 
-var (
-	runnerSetMetrics = []prometheus.Collector{
-		runnerSetReplicas,
-	}
-)
-
 var (
 	runnerSetReplicas = prometheus.NewGaugeVec(
 		prometheus.GaugeOpts{

controllers/multi_githubclient.go

@@ -151,14 +151,6 @@ func (c *MultiGitHubClient) DeinitForRunnerSet(rs *v1alpha1.RunnerSet) {
 	c.derefClient(rs.Namespace, secretName, refFromRunnerSet(rs))
 }
 
-func (c *MultiGitHubClient) deinitClientForRunnerReplicaSet(rs *v1alpha1.RunnerReplicaSet) {
-	c.derefClient(rs.Namespace, rs.Spec.Template.Spec.GitHubAPICredentialsFrom.SecretRef.Name, refFromRunnerReplicaSet(rs))
-}
-
-func (c *MultiGitHubClient) deinitClientForRunnerDeployment(rd *v1alpha1.RunnerDeployment) {
-	c.derefClient(rd.Namespace, rd.Spec.Template.Spec.GitHubAPICredentialsFrom.SecretRef.Name, refFromRunnerDeployment(rd))
-}
-
 func (c *MultiGitHubClient) DeinitForHRA(hra *v1alpha1.HorizontalRunnerAutoscaler) {
 	var secretName string
 	if hra.Spec.GitHubAPICredentialsFrom != nil {
@@ -310,22 +302,6 @@ func secretDataToGitHubClientConfig(data map[string][]byte) (*github.Config, err
 	return &conf, nil
 }
 
-func refFromRunnerDeployment(rd *v1alpha1.RunnerDeployment) *runnerOwnerRef {
-	return &runnerOwnerRef{
-		kind: rd.Kind,
-		ns:   rd.Namespace,
-		name: rd.Name,
-	}
-}
-
-func refFromRunnerReplicaSet(rs *v1alpha1.RunnerReplicaSet) *runnerOwnerRef {
-	return &runnerOwnerRef{
-		kind: rs.Kind,
-		ns:   rs.Namespace,
-		name: rs.Name,
-	}
-}
-
 func refFromRunner(r *v1alpha1.Runner) *runnerOwnerRef {
 	return &runnerOwnerRef{
 		kind: r.Kind,

controllers/runner_controller.go

@@ -715,7 +715,7 @@ func runnerHookEnvs(pod *corev1.Pod) ([]corev1.EnvVar, error) {
 				},
 			},
 		},
-		corev1.EnvVar{
+		{
 			Name:  "ACTIONS_RUNNER_REQUIRE_SAME_NODE",
 			Value: strconv.FormatBool(isRequireSameNode),
 		},
@@ -834,7 +834,7 @@ func newRunnerPodWithContainerMode(containerMode string, template corev1.Pod, ru
 		if dockerdContainer != nil {
 			template.Spec.Containers = append(template.Spec.Containers[:dockerdContainerIndex], template.Spec.Containers[dockerdContainerIndex+1:]...)
 		}
-		if runnerContainerIndex < runnerContainerIndex {
+		if dockerdContainerIndex < runnerContainerIndex {
 			runnerContainerIndex--
 		}
 		dockerdContainer = nil
@@ -1226,13 +1226,3 @@ func isRequireSameNode(pod *corev1.Pod) (bool, error) {
 	}
 	return false, nil
 }
-
-func overwriteRunnerEnv(runner *v1alpha1.Runner, key string, value string) {
-	for i := range runner.Spec.Env {
-		if runner.Spec.Env[i].Name == key {
-			runner.Spec.Env[i].Value = value
-			return
-		}
-	}
-	runner.Spec.Env = append(runner.Spec.Env, corev1.EnvVar{Name: key, Value: value})
-}

controllers/runner_pod_owner.go

@@ -315,7 +315,7 @@ func syncRunnerPodsOwners(ctx context.Context, c client.Client, log logr.Logger,
 	numOwners := len(owners)
 
 	var hashes []string
-	for h, _ := range state.podsForOwners {
+	for h := range state.podsForOwners {
 		hashes = append(hashes, h)
 	}
 

github/fake/fake.go

@@ -47,7 +47,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	status := req.URL.Query().Get("status")
 	if h.Statuses != nil {
 		if body, ok := h.Statuses[status]; ok {
-			fmt.Fprintf(w, body)
+			fmt.Fprint(w, body)
 			return
 		}
 	}
@@ -69,7 +69,7 @@ func (h *MapHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		w.WriteHeader(404)
 	} else {
 		w.WriteHeader(h.Status)
-		fmt.Fprintf(w, body)
+		fmt.Fprint(w, body)
 	}
 }
 

simulator/runnergroup_visibility.go

@@ -41,18 +41,3 @@ func (c *Simulator) GetRunnerGroupsVisibleToRepository(ctx context.Context, org,
 
 	return visible, nil
 }
-
-func (c *Simulator) hasRepoAccessToOrganizationRunnerGroup(ctx context.Context, org string, runnerGroupId int64, repo string) (bool, error) {
-	repos, err := c.Client.ListRunnerGroupRepositoryAccesses(ctx, org, runnerGroupId)
-	if err != nil {
-		return false, err
-	}
-
-	for _, githubRepo := range repos {
-		if githubRepo.GetFullName() == repo {
-			return true, nil
-		}
-	}
-
-	return false, nil
-}

test/e2e/cmd/main.go

@@ -56,8 +56,3 @@ func delete(cmName string) ([]byte, error) {
 	cmd := exec.Command("kubectl", "delete", "cm", cmName)
 	return cmd.CombinedOutput()
 }
-
-func deleteControllerManagerSecret() ([]byte, error) {
-	cmd := exec.Command("kubectl", "-n", "actions-runner-system", "delete", "secret", "controller-manager")
-	return cmd.CombinedOutput()
-}

test/e2e/e2e_test.go

@@ -616,9 +616,6 @@ func (e *env) checkGitHubToken(t *testing.T, tok string) error {
 	return nil
 }
 
-func (e *env) f() {
-}
-
 func (e *env) buildAndLoadImages(t *testing.T) {
 	t.Helper()
 
@@ -1070,7 +1067,7 @@ func verifyActionsWorkflowRun(t *testing.T, env *testing.Env, testJobs []job, ti
 
 	var expected []string
 
-	for _ = range testJobs {
+	for range testJobs {
 		expected = append(expected, "ok")
 	}
 

testing/kubectl.go

@@ -124,7 +124,7 @@ func (k *Kubectl) kubectlCmd(ctx context.Context, c string, args []string, cfg K
 	}
 
 	if cfg.Timeout > 0 {
-		args = append(args, "--timeout="+fmt.Sprintf("%s", cfg.Timeout))
+		args = append(args, fmt.Sprintf("--timeout=%v", cfg.Timeout.String()))
 	}
 
 	cmd := exec.CommandContext(ctx, "kubectl", args...)