Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use stricter go linter configuration. #807

Merged
merged 4 commits into from
Jul 15, 2024

Conversation

jvanz
Copy link
Member

@jvanz jvanz commented Jul 11, 2024

Description

Updates the Go lang linter to stricter configuration. Also, updates the controller code to make the linter happy again.

Fix #778

Test

make lint

Additional Information

I've removed some exclude-rules not used from the suggested configuration file to be used. As well as, disabled the testpackage linter. I don't think we need it for now.

Keep in mind that some changed lines are go fmt output. ;)

@jvanz jvanz self-assigned this Jul 11, 2024
Copy link

codecov bot commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 46.15385% with 28 lines in your changes missing coverage. Please review.

Project coverage is 70.33%. Comparing base (d4cb51f) to head (8d43469).

Files Patch % Lines
internal/controller/policy_subreconciler.go 14.28% 0 Missing and 6 partials ⚠️
...l/controller/policyserver_controller_deployment.go 77.27% 4 Missing and 1 partial ⚠️
...rnal/controller/policyserver_controller_service.go 0.00% 5 Missing ⚠️
...al/controller/clusteradmissionpolicy_controller.go 0.00% 4 Missing ⚠️
internal/controller/policyserver_controller.go 20.00% 0 Missing and 4 partials ⚠️
...nternal/controller/policy_subreconciler_webhook.go 0.00% 2 Missing ⚠️
...al/controller/policyserver_controller_configmap.go 0.00% 0 Missing and 1 partial ⚠️
internal/metrics/metrics.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #807      +/-   ##
==========================================
- Coverage   70.37%   70.33%   -0.05%     
==========================================
  Files          19       19              
  Lines        1708     1719      +11     
==========================================
+ Hits         1202     1209       +7     
- Misses        388      393       +5     
+ Partials      118      117       -1     
Flag Coverage Δ
integration-tests 62.01% <44.23%> (+0.01%) ⬆️
unit-tests 39.86% <16.66%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not bad! I think it looks nice

@jvanz jvanz marked this pull request as ready for review July 11, 2024 17:08
@jvanz jvanz requested a review from a team as a code owner July 11, 2024 17:08
Copy link
Contributor

@fabriziosestito fabriziosestito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!
A couple of comments added

.golangci.yml Show resolved Hide resolved
.golangci.yml Outdated Show resolved Hide resolved
cmd/main.go Show resolved Hide resolved
Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo Fabrizio's comments. I'm ok on punting on the funlen.

@jvanz jvanz force-pushed the issue778-golint branch 2 times, most recently from 838497f to df2782d Compare July 12, 2024 18:37
@jvanz
Copy link
Member Author

jvanz commented Jul 12, 2024

@fabriziosestito @flavio @viccuad can you review it again? I've noticed that the golangci.yml file that I've added in this PR was not the latest one. I've copied the version from spinkube repository.

Now, I've copied the one from Marat Reymers gist and fix everything new that shown up.

Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Although I find weird to go with an allow-list for linters instead of deny-list.

Copy link
Contributor

@fabriziosestito fabriziosestito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!
Love the approach of excluding linters in the configuration file for tests, but keeping the explicit nolint commits for the controller code with an explanation,

.golangci.yml Show resolved Hide resolved
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the change. I'm fine having an allow list, since the configuration of golangci changes A LOT between release, causing quite some problems when new linters are added out of the blue

Updates the golang linter configuration to be more stricter. The used
configuration is originally written by Marat Reymers

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Adapts the golden config for golang linter from Marat Reymers to the
Kubewarden project.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Removes an issue spot by the Go lang linter about using a global
variable.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Fix the code base after updating the Golang linter configuration.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ship it 🚀

@jvanz jvanz merged commit dcfc905 into kubewarden:main Jul 15, 2024
8 of 9 checks passed
@jvanz jvanz deleted the issue778-golint branch July 15, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uniform golangci configuration across all repositories
4 participants