Update golangci-lint, its config, and fix some warnings #3717
Conversation
openshift-ci
bot
added
the
kind/cleanup
|
TODO (once this is merged):
|
|
@nalind PTAL |
|
Changes LGTM, but it looks like parts of |
|
LGTM once tests are happy |
It seems I will never be able to do Updated. |
Fix this warning from gosimple linter:
> util.go:127:19: S1040: type assertion to the same type: ref.DockerReference() already has type reference.Named (gosimple)
> if named, ok := ref.DockerReference().(reference.Named); ok {
> ^
Since containers/image commit dfe2fafaa2d702e5a932721aed55b996024051b1
(made in 2017), DockerReference() always return reference.Named type.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Fix the following warning:
> imagebuildah/executor.go:307:6: S1033: unnecessary guard around call to delete (gosimple)
> if _, stillUnused := exec.unusedArgs[list[0]]; stillUnused {
> ^
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
> tests/e2e/buildah_suite_test.go:166:27: S1025: the argument's underlying type is a slice of bytes, should use a simple conversion instead of fmt.Sprintf (gosimple)
> fields := strings.Fields(fmt.Sprintf("%s", s.Out.Contents()))
> ^
> tests/e2e/buildah_suite_test.go:173:12: S1025: the argument's underlying type is a slice of bytes, should use a simple conversion instead of fmt.Sprintf (gosimple)
> output := fmt.Sprintf("%s", s.Out.Contents())
> ^
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
AKA "unnecessary use of fmt.Sprintf (gosimple)" Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Newer golangci-lint does not understand the nolint: directive because it is followed by some comment. To fix, add a comment before the (human) comment. While at it, remove the space after // since comments targeted for non-humans should not have space (this is currently not enforced by golangci-lint). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit 3eef1ed (January 2019) using capabilities.NewPid() is deprecated. Replace with NewPid2(). Note that in chroot/run.go we used to load then clear all capabilities bits. With NewPid2, this is no longer needed -- we do not load caps, so there is no need to clear. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 3a122aa added ignoring any file or directory named result, which is wrong. What it meant to do is to ignore top-level result directory. Fix accordingly. Same for tests/tools -- this is meant to be the top-level directory only. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
... and its dependencies. Also, modify .golangci.yml since new golangci-lint no longer have some of the linters mentioned. Besides, it is unsafe to enable all linters, because (1) not all linters are good/useful; (2) new golangci-lint releases can bring more linters and thus more CI issues. Instead, use the default set of linters, plus enable a few more: * revive (which replaces golint) * unconvert Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
Ah, now top-level .gitignore stands in a way of proper vendoring. Fixed. |
|
@vrothberg can you please review the 0b86b16? |
|
It seems like fedora CI failure is a temp glitch (quay.io timeout). Can someone please restart it? [+2834s] not ok 535 pull image with TMPDIR set |
|
"Unit tests w/ vfs" job timed out after 60 minutes. A flake? |
LGTM, thanks for cleaning up! |
|
Restarted both failed jobs; looked like flakes to me. |
|
LGTM, and since I don't see any unresolved requests for changes from other maintainers, |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kolyshkin, rhatdan 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Currently, golangci-lint used by this repo is old, and its configuration is unsafe (for updates, that it).
Fix both issues, run the updated linter, fix some of its warnings (surely those fixes come first to not break git-bisect).
Please see individual commits for details.
How to verify it
I hope that CI that runs on PRs is sufficient.
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Some of the fixes are not too trivial, so a careful review is appreciated.
Does this PR introduce a user-facing change?