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

lint: enable errcheck and fix some issues #327

Merged
merged 2 commits into from Sep 3, 2019

Conversation

@ferhatelmas
Copy link
Contributor

commented Sep 2, 2019

  • add .errcheck-excluded.txt to disable some function calls as an example
  • fixes #325
lint: enable errcheck and fix some issues
* add .errcheck-excluded.txt to disable some function calls
* related to #325

@ferhatelmas ferhatelmas force-pushed the ferhatelmas:enable-errcheck branch from 16da45d to c066971 Sep 2, 2019

@ferhatelmas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

I had added fixes keyword but it seems not allowed so changed it to related to.

@codecov-io

This comment has been minimized.

Copy link

commented Sep 2, 2019

Codecov Report

Merging #327 into master will decrease coverage by 0.41%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
- Coverage    57.3%   56.89%   -0.42%     
==========================================
  Files          19       19              
  Lines         904      907       +3     
==========================================
- Hits          518      516       -2     
- Misses        335      338       +3     
- Partials       51       53       +2
Impacted Files Coverage Δ
pkg/download/downloader.go 65.71% <0%> (-1.6%) ⬇️
cmd/validate-krew-manifest/main.go 56.12% <0%> (-2.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 798fe49...9ee6a54. Read the comment docs.

flag.Set("logtostderr", "true") // Set glog default to stderr
// Set glog default to stderr
if err := flag.Set("logtostderr", "true"); err != nil {
glog.Fatal(err)

This comment has been minimized.

Copy link
@ahmetb

ahmetb Sep 2, 2019

Member

I think it would be good to use fmt.Printf + os.Exit(1) here, as glog is currently not initialized probably.

This comment has been minimized.

Copy link
@ferhatelmas

ferhatelmas Sep 3, 2019

Author Contributor

updated as suggested.

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

Looks good to me. Thanks for getting this done!

Have you tried changing the PR description from "related to #325" to "fixes #325"? I think it's only forbidden in commit messages but fine in PR descriptions.

@corneliusweig
Copy link
Contributor

left a comment

Looking at this a little more, I do not understand why some errors are not reported. For example, in pkg/receiptsmigration/migration_test.go:66 there is a Setenv call which returns an unhandled error that is not reported. On the other hand, the unhandled MkdirAll error in line 72 is reported. So what is the logic behind reporting some but not all errors?

@@ -0,0 +1,2 @@
os.MkdirAll

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Sep 2, 2019

Contributor

Would it also be possible to explicitly ignore errors in the exact locations where we want to ignore errors. Like so:

_ = os.MkdirAll(...)

This comment has been minimized.

Copy link
@ferhatelmas

ferhatelmas Sep 3, 2019

Author Contributor

definitely. Then we shouldn't have this file for the sake of being explicit all the time.

This comment has been minimized.

Copy link
@ferhatelmas

ferhatelmas Sep 3, 2019

Author Contributor

yes, it's possible. For being explicit, I dropped this file.

@ferhatelmas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

I think it's only forbidden in commit messages but fine in PR descriptions.

@corneliusweig updated and seems so. Thanks!

@@ -0,0 +1,2 @@
os.MkdirAll

This comment has been minimized.

Copy link
@ahmetb

ahmetb Sep 3, 2019

Member

since it's just 2 lines or possibly even 1, let's just drop this file if possible and call errcheck with --flags. it's kinda annoying to have an extra file in repo root just for this.

This comment has been minimized.

Copy link
@ferhatelmas

ferhatelmas Sep 3, 2019

Author Contributor

yes, dropped. Will push soon.

This comment has been minimized.

Copy link
@ferhatelmas

ferhatelmas Sep 3, 2019

Author Contributor

pushed.

@ferhatelmas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

Looking at this a little more, I do not understand why some errors are not reported. For example, in pkg/receiptsmigration/migration_test.go:66 there is a Setenv call which returns an unhandled error that is not reported. On the other hand, the unhandled MkdirAll error in line 72 is reported. So what is the logic behind reporting some but not all errors?

It seems a bug from golangci-lint. Even if errcheck reports it, golangci-lint doesn't report it. Reported at golangci/golangci-lint#657

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Sep 3, 2019

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

/lgtm
waiting for @ahmetb to approve.

@ahmetb

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Sep 3, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, ferhatelmas

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-ci-robot k8s-ci-robot merged commit f7a36f5 into kubernetes-sigs:master Sep 3, 2019

2 of 3 checks passed

tide Not mergeable. Needs approved label.
Details
cla/linuxfoundation ferhatelmas authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ferhatelmas ferhatelmas deleted the ferhatelmas:enable-errcheck branch Sep 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.