From c91e76f1695deec251356f8f31c369e58dec486a Mon Sep 17 00:00:00 2001 From: Cory Miller <13227161+cory-miller@users.noreply.github.com> Date: Tue, 20 Sep 2022 20:08:22 -0400 Subject: [PATCH] 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 | 23 ++++++++++++++++++ .golangci.yaml | 17 +++++++++++++ CONTRIBUTING.md | 4 ++++ Makefile | 3 +++ api/v1alpha1/runner_types.go | 6 +---- ...orizontal_runner_autoscaler_batch_scale.go | 1 - .../horizontal_runner_autoscaler_webhook.go | 10 ++++---- ...rizontal_runner_autoscaler_webhook_test.go | 9 ++++--- controllers/metrics/runnerset.go | 6 ----- controllers/multi_githubclient.go | 24 ------------------- controllers/runner_controller.go | 14 ++--------- controllers/runner_pod_owner.go | 2 +- github/fake/fake.go | 4 ++-- simulator/runnergroup_visibility.go | 15 ------------ test/e2e/cmd/main.go | 5 ---- test/e2e/e2e_test.go | 5 +--- testing/kubectl.go | 2 +- 17 files changed, 63 insertions(+), 87 deletions(-) create mode 100644 .github/workflows/golangci-lint.yaml create mode 100644 .golangci.yaml diff --git a/.github/workflows/golangci-lint.yaml b/.github/workflows/golangci-lint.yaml new file mode 100644 index 00000000..26d1bdb1 --- /dev/null +++ b/.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 diff --git a/.golangci.yaml b/.golangci.yaml new file mode 100644 index 00000000..a3957802 --- /dev/null +++ b/.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" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c19d350c..49efbab5 100644 --- a/CONTRIBUTING.md +++ b/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. \ No newline at end of file diff --git a/Makefile b/Makefile index 1db89f7c..ea0e64b1 100644 --- a/Makefile +++ b/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 diff --git a/api/v1alpha1/runner_types.go b/api/v1alpha1/runner_types.go index 38b4d8e6..bda324b8 100644 --- a/api/v1alpha1/runner_types.go +++ b/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 diff --git a/controllers/horizontal_runner_autoscaler_batch_scale.go b/controllers/horizontal_runner_autoscaler_batch_scale.go index d89d85cd..f6317ba5 100644 --- a/controllers/horizontal_runner_autoscaler_batch_scale.go +++ b/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{ diff --git a/controllers/horizontal_runner_autoscaler_webhook.go b/controllers/horizontal_runner_autoscaler_webhook.go index 02ad9855..c500860d 100644 --- a/controllers/horizontal_runner_autoscaler_webhook.go +++ b/controllers/horizontal_runner_autoscaler_webhook.go @@ -20,7 +20,7 @@ import ( "context" "encoding/json" "fmt" - "io/ioutil" + "io" "net/http" "strings" "sync" @@ -75,10 +75,8 @@ type HorizontalRunnerAutoscalerGitHubWebhook struct { // A scale target is enqueued on each retrieval of each eligible webhook event, so that it is processed asynchronously. QueueLimit int - worker *worker - workerInit sync.Once - workerStart sync.Once - batchCh chan *ScaleTarget + worker *worker + workerInit sync.Once } 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") diff --git a/controllers/horizontal_runner_autoscaler_webhook_test.go b/controllers/horizontal_runner_autoscaler_webhook_test.go index 4db5db79..d1a336e6 100644 --- a/controllers/horizontal_runner_autoscaler_webhook_test.go +++ b/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 } diff --git a/controllers/metrics/runnerset.go b/controllers/metrics/runnerset.go index ece2b2db..39384559 100644 --- a/controllers/metrics/runnerset.go +++ b/controllers/metrics/runnerset.go @@ -10,12 +10,6 @@ const ( rsNamespace = "namespace" ) -var ( - runnerSetMetrics = []prometheus.Collector{ - runnerSetReplicas, - } -) - var ( runnerSetReplicas = prometheus.NewGaugeVec( prometheus.GaugeOpts{ diff --git a/controllers/multi_githubclient.go b/controllers/multi_githubclient.go index cd873aff..3bcdccdd 100644 --- a/controllers/multi_githubclient.go +++ b/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, diff --git a/controllers/runner_controller.go b/controllers/runner_controller.go index 4a2f798d..51052c1c 100644 --- a/controllers/runner_controller.go +++ b/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}) -} diff --git a/controllers/runner_pod_owner.go b/controllers/runner_pod_owner.go index abc5020a..ebafe22b 100644 --- a/controllers/runner_pod_owner.go +++ b/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) } diff --git a/github/fake/fake.go b/github/fake/fake.go index 1416f597..3606792c 100644 --- a/github/fake/fake.go +++ b/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) } } diff --git a/simulator/runnergroup_visibility.go b/simulator/runnergroup_visibility.go index ee62a821..5a655f6a 100644 --- a/simulator/runnergroup_visibility.go +++ b/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 -} diff --git a/test/e2e/cmd/main.go b/test/e2e/cmd/main.go index 2c8bb9f7..4a487aa7 100644 --- a/test/e2e/cmd/main.go +++ b/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() -} diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 4e21edb9..1160947c 100644 --- a/test/e2e/e2e_test.go +++ b/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") } diff --git a/testing/kubectl.go b/testing/kubectl.go index fbbc75c6..97c90fab 100644 --- a/testing/kubectl.go +++ b/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...)