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 59.20%. Comparing base (ba0dbb9) to head (66c51fb).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7194   +/-   ##
=======================================
  Coverage   59.20%   59.20%           
=======================================
  Files         367      367           
  Lines       30838    30838           
=======================================
  Hits        18259    18259           
  Misses      11119    11119           
  Partials     1460     1460           

☔ 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]

@mmorel-35 could you please review?

I'll start with I believe will get this merged. Please move everything that it unrelated to most important scope of this PR.

For example, move golangci-lint config but don't change it's content. Also don't fix linters issues inside golang files.
You can create create a second PR to modify the content of golangci config and a third to address the fix of a linter.

After this is done. Things are going to be really easier for reviewers.

Do one thing at a time that will greatly help others follow your move.

This seems quite promising but is really too huge to not miss something for a reviewer

[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