Fix golint failures in some pkg/kubelet packages #75303

obitech · 2019-03-12T21:41:03Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Fixes golint failures for packages under pkg/kubelet/:

pkg/kubelet/pod
pkg/kubelet/metrics
pkg/kubelet/configmap
pkg/kubelet/config

ref: #68026

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-03-12T21:41:10Z

Hi @obitech. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

obitech · 2019-03-12T21:43:41Z

/assign @vishh

mattjmcnaughton

Overall, this looks good to me (modulo some small changes which I think should be easy)! I don't have approval rights, but thanks for tackling these lint errors :)

One request - can you please also update .hack/.golint_failures to remove these packages (if they are now fully passing golint)?

mattjmcnaughton · 2019-04-04T13:34:16Z

pkg/kubelet/config/apiserver.go

@@ -14,7 +14,6 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-// Reads the pod configuration from the Kubernetes apiserver.


Curiosity question... how did this comment violate the golinter? Or did you just decide it wasn't necessary?

Either is fine, just curious :)

Hmm, when I first ran it, golint gave me an error because every package should only have one package level comment (basically a comment above the package statement). However, if I try to reproduce it (by putting comments back into the top of the files) now, I don't get any errors. If you want I can put them back in. However, I think we should rather come up with a true package-level comment instead of comments describing what the file does.

I don't think we need this comment here, the methods in this file are commented.

the package itself should be documented though.

pkg/kubelet/metrics/metrics.go

mattjmcnaughton · 2019-04-04T13:43:40Z

/ok-to-test

mattjmcnaughton · 2019-04-04T13:45:18Z

@obitech thanks for your pr :) After making the changes I described, you may want to tag an additional approver. You can find a list of approvers here: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/OWNERS

mattjmcnaughton · 2019-04-05T13:39:36Z

Just a heads up, the pull-kubernetes-verify test is raising legitimate errors that we'll want to address before this is merged :)

obitech · 2019-04-07T15:26:39Z

Hey Matt,

thanks for your reply on this! I will check out your suggestions. Regarding the pull-kubernetes tests, I'll fix it :)

obitech · 2019-04-07T15:27:45Z

/assign @derekwaynecarr

obitech · 2019-04-07T20:51:47Z

/retest

obitech · 2019-04-13T20:21:19Z

/test pull-kubernetes-kubemark-e2e-gce-big

obitech · 2019-04-21T08:49:14Z

All tests are passing, assigning additional approver for hack/:

/assign @soltysh

obitech · 2019-04-24T19:23:52Z

@derekwaynecarr golint was throwing the following error:

pkg/kubelet/config/common.go:37:2: a blank import should be only in a main or test package, or have a comment justifying it

From what I can tell the import is not used anymore and I had troubles coming up with a reason/comment why it should remain, so I removed it.

obitech · 2019-05-31T20:54:15Z

Does this PR need any more attention from me? Any chance to merge this before the code freeze?

mattjmcnaughton · 2019-06-01T14:14:27Z

Does this PR need any more attention from me? Any chance to merge this before the code freeze?

Unfortunately, I think this missed code freeze :( Hopefully it will be an early candidate for merge when the repo unfreezes.

In the mean time, I think you need to do a quick rebase because of some conflicts with hack/.golint_failures.

Thanks again for your work here :)

obitech · 2019-06-01T19:25:47Z

Ah no worries, I'll wait a bit longer then :)

obitech · 2019-06-01T21:05:12Z

/retest

dims · 2019-07-08T15:31:14Z

/uncc

derekwaynecarr

have a question on why an import was removed.

derekwaynecarr · 2019-04-24T19:11:31Z

pkg/kubelet/config/common.go

 	// TODO: remove this import if
 	// api.Registry.GroupOrDie(v1.GroupName).GroupVersion.String() is changed
 	// to "v1"?
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
-	_ "k8s.io/kubernetes/pkg/apis/core/install"


why was this removed?

I removed it becaused golint was complaining the blank import:

pkg/kubelet/config/common.go:37:2: a blank import should be only in a main or test package, or have a comment justifying it

@derekwaynecarr should I put it back in? I'll have to put pkg/kubelet/config back on .golint-failures though.

that package has an init function.

https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/install/install.go#L29

I think you need to keep the import but add a comment so it passes.

I put the import back in, unfortunately golint doesn't seem to allow ignoring individual lines so I had to put pkg/kubelet/config back into .golint-failures. But we're still passing a lot more golint so this should be fine I think

I put the import back in, unfortunately golint doesn't seem to allow ignoring individual lines so I had to put pkg/kubelet/config back into .golint-failures.

er to clarify ...

I think you need to keep the import but add a comment so it passes.

@derekwaynecarr is not saying to put comment to suppress go lint like say, pylint. go lint is requiring that you have a comment justifying the import if you have an underscore import, IE we should comment about the dependency on init()

obitech · 2019-07-10T17:56:23Z

/test pull-kubernetes-e2e-gce-100-performance

Fixed: - pkg/kubelet/pod - pkg/kubelet/metrics - pkg/kubelet/configmap - pkg/kubelet/config

derekwaynecarr

/approve
/lgtm

obitech · 2019-08-03T20:44:51Z

/assign @BenTheElder

BenTheElder

/approve
for hack

I suspect you can get these other packages passing again and remove them as well by commenting on why we underscore import these other packages.

k8s-ci-robot · 2019-08-05T19:37:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, derekwaynecarr, obitech

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:

~~hack/OWNERS~~ [BenTheElder]
~~pkg/kubelet/OWNERS~~ [derekwaynecarr]

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

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 12, 2019

k8s-ci-robot requested review from dims and sjenning March 12, 2019 21:41

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 12, 2019

obitech mentioned this pull request Mar 12, 2019

Fix golint failures #68026

Closed

k8s-ci-robot assigned vishh Mar 12, 2019

mattjmcnaughton approved these changes Apr 4, 2019

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 4, 2019

k8s-ci-robot assigned derekwaynecarr Apr 7, 2019

obitech force-pushed the fix_golint_failures_pkg_kubelet branch from 25b1fc8 to 7df283f Compare April 7, 2019 20:48

obitech force-pushed the fix_golint_failures_pkg_kubelet branch from 7df283f to c8c2368 Compare April 11, 2019 22:00

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2019

obitech force-pushed the fix_golint_failures_pkg_kubelet branch from 5646bc0 to 22bf71b Compare April 27, 2019 08:02

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2019

obitech force-pushed the fix_golint_failures_pkg_kubelet branch from 22bf71b to 0e4b264 Compare May 23, 2019 11:11

obitech force-pushed the fix_golint_failures_pkg_kubelet branch from 55ad932 to 6c4bcb8 Compare June 1, 2019 19:24

k8s-ci-robot removed the request for review from dims July 8, 2019 15:31

obitech force-pushed the fix_golint_failures_pkg_kubelet branch from 6c4bcb8 to 50c7d1c Compare July 8, 2019 19:10

derekwaynecarr requested changes Jul 8, 2019

View reviewed changes

obitech added 4 commits August 3, 2019 21:07

Fix golint failures in some pkg/kubelet packages

898c40a

Fixed: - pkg/kubelet/pod - pkg/kubelet/metrics - pkg/kubelet/configmap - pkg/kubelet/config

Fix suggestions, track removed library in bazel

457972f

Fixed pull-kubernetes-verify issues

a5bc997

Re-import api/core/install for init function

17d2ba0

obitech force-pushed the fix_golint_failures_pkg_kubelet branch from 50c7d1c to 17d2ba0 Compare August 3, 2019 19:07

derekwaynecarr approved these changes Aug 3, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2019

k8s-ci-robot assigned BenTheElder Aug 3, 2019

BenTheElder approved these changes Aug 5, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2019

k8s-ci-robot merged commit cea34cc into kubernetes:master Aug 5, 2019

k8s-ci-robot added this to the v1.16 milestone Aug 5, 2019

obitech mentioned this pull request Aug 8, 2019

REQUEST: New membership for obitech kubernetes/org#1083

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix golint failures in some pkg/kubelet packages #75303

Fix golint failures in some pkg/kubelet packages #75303

obitech commented Mar 12, 2019

k8s-ci-robot commented Mar 12, 2019

obitech commented Mar 12, 2019

mattjmcnaughton left a comment

mattjmcnaughton Apr 4, 2019

obitech Apr 7, 2019

BenTheElder Aug 5, 2019

mattjmcnaughton commented Apr 4, 2019

mattjmcnaughton commented Apr 4, 2019

mattjmcnaughton commented Apr 5, 2019 •

edited

obitech commented Apr 7, 2019

obitech commented Apr 7, 2019

obitech commented Apr 7, 2019

obitech commented Apr 13, 2019

obitech commented Apr 21, 2019

obitech commented Apr 24, 2019

obitech commented May 31, 2019

mattjmcnaughton commented Jun 1, 2019

obitech commented Jun 1, 2019

obitech commented Jun 1, 2019

dims commented Jul 8, 2019

derekwaynecarr left a comment

derekwaynecarr Apr 24, 2019

obitech Jul 9, 2019

obitech Jul 11, 2019

derekwaynecarr Aug 3, 2019

obitech Aug 3, 2019

BenTheElder Aug 5, 2019 •

edited

obitech commented Jul 10, 2019

derekwaynecarr left a comment

obitech commented Aug 3, 2019

BenTheElder left a comment

k8s-ci-robot commented Aug 5, 2019

Fix golint failures in some pkg/kubelet packages #75303

Fix golint failures in some pkg/kubelet packages #75303

Conversation

obitech commented Mar 12, 2019

k8s-ci-robot commented Mar 12, 2019

obitech commented Mar 12, 2019

mattjmcnaughton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattjmcnaughton commented Apr 4, 2019

mattjmcnaughton commented Apr 4, 2019

mattjmcnaughton commented Apr 5, 2019 • edited

obitech commented Apr 7, 2019

obitech commented Apr 7, 2019

obitech commented Apr 7, 2019

obitech commented Apr 13, 2019

obitech commented Apr 21, 2019

obitech commented Apr 24, 2019

obitech commented May 31, 2019

mattjmcnaughton commented Jun 1, 2019

obitech commented Jun 1, 2019

obitech commented Jun 1, 2019

dims commented Jul 8, 2019

derekwaynecarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder Aug 5, 2019 • edited

Choose a reason for hiding this comment

obitech commented Jul 10, 2019

derekwaynecarr left a comment

Choose a reason for hiding this comment

obitech commented Aug 3, 2019

BenTheElder left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 5, 2019

mattjmcnaughton commented Apr 5, 2019 •

edited

BenTheElder Aug 5, 2019 •

edited