/sig architecture
/sig testing

@dims Hey, I would like to resolve this issue. Could you assign it to me ?

@fengzixu there are a lot of packages so there will be many people working on this. Also we cannot assign issue to folks who are not in the github org for kubernetes. please feel free to pick up a few packages like i mentioned and file a PR.

Make sure you link back to this issue, so folks can see which packages you filed PR for.

OK. I see. I will firstly pick up the package called cmd to fix. @dims

Hi dims. @dims Should i submit my PR before i remove one or two packages (related packages) from the list in - https://github.com/kubernetes/kubernetes/blame/master/hack/.golint_failures ？ Or I first remove the package from list, then I fix the issue and submit a PR?

@fengzixu in your PR, you will have both the fixes to packages and some lines removed from /.golint_failures

When I run verify-golint.sh, I found some errors as below:

➜  hack git:(master) ✗ ./verify-golint.sh
ls: github.com/kubernetes/kubernetes/cluster/addons/fluentd-elasticsearch/es-image/*.go: No such file or directory
!!! Error in ./verify-golint.sh:72
  Error in ./verify-golint.sh:72. 'failedLint=$(ls "$p"/*.go | egrep -v "(zz_generated.*.go|generated.pb.go|generated.proto|types_swagger_doc_generated.go)" | xargs -L1 golint 2>/dev/null)' exited with status 1
Call stack:
  1: ./verify-golint.sh:72 main(...)
Exiting with status 1

I found that there is a go file in github.com/kubernetes/kubernetes/cluster/addons/fluentd-elasticsearch/es-image/. But there is still a error. @dims

@fengzixu

• Say your GOPATH looks like this GOPATH=/Users/dims/go
• Your kubernetes directory should be /Users/dims/go/src/k8s.io/kubernetes ($GOPATH/src/k8s.io/kubernetes)
• cd to /Users/dims/go/src/k8s.io/kubernetes and then run hack/verify-golint.sh from that directory.

Is it necessary to obey all rules in golint? Rules in golint are set in stone. Maybe using gometalinter to ignore some specific errors is better? @dims

@dims I revise some erros about golint in some packages. Please review it. #68113

@mathspanda which errors would you like to ignore? if you can open a new issue with the list of errors to ignore (and proposing gometalinter), we can get some opinions there from others in the community.

Let me take the kubectl cmd part...

pkg/kubectl/cmd

Can someone please review and let me know if I am doing it right? #68176

I ll work on all the packages related to pkg/cloudprovider/*

we get 800-ish pkgs left to fix. my concern is that if we fix them package by package, then such pulls will be flooding the repo. can't we possibly make it in larger pulls? the larger the better?

@yue9944882 +1 to slightly larger pulls with related packages. if allow too large then the reviewers will not be able to do a thorough review. we should spread the load among multiple reviewers. (if we allow a really large pull, we will end up with top level reviewers who will face the burden and the PR(s) will just stagnate)

if we allow a really large pull, we will end up with top level reviewers who will face the burden and the PR(s) will just stagnate

oh good point. we could put one pkg's change into one commit, and merge all changes under one OWNER area into one pull so that it wont spread the load. wdyt? for each pull, we can have hundred-ish lines of change. but it will still be 30~50 pulls i guess? good news is that we can take our time to deal with it, at least before 1.12 😉

@yue9944882 sounds great! please feel free to help make it happen :) (by suggesting groups of packages that kinda go together)

After a couple years of seeing changes spawned from this issue, I don't think this is a good use of contributors' or reviewers' time. golint is very opinionated about:

• exported type and function names, leading to PRs attempting to change exported things people use downstream
• missing documentation on interface implementation functions, which leads to not-very-useful copy/paste of interface godoc

the govet and staticcheck verify jobs are much more useful since they catch correctness issues, but I would recommend closing this issue and dropping the golint checks from CI

golint doc itself discourages from using its output in an automated enforced way:

from https://github.com/golang/lint#purpose:

The suggestions made by golint are exactly that: suggestions. Golint is not perfect, and has both false positives and false negatives. Do not treat its output as a gold standard. We will not be adding pragmas or other knobs to suppress specific warnings, so do not expect or require code to be completely "lint-free". In short, this tool is not, and will never be, trustworthy enough for its suggestions to be enforced automatically, for example as part of a build process.

i guess one can say gofmt is opinionated too, but something good about Golang is that it comes with an opinion and a set of standard tools.

for API documentation at least we found inconsistencies on how most exported API items are documented:
kubernetes/website#23889

golint can help with that, but if there are no "pragmas" to avoid breaking changes and given we have codegen generating non-compliant names enabling golint on API packages is sort of blocked in Kubernetes.

for non-API, non-library, non-public code, i think being inconsistent is mostly OK.

I would recommend closing this issue and dropping the golint checks from CI

no preference on my side. shrug

i guess one can say gofmt is opinionated too, but something good about Golang is that it comes with an opinion and a set of standard tools.

true, but code formatting has the benefits of mostly eliminating meaningless diffs in PRs due to code movement/whitespace/etc

for API documentation at least we found inconsistencies on how most exported API items are documented

if there are specific packages (like API packages) we lack documentation on, adding checks specifically for that could make sense

golint can help with that, but if there are no "pragmas" to avoid breaking changes and given we have codegen generating non-compliant names enabling golint on API packages is sort of blocked in Kubernetes.

exactly, I don't think golint is the right tool for making sure we have API docs

golint can help with that, but if there are no "pragmas" to avoid breaking changes and given we have codegen generating non-compliant names enabling golint on API packages is sort of blocked in Kubernetes.

golint itself doesn't have these and the go project is against this sort of comment AIUI, however golangci-lint does have them.

I think there are plenty of other static analysis lints we should use instead, beyond just staticcheck and vet there is e.g. ineffassign

btw, if we remove golint can we see e.g. all-caps constants or functions with underscores dodging review?

perhaps we should add golangci-lint soon after:
#81518

Completely agree there are better places to spend review bandwidth.

I was curious to see what the effort looked like.

There have been 890 commits that touched hack/.golint_failures over the past 3+ years

/remove-help
/remove-good-first-issue

Overall agree on the bandwidth. Also note that it was a good first issue with very low bar to get them in the repo. May be we need to find a similar low bar, but high value good first issue. UT's come to mind, are there other things we curate for new contributors ?

Do we have an alternative way of enforcing that new exported code (not necessarily API fields) has at least some minimum documentation?

As a reviewer, I ask contributors to add meaningful comments. But reality is that I sometimes stumble into old code that doesn't have documentation because we didn't have golint at some point.

I agree that fix-golint PRs just add burden to reviewers. But, before removing the lint check, can we wait and see if it's enough to discourage people from sending more PRs like this?
Additionally, we can encourage reviewers to just close PRs that only fix golint issues.

+1 for adding burden to reviewers.
However, as a new contributor, I think this a very good opportunity for new contributors to study the code base and submit their first PR. In my opinion, I think this issue would be able to attract more new contributors, get familiar with the code base, and get familiar with the git workflow.

thanks everyone for getting us this far. We essentially gave up on the existing structure/system for golint:
#98063

At some point we can look at adding golangci-lint, there's a WIP in progress in #93656

/close

@dims: Closing this issue.