Fix undocumented golint errors #69269

miguelbernadi · 2018-09-30T09:18:44Z

What this PR does / why we need it:
Fixes some golint errors due to missing comment descriptions. These were reported by running hack/verify-golint.sh directly, as they were not present in hack/.golint_failures.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to #68026

Special notes for your reviewer:

Release note:

NONE

krmayankk · 2018-09-30T23:04:28Z

/ok-to-test

miguelbernadi · 2018-10-01T18:17:02Z

It seems this PR interferes with the code generation. Any idea why? Is it possible to fix or should this just be discarded, as the golint violation is about comments? @dims

krmayankk · 2018-10-01T19:03:47Z

forgot about this, if this is generated things, then we will have to change this somewhere else . @kubernetes/api-reviewers might have some hints

jennybuckley · 2018-10-01T20:13:15Z

/assign @logicalhan

k8s-ci-robot · 2018-10-01T20:13:16Z

@jennybuckley: GitHub didn't allow me to assign the following users: logicalhan.

Note that only kubernetes members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @logicalhan

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.

logicalhan · 2018-10-01T21:02:11Z

I may be mistaken, but I believe that these files are actually code-generated, which means any lint errors you may be encountering will be reintroduced if these files are regenerated.

It is probably desirable to fix string which is used to generate this code (i.e. kubernetes/staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factoryinterface.go) and then regenerate the files which have this lint error (I think there are more than just these two).

logicalhan · 2018-10-01T21:04:05Z

It seems this PR interferes with the code generation. Any idea why? Is it possible to fix or should this just be discarded, as the golint violation is about comments? Davanum Srinivas

Oh, I just saw your comment. Yes, I believe that these files are code-generated, so it is probably prudent to resolve this at the source and re-generate the files in order to fix the lint error.

logicalhan · 2018-10-01T21:27:50Z

I've looked into this and it seems that these files should be automatically excluded from lint since they are flagged as being code-generated. Would mind running golint against these files directly (without your changes) and letting me know what your output is?

lavalamp · 2018-10-01T22:28:59Z

Please fix at the source: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factoryinterface.go#L79

miguelbernadi · 2018-10-02T08:29:55Z

I've looked into this and it seems that these files should be automatically excluded from lint since they are flagged as being code-generated. Would mind running golint against these files directly (without your changes) and letting me know what your output is?

$ golint ./pkg/client/informers/informers_generated/internalversion/internalinterfaces/factory_interfaces.go
./pkg/client/informers/informers_generated/internalversion/internalinterfaces/factory_interfaces.go:30:6: exported type NewInformerFunc should have comment or be unexported
./pkg/client/informers/informers_generated/internalversion/internalinterfaces/factory_interfaces.go:38:6: exported type TweakListOptionsFunc should have comment or be unexported

$ golint ./staging/src/k8s.io/sample-controller/pkg/client/informers/externalversions/internalinterfaces/factory_interfaces.go
./staging/src/k8s.io/sample-controller/pkg/client/informers/externalversions/internalinterfaces/factory_interfaces.go:30:6: exported type NewInformerFunc should have comment or be unexported
./staging/src/k8s.io/sample-controller/pkg/client/informers/externalversions/internalinterfaces/factory_interfaces.go:38:6: exported type TweakListOptionsFunc should have comment or be unexported

I found them initially by running hack/verify-golint.sh before picking a violation to fix, expecting the results of execution would be empty, and found there were several violations.

miguelbernadi · 2018-10-02T08:30:46Z

Please fix at the source: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factoryinterface.go#L79

I'll be doing this now. I hope it can fit in this PR in a clear history way, otherwise I'll close this and open a new one for that.

miguelbernadi · 2018-10-03T20:55:59Z

/kind cleanup

lavalamp · 2018-10-08T20:50:25Z

staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factoryinterface.go

@@ -76,6 +76,7 @@ func (g *factoryInterfaceGenerator) GenerateType(c *generator.Context, t *types.
 }

 var externalSharedInformerFactoryInterface = `
+// NewInformerFunc is a function returning a SharedIndexInformer


Nit: probably not worth changing, but next time think about customizing the comment for the type. (and ending with a period ;) )

codegen has to be updated for tests to pass, so I'll solve the nit as well. Should I rebase or add them as new commits?

lavalamp · 2018-10-08T20:50:56Z

/lgtm
/approve

fejta-bot · 2018-10-08T23:58:59Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-10-09T02:47:00Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-10-09T05:56:01Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

miguelbernadi · 2018-10-09T07:34:05Z

Addressed the nits and rebased on master (was failing verification due to new code-generated files). Added them as new commits, but open to squash them in 2 or 1 commit, as needed.

miguelbernadi · 2018-10-13T13:59:19Z

@lavalamp ☝️

mikedanese · 2018-10-16T05:12:44Z

Edit: I see we are already fixing this in the code gen. I'm happy with this as is if others are.

...lient/informers/informers_generated/internalversion/internalinterfaces/factory_interfaces.go

miguelbernadi · 2018-10-16T21:28:38Z

@mikedanese @lavalamp Nits addressed and commits squashed.

mikedanese · 2018-10-16T22:13:19Z

/lgtm
/approve

k8s-ci-robot · 2018-10-16T22:13:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, miguelbernadi, mikedanese

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/client/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/code-generator/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/csi-api/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/sample-controller/OWNERS~~ [lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mikedanese · 2018-10-16T22:14:18Z

...les/client-go/pkg/client/informers/externalversions/internalinterfaces/factory_interfaces.go

@@ -35,4 +36,5 @@ type SharedInformerFactory interface {
 	InformerFor(obj runtime.Object, newFunc NewInformerFunc) cache.SharedIndexInformer
 }

+// TweakListOptionsFunc is a function that applies a transform on v1.ListOptions.


Gah I wrote my comment late at night. "is a function that applies a tranformation to a"? "is a function that transforms a"?

language corrected.

mikedanese · 2018-10-16T22:26:50Z

Thanks!

/lgtm

mikedanese · 2018-10-17T00:23:44Z

/retest

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 30, 2018

k8s-ci-robot requested review from dchen1107 and munnerz September 30, 2018 09:18

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 30, 2018

miguelbernadi force-pushed the fix-golint-issues-68026 branch from 610e27f to d7d6205 Compare October 3, 2018 19:52

k8s-ci-robot added the area/code-generation label Oct 3, 2018

miguelbernadi force-pushed the fix-golint-issues-68026 branch from d7d6205 to a198790 Compare October 3, 2018 20:44

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 3, 2018

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 3, 2018

lavalamp reviewed Oct 8, 2018

View reviewed changes

k8s-ci-robot assigned lavalamp Oct 8, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 8, 2018

miguelbernadi force-pushed the fix-golint-issues-68026 branch from a198790 to 029207a Compare October 9, 2018 07:32

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2018

k8s-ci-robot assigned mikedanese Oct 16, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2018

mikedanese removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2018

mikedanese reviewed Oct 16, 2018

View reviewed changes

...lient/informers/informers_generated/internalversion/internalinterfaces/factory_interfaces.go Outdated Show resolved Hide resolved

miguelbernadi force-pushed the fix-golint-issues-68026 branch from 029207a to 9d98412 Compare October 16, 2018 21:19

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2018

mikedanese reviewed Oct 16, 2018

View reviewed changes

Fix golint errors when generating informer code

acf78cd

miguelbernadi force-pushed the fix-golint-issues-68026 branch from 9d98412 to acf78cd Compare October 16, 2018 22:25

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2018

k8s-ci-robot merged commit 3143871 into kubernetes:master Oct 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix undocumented golint errors #69269

Fix undocumented golint errors #69269

miguelbernadi commented Sep 30, 2018

krmayankk commented Sep 30, 2018

miguelbernadi commented Oct 1, 2018

krmayankk commented Oct 1, 2018

jennybuckley commented Oct 1, 2018

k8s-ci-robot commented Oct 1, 2018

logicalhan commented Oct 1, 2018

logicalhan commented Oct 1, 2018

logicalhan commented Oct 1, 2018

lavalamp commented Oct 1, 2018

miguelbernadi commented Oct 2, 2018

miguelbernadi commented Oct 2, 2018

miguelbernadi commented Oct 3, 2018

lavalamp Oct 8, 2018

miguelbernadi Oct 9, 2018

lavalamp commented Oct 8, 2018

fejta-bot commented Oct 8, 2018

fejta-bot commented Oct 9, 2018

fejta-bot commented Oct 9, 2018

miguelbernadi commented Oct 9, 2018

miguelbernadi commented Oct 13, 2018

mikedanese commented Oct 16, 2018 •

edited

miguelbernadi commented Oct 16, 2018

mikedanese commented Oct 16, 2018

k8s-ci-robot commented Oct 16, 2018

mikedanese Oct 16, 2018

miguelbernadi Oct 16, 2018

mikedanese commented Oct 16, 2018

mikedanese commented Oct 17, 2018

Fix undocumented golint errors #69269

Fix undocumented golint errors #69269

Conversation

miguelbernadi commented Sep 30, 2018

krmayankk commented Sep 30, 2018

miguelbernadi commented Oct 1, 2018

krmayankk commented Oct 1, 2018

jennybuckley commented Oct 1, 2018

k8s-ci-robot commented Oct 1, 2018

logicalhan commented Oct 1, 2018

logicalhan commented Oct 1, 2018

logicalhan commented Oct 1, 2018

lavalamp commented Oct 1, 2018

miguelbernadi commented Oct 2, 2018

miguelbernadi commented Oct 2, 2018

miguelbernadi commented Oct 3, 2018

lavalamp Oct 8, 2018

Choose a reason for hiding this comment

miguelbernadi Oct 9, 2018

Choose a reason for hiding this comment

lavalamp commented Oct 8, 2018

fejta-bot commented Oct 8, 2018

fejta-bot commented Oct 9, 2018

fejta-bot commented Oct 9, 2018

miguelbernadi commented Oct 9, 2018

miguelbernadi commented Oct 13, 2018

mikedanese commented Oct 16, 2018 • edited

miguelbernadi commented Oct 16, 2018

mikedanese commented Oct 16, 2018

k8s-ci-robot commented Oct 16, 2018

mikedanese Oct 16, 2018

Choose a reason for hiding this comment

miguelbernadi Oct 16, 2018

Choose a reason for hiding this comment

mikedanese commented Oct 16, 2018

mikedanese commented Oct 17, 2018

mikedanese commented Oct 16, 2018 •

edited