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

Pass files to golint one at a time #67675

Merged
merged 3 commits into from Aug 22, 2018

Conversation

Projects
None yet
4 participants
@cblecker
Member

cblecker commented Aug 21, 2018

What this PR does / why we need it:
When we pass multiple files into golint at once, and golint detects a fatal error in one of them, it will actually exit and not process any more of the files passed. For large packages, this increases the chance that golint will exit.

This change will, instead of golinting once per package, will use xargs to run each file through golint individually.

Special notes for your reviewer:
Out of interest, I timed the difference between using find and a regex to exclude the generated files, but ls | grep was about 15-20 seconds faster throughout the entire duration of the run (about 2 minutes).

Release note:

NONE
@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Aug 21, 2018

Member

/cc @nikhita

Still need to write stuff up on this one, but figured I might as well throw it in front of CI while I grab some lunch.

Member

cblecker commented Aug 21, 2018

/cc @nikhita

Still need to write stuff up on this one, but figured I might as well throw it in front of CI while I grab some lunch.

@k8s-ci-robot k8s-ci-robot requested a review from nikhita Aug 21, 2018

@nikhita

This comment has been minimized.

Show comment
Hide comment
@nikhita

nikhita Aug 21, 2018

Member

@cblecker just a heads up that once #67635 merges, the list would need to be updated again.

Member

nikhita commented Aug 21, 2018

@cblecker just a heads up that once #67635 merges, the list would need to be updated again.

@nikhita

This comment has been minimized.

Show comment
Hide comment
@nikhita

nikhita Aug 21, 2018

Member

/cc @spxtr
since you had updated this file previously

Member

nikhita commented Aug 21, 2018

/cc @spxtr
since you had updated this file previously

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Aug 22, 2018

Member

This is what the failures look like now that it's actually looking at all files:
https://gist.github.com/cblecker/55b0f86d93b5b7c6d6857d68f35668fc

Member

cblecker commented Aug 22, 2018

This is what the failures look like now that it's actually looking at all files:
https://gist.github.com/cblecker/55b0f86d93b5b7c6d6857d68f35668fc

@cblecker cblecker changed the title from [WIP] fix golint to Pass files to golint one at a time Aug 22, 2018

@nikhita

This comment has been minimized.

Show comment
Hide comment
@nikhita

nikhita Aug 22, 2018

Member

Adding some more details so that if someone comes across this, it's easier for them to understand the line of thought. :)

When we pass multiple files into golint at once, and golint detects a fatal error in one of them, it will actually exit and not process any more of the files passed.

This is due to an existing golint bug: golang/lint#68 (comment). If we lint a package using golint p/*.go and the package p contains test files with the package name p_test, this gives a fatal error, something like:

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go is in package unstructured_test, not unstructured

and skips the linting. Current behaviour was to ignore this error, which also meant that we ignored the linting of these packages all together.

Also, it's not possible to lint the whole package together through golint p because that would not lint _test.go files due to another golint bug: golang/lint#200.

This change will, instead of golinting once per package, will use xargs to run each file through golint individually.

And so we use xargs to make sure we are linting all files and don't hit any errors.

Member

nikhita commented Aug 22, 2018

Adding some more details so that if someone comes across this, it's easier for them to understand the line of thought. :)

When we pass multiple files into golint at once, and golint detects a fatal error in one of them, it will actually exit and not process any more of the files passed.

This is due to an existing golint bug: golang/lint#68 (comment). If we lint a package using golint p/*.go and the package p contains test files with the package name p_test, this gives a fatal error, something like:

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go is in package unstructured_test, not unstructured

and skips the linting. Current behaviour was to ignore this error, which also meant that we ignored the linting of these packages all together.

Also, it's not possible to lint the whole package together through golint p because that would not lint _test.go files due to another golint bug: golang/lint#200.

This change will, instead of golinting once per package, will use xargs to run each file through golint individually.

And so we use xargs to make sure we are linting all files and don't hit any errors.

cblecker added some commits Aug 22, 2018

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Aug 22, 2018

Member

@nikhita Addressed your feedback. PTAL!

Also thanks for the amazing write up 🗒 ! I linked to this PR in the comments to so future bash spelunkers will be able to find it! :bash:

Member

cblecker commented Aug 22, 2018

@nikhita Addressed your feedback. PTAL!

Also thanks for the amazing write up 🗒 ! I linked to this PR in the comments to so future bash spelunkers will be able to find it! :bash:

@nikhita

This comment has been minimized.

Show comment
Hide comment
@nikhita

nikhita Aug 22, 2018

Member

I linked to this PR in the comments to so future bash spelunkers will be able to find it!

🎉

/lgtm
/retest

Member

nikhita commented Aug 22, 2018

I linked to this PR in the comments to so future bash spelunkers will be able to find it!

🎉

/lgtm
/retest

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 22, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, nikhita

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:

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

Contributor

k8s-ci-robot commented Aug 22, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, nikhita

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:

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

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 22, 2018

Contributor

Automatic merge from submit-queue (batch tested with PRs 67378, 67675, 67654). If you want to cherry-pick this change to another branch, please follow the instructions here.

Contributor

k8s-merge-robot commented Aug 22, 2018

Automatic merge from submit-queue (batch tested with PRs 67378, 67675, 67654). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 0c75bf6 into kubernetes:master Aug 22, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation cblecker authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@cblecker cblecker deleted the cblecker:fix-golint branch Aug 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment