Fix all lint warnings

[ReToCode]

trafficstars

Problem description

We have several lint warnings all over Serving. These introduce lint errors in PRs, even when the change is not related to those. Example here. We probably should update all those occurrence's separately:

• [x] SA1019: sets.String is deprecated
• [ ] SA1019: sets.Int is deprecated
• [ ] SA1019: wait.PollImmediate is deprecated
• [ ] dot-imports: should not use dot imports, see https://github.com/knative/serving/issues/14692#issuecomment-2148371058. If possible, it should be allowed for test in the linter config.
• [ ] unused-parameter --> see comment
• [ ] redefines-builtin-id
• [ ] SA1019: rand.Seed has been deprecated
• [ ] indent-error-flow
• [ ] superfluous-else
• [ ] G402: TLS MinVersion too low
• [ ] empty-block

To get a full list run:

golangci-lint version      
golangci-lint has version 1.55.2 built with go1.21.3 from e3c2265 on 2023-11-02T21:40:02Z
                                                                                                                                                                                                          
golangci-lint run --go=1.21

Make sure to have an up to date version of the linter.

[navinag1989]

/assign

[skonto]

I would start with the issues in knative/pkg and other deps. I started a PR some time ago but first you need to fix also the ones that come from the dependencies if not mistaken.

[navinag1989]

I believe we should consider excluding 'test.go' files from linting, or at the very least, exempting specific rules for test files, such as dot-imports and unused parameters. The number of occurrences is too high, making it challenging to address all of them. cc: @skonto @ReToCode

[ReToCode]

I believe we should consider excluding 'test.go' files from linting, or at the very least, exempting specific rules for test files, such as dot-imports and unused parameters. The number of occurrences is too high, making it challenging to address all of them.

Hm I don't think I'd agree :) Tests should also be written using best practices. Let's do as Stavros proposed and go with fixing one of them at a time.

[akagami-harsh]

hey @ReToCode, i would also like to work on this issue, if @navinag1989 dosen't mind

[navinag1989]

Sure @akagami-harsh Start with knative/pkg repo and other deps first. I have raised PR in knative/pkg - https://github.com/knative/pkg/pull/2915

[ReToCode]

The additional tasks are up to be grabbed.

[skonto]

@navinag1989 @akagami-harsh feel free to pick up any of the remaining tasks.

[dprotaso]

For unused-parameter I say we disable it in in our config here - https://github.com/knative/serving/blob/main/.golangci.yaml

I think the PRs for that will be noisy and wont really improve anything

eg. then we're just renaming our functions vars

func SomeFunc(someObj *MyStruct) {} 

becomes

func SomeFunc(_ *MyStruct) {} 

[cjhetzle]

Sure I can chip into the issue

[prashantrewar]

Hey @skonto and @ReToCod, SA1019: wait.PollImmediate is deprecated task is still up for grabs?

[dprotaso]

@prashantrewar go for it

[prashantrewar]

Hey @skonto and @ReToCod, dot-imports: should not use dot imports task is still up for grabs?

[asr2003]

@dprotaso @ReToCode @skonto What lint warnings are still to be fixed ?

[dprotaso]

@asr2003 - @ReToCode classified a bunch already

We use this for our linting tool - it calls out to other linters https://github.com/golangci/golangci-lint

@prashantrewar we use dot-imports in tests. Not sure if we can configure the linter (https://github.com/knative/serving/blob/main/.golangci.yaml) to only enforce that run in non-tests

[ReToCode]

@asr2003 as Dave pointed out, you can just run the linter and see the exact occurrences of the lint warnings.