fix: Clean up golangci-lint config

[mateusoliveira43]

Please add a summary of your change

Clean up of golangci-lint config file:

• removed unused configs
  • 18 linters-settings (dupl, exhaustive, funlen, gocognit, nestif, gocritic, gocyclo, godot, godox, golint, gomnd, gomodguard, depguard, lll, maligned, prealloc, rowserrcheck and testpackage) were declared that would do nothing, since they are not enabled in linters sections
• updated wrong configs
  • added config verify command to CI to avoid this in the future
• removed doc comments and add golangci-lint doc link
  • file was generated copying example from golangci-lint, which is not a configuration example per https://github.com/golangci/golangci-lint/blob/7f6606a7ea1ba30ca5f56fdc4f11e9d318e6cbec/.golangci.reference.yml#L4
  • this makes reading config file a lot easier. File will look like this after merge https://github.com/mateusoliveira43/velero/blob/fix/golangci-lint-config/.golangci.yaml
  • easier to catch wrong config as noted here Enabled linters with all options set to false #7801

Also:

• Delete unnecessary scripts (golangci-lint already does that)
• Update documentation around linter (which was outdated)
• sorted things alphabetically to more easy read of the config

This PR would also change golangci-lint config file to default name and add golangci-lint github action, which was already done in 884bcbe and rebased on top of it.

Does your change fix a particular issue?

Related to #6023

Please indicate you've done the following:

• Accepted the DCO. Commits without the DCO will delay acceptance.
• Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
• Updated the corresponding documentation in site/content/docs/main.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.99%. Comparing base (c53ab20) to head (180f6d2).
Report is 166 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7194   +/-   ##
=======================================
  Coverage   58.99%   58.99%           
=======================================
  Files         367      367           
  Lines       38847    38847           
=======================================
  Hits        22918    22918           
  Misses      14467    14467           
  Partials     1462     1462           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kaovilai]

Why use make lint-fix over make update?

[kaovilai]

make update is how we today make sure the format satisfy the ci lint. If make lint-fix is replacing that flow then note that in the PR description.

[kaovilai]

This PR breaks make update by removing hack/update-1fmt.sh that is relied upon by

update:
	@$(MAKE) shell CMD="-c 

> 'hack/update-all.sh'"

https://github.com/vmware-tanzu/velero/blob/dcd4392744b10cde4984de7d1a423aff3dd74fa5/Makefile#L234C1-L235C46

which runs

for f in ${HACK_DIR}/update-*.sh; do
  if [[ $f = "${HACK_DIR}/update-all.sh" ]]; then
    continue
  fi
  $f
done

https://github.com/vmware-tanzu/velero/blob/dcd4392744b10cde4984de7d1a423aff3dd74fa5/hack/update-all.sh

Correct me if I'm wrong but is this PR created because you were not aware of the make update?

If this PR merge, we now have to run make update lint-fix instead to fix lint and generate CRDs etc.

[mateusoliveira43]

@kaovilai I did not intend to change make update at first. That command would run gofmt and goimports (which is a different thing then generating files) and generate files, right?

I removed hack/update-1fmt.sh because we would be doing the same thing twice (with the script and with golangci-lint). make-lint-fix only replaces the formatting script, still need to run make update for the scenarios described in documentation https://github.com/vmware-tanzu/velero/blob/main/site/content/docs/main/development.md#update-generated-files

Run make update to regenerate files if you make the following changes:

• Add/edit/remove command line flags and/or their help text
• Add/edit/remove commands or subcommands
• Add new API types
• Add/edit/remove plugin protobuf message or service definitions

So, removed the script for both semantic and performance.

Since update-all script runs a for loop, I just removed a item from the list, but it will still work (example is Pull Request CI Check for this PR).

No no, my intention was to clean up config file for golangci-lint. There was unused config (for example, linters-settings for linters not enabled, enabled linters without config, etc), found the other issues along the way. I would be ok to break the PR in smaller ones.

Added to PR description a note about removal of scripts.

[kaovilai]

I just removed a item from the list, but it will still work

Just in github actions because it's covered by golangci-lint? or also locally on dev machine for lint formatting.

Is the expectation that make update should still format lint on local dev machine?

[mateusoliveira43]

@tiger oh no, make update will not format Go code anymore (but as it was, it would not run all linters defined in golangci-lint config file, only gofmt and goimport. So, in the past running make update would not guarantee CI would pass)

[kaovilai]

make update will not format Go code anymore
So, in the past running make update would not guarantee CI would pass)

So now we run make update lint-fix before making PR to guarantee CRD generation and also lint right?

[mateusoliveira43]

@kaovilai that's right!

[weshayutin]

@reasonerjt @sseago as you have time can please weigh in on linting :)

[mateusoliveira43]

@mmorel-35 could you please review?

[kaovilai]

I would link fixable issues table here.

[mateusoliveira43]

I think there is not such table

here https://golangci-lint.run/usage/linters/ shows all linters (but this is related only for last release) and those that have ✅ in AutoFix column, are the ones that can be fixable

[mmorel-35]

Isn’t it more common to use make fmt or make format instead ?

You can also define a make verify and reuse it as a prerequisite for lint and fmt targets , wdyt ?

[mateusoliveira43]

Since its inside an if, I think we will write less code by being explicit here instead of writing a new command verify-config and adding it to the 2 lint commands

From what I am familiar (example https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/Makefile#L55) fmt command is commonly used to call go fmt. Since golangci-lint does more than that, and the non fix command already contains lint in its name, I think lint-fix would make more sense here

[sseago]

One thing that's not clear to me is whether the difference in file list for gofmt matters. I'm not quite sure how the new approach in this PR generates the file list for gofmt, but the prior approach explicitly omitted autogenerated files:

-files="$(find . -type f -name '*.go' -not -path './.go/*' -not -path './vendor/*' -not -path './site/*' -not -path './.git/*' -not -path '*/generated/*' -not -name 'zz_generated*' -not -path '*/mocks/*')"

On the other hand, the old gofmt script in make update no longer works for me -- we have too many go files for the explicit arg list to work in bash anymore:

Running all update scripts
Updating gofmt
hack/update-1fmt.sh: line 38: /usr/local/go/bin/gofmt: Argument list too long

So the new approach in this PR is definitely an improvement -- assuming the right files are being selected for gofmt.

[mateusoliveira43]

@sseago did some tests and only files that are not excluded explicitly in the config file, are files under mocks and zz_generated paths

But since those files are generated files (have comments in their headers telling it), the linter tool will exclude them by default. References https://github.com/golangci/golangci-lint/blob/v1.57.2/.golangci.reference.yml#L2835-L2837 https://github.com/golangci/golangci-lint/blob/v1.57.2/.golangci.reference.yml#L2845-L2855

[shubham-pampattiwar]

@mateusoliveira43 Please rebase and resolve the conflicts, Thank you !

[mateusoliveira43]

@shubham-pampattiwar done

[sseago]

Note that the prior error I reported with make update (involving "argument list too long") ended up being a problem with my particular git checkout. But still, the point here is that gofmt is already run as part of lint, so it shouldn't really be needed as part of "make update" as well.