Run golangci-lint on project, fix and implement gh actions

[rikatz]

What happened:
Our linting today doesn't catch some basic scenarios, like unused vars, lack of error validation, etc.

We should migrate to using golangci-lint, but first we need to check the errors:

• Run https://golangci-lint.run/ and fix the errors (please send one PR to fix them all, don't break in multiple PRs!)
• Add ghactions https://golangci-lint.run/usage/install/ on a different PR

Who is willing to work on this, please ping me on Slack before assigning yourself to it, on #ingress-nginx-dev channel

/good-first-issue
/help

[k8s-ci-robot]

@rikatz:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

What happened:
Our linting today doesn't catch some basic scenarios, like unused vars, lack of error validation, etc.

We should migrate to using golangci-lint, but first we need to check the errors:

• Run https://golangci-lint.run/ and fix the errors (please send one PR to fix them all, don't break in multiple PRs!)
• Add ghactions https://golangci-lint.run/usage/install/ on a different PR

Who is willing to work on this, please ping me on Slack before assigning yourself to it, on #ingress-nginx-dev channel

/good-first-issue
/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[rikatz]

/triage accepted

[longwuyuan]

We should close this #9037

…

On Sun, 18 Jun, 2023, 7:26 pm Ricardo Katz, ***@***.***> wrote: /triage accepted — Reply to this email directly, view it on GitHub <#10097 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWW7JO4RSSYOJFE7MM3XL4CHHANCNFSM6AAAAAAZK5C3TU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[z1cheng]

@rikatz Already ping you on slack. I tried to run golangci-lint run -D errcheck on my machine:

internal/ingress/controller/config/config.go:94:19: SA5008: unknown JSON option "squash" (staticcheck)
        defaults.Backend `json:",squash"`
                         ^
cmd/dataplane/main.go:44:2: SA1019: rand.Seed has been deprecated since Go 1.20 and an alternative has been available since Go 1.0: Programs that call Seed and then expect a specific sequence of results from the global random source (using functions such as Int) can be broken when a dependency changes how much it consumes from the global random source. To avoid such breakages, programs that need a specific result sequence should use NewRand(NewSource(seed)) to obtain a random generator that other packages cannot access. (staticcheck)
        rand.Seed(time.Now().UnixNano())
        ^
test/e2e/framework/deployment.go:59:2: field `port` is unused (unused)
        port           int32
        ^
test/e2e/framework/deployment.go:61:2: field `command` is unused (unused)
        command        []string
        ^
test/e2e/framework/deployment.go:62:2: field `args` is unused (unused)
        args           []string
        ^
test/e2e/framework/deployment.go:63:2: field `env` is unused (unused)
        env            []corev1.EnvVar
        ^
test/e2e/framework/deployment.go:64:2: field `volumeMounts` is unused (unused)
        volumeMounts   []corev1.VolumeMount
        ^
test/e2e/framework/deployment.go:65:2: field `volumes` is unused (unused)
        volumes        []corev1.Volume
        ^
test/e2e/framework/deployment.go:67:2: field `setProbe` is unused (unused)
        setProbe       bool
        ^
test/e2e/framework/httpexpect/chain.go:40:17: func `(*chain).reset` is unused (unused)
func (c *chain) reset() {
                ^
test/e2e/framework/httpexpect/chain.go:44:17: func `(*chain).assertFailed` is unused (unused)
func (c *chain) assertFailed(r Reporter) {
                ^
test/e2e/framework/httpexpect/chain.go:50:17: func `(*chain).assertOK` is unused (unused)
func (c *chain) assertOK(r Reporter) {
                ^
test/e2e/status/update.go:74:3: ineffectual assignment to ing (ineffassign)
                ing := f.EnsureIngress(framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, nil))
                ^
internal/ingress/controller/template/template.go:1191:2: SA1019: rand.Seed has been deprecated since Go 1.20 and an alternative has been available since Go 1.0: Programs that call Seed and then expect a specific sequence of results from the global random source (using functions such as Int) can be broken when a dependency changes how much it consumes from the global random source. To avoid such breakages, programs that need a specific result sequence should use NewRand(NewSource(seed)) to obtain a random generator that other packages cannot access. (staticcheck)
        rand.Seed(time.Now().UnixNano())
        ^
internal/ingress/controller/nginx.go:252:2: field `admissionCollector` is unused (unused)
        admissionCollector metric.Collector
        ^
internal/ingress/controller/nginx.go:804:6: func `clearCertificates` is unused (unused)
func clearCertificates(config *ingress.Configuration) {
     ^
internal/ingress/controller/nginx.go:816:6: func `clearL4serviceEndpoints` is unused (unused)
func clearL4serviceEndpoints(config *ingress.Configuration) {
     ^
internal/net/ssl/ssl.go:231:12: SA1019: x509.ParseCRL has been deprecated since Go 1.19: Use ParseRevocationList instead. (staticcheck)
        _, err := x509.ParseCRL(pemCRLBlock.Bytes)
                  ^
test/e2e/annotations/affinitymode.go:128:3: SA4006: this value of `response` is never used (staticcheck)
                response = request.WithCookies(cookies).Expect().Status(http.StatusServiceUnavailable)
                ^
internal/admission/controller/main_test.go:70:2: ineffectual assignment to result (ineffassign)
        result, err := adm.HandleAdmission(&admissionv1.AdmissionReview{
        ^
internal/admission/controller/main_test.go:79:2: ineffectual assignment to result (ineffassign)
        result, err = adm.HandleAdmission(nil)
        ^
cmd/nginx/main.go:57:2: SA1019: rand.Seed has been deprecated since Go 1.20 and an alternative has been available since Go 1.0: Programs that call Seed and then expect a specific sequence of results from the global random source (using functions such as Int) can be broken when a dependency changes how much it consumes from the global random source. To avoid such breakages, programs that need a specific result sequence should use NewRand(NewSource(seed)) to obtain a random generator that other packages cannot access. (staticcheck)
        rand.Seed(time.Now().UnixNano())
        ^

I skipped the errcheck, because as you said, some errcheck warnings are not valid, we can consider other warnings first

[tao12345666333]

if you want to ignore some warnings, you can add a .golangci.yaml

[z1cheng]

@tao12345666333 Yes, and we can refer to other .golangci.yaml, like https://github.com/kubernetes/kompose/blob/main/.golangci.yml (Also ignore errcheck linter)

First I think we can confirm which linters we want to enable.

[rikatz]

/assign @z1cheng
Thanks for stepping forward on this :)

[rikatz]

On the errcheck, please ignore it always locally and never on the plugin. We may not want to have people skipping error checks on valid code.

So instead you can do a //nolint comment on the line we want to skip

[sonbui00]

I saw some bots can auto fix this. Ex: https://github.com/wearerequired/lint-action

[z1cheng]

@sonbui00 Thanks for paying attention to this issue, the example bot only support golint tool (has been deprecated). And the bot does not support auto-fix (https://github.com/wearerequired/lint-action#linter-support). Only a few linters in golangci-lint support AutoFix, see the linter list: https://golangci-lint.run/usage/linters/