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

Review use of nolint directives across code #2384

Closed
15 tasks done
killianmuldoon opened this issue Sep 25, 2023 · 13 comments · Fixed by #2427
Closed
15 tasks done

Review use of nolint directives across code #2384

killianmuldoon opened this issue Sep 25, 2023 · 13 comments · Fixed by #2427
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.

Comments

@killianmuldoon
Copy link
Contributor

killianmuldoon commented Sep 25, 2023

CAPV uses many linters from golangci-lint, but many of these linters are skipped in some packages due to the use of nolint directives. The usage of these should be reviewed and as many as possible should be removed. This work should be tackled as a follow-on from #2058

Some initial findings:

The above usages should be investigated and we should remove where possible. Where not possible we should add comments beside the lint ignore directives explaining why these linters are ignored.

@killianmuldoon
Copy link
Contributor Author

/help

@k8s-ci-robot
Copy link
Contributor

@killianmuldoon:
This request has been marked as needing help from a contributor.

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-help command.

In response to this:

/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.

@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Sep 25, 2023
@zhanggbj
Copy link
Contributor

zhanggbj commented Sep 27, 2023

Also CC @MrDXY @xiujuanx,if you have some bandwidth :-)

@MrDXY
Copy link
Contributor

MrDXY commented Sep 27, 2023

I can take this task as I have done similar work previously. 😄

  • gocritic

@xiujuanx
Copy link
Contributor

xiujuanx commented Sep 27, 2023

I can pick this one:

  • staticcheck: 8

@adityabhatia
Copy link
Contributor

I can pickup nestif cases

@Ankitasw
Copy link
Member

I can pick gosec

@Ankitasw
Copy link
Member

Ankitasw commented Sep 29, 2023

Will pick gocyclo , paralleltest and gocognit
@killianmuldoon do we have to enable these linters, as right now these are disabled?

@Ankitasw
Copy link
Member

@killianmuldoon Also I see other linters have only occurrences <=2, can we combine them in same PR? Since the changes wont be significant.

@Ankitasw
Copy link
Member

Will pick rest of the linters:
errcheck: 2
revive: 2
prealloc: 1
goconst: 1
ineffassign: 1

@adityabhatia
Copy link
Contributor

adityabhatia commented Sep 29, 2023

@killianmuldoon nestif was also disabled, so the nolint:nestif directives were unused.

@chrischdi
Copy link
Member

#2399 seems to be the last open PR for this 🎉

@killianmuldoon
Copy link
Contributor Author

Looks like we've ticked everything off the list here - thanks a lot for all the effort folks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants