Switch to golangci-lint

[saschagrunert]

This PR switches from the deprecated gometalinter to golangci-lint. Beside this, two other commits enable the deadcode and varcheck linters as well as fix the first issues.

• If you don't like the code removal I could go for a // nolint on these too
• During cleanup I came across this code path: https://github.com/containers/libpod/blob/6a68ce59e4f4b0c6a246b215b9115895dc83f1e8/cmd/podman/pod_ps.go#L504-L508 Is it correct to have no explicit STOPPED state here?

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: saschagrunert
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mrunalp

If they are not already assigned, you can assign the PR to them by writing /assign @mrunalp in a comment when ready.

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[openshift-ci-robot]

Hi @saschagrunert. Thanks for your PR.

I'm waiting for a containers or openshift 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.

Details

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.

[rh-atomic-bot]

Can one of the admins verify this patch?
I understand the following commands:

• bot, add author to whitelist
• bot, test pull request
• bot, test pull request once

[vrothberg]

bot, add author to whitelist

[saschagrunert]

To make the CI happy we would have to update the quay.io/libpod/gate:latest image and I guess I am not privileged to do this.

[vrothberg]

To make the CI happy we would have to update the quay.io/libpod/gate:latest image and I guess I am not privileged to do this.

Calling @cevich for help 👨‍🚒

[rhatdan]

/ok-to-test

[cevich]

@saschagrunert @vrothberg Quick background: The container that runs the linter is defined in contrib/gate/. It uses a wrapper script entrypoint.sh to call "make whatever". This happens from some commands you'll find in the gate-task of .cirrus.yml. So it's fairly simple and the changes you've made appear like they will be compatible and function as-is.

For this PR, building the container image will happen automatically once it merges. So, in order to validate this process will work, a few temporary manual steps are needed to validate the new linter against this PR:

1. Manually build the gate container image (from repo. root) and stick it somewhere publicly accessible. $ podman build -f contrib/gate/Dockerfile -t some_name:some_tag . then push to docker hub or quay.
2. Add a temporary testing-only commit to this PR that modifies .cirrus.yml to reference your just built image from the gate_task.
3. Push the temporary commit to this PR, and Cirrus CI will take it from there, sending feedback to the PR as before, but using your testing image instead.

To make sure this PR doesn't accidentally merge with a test commit, I'll stick on a:
/hold

Assuming there are no problems, and the output looks clean:

1. I'll give a "go-ahead" to remove test-commit, and I'll un-hold the PR for merging.
2. Post-merge testing will fail (automated image build won't be fast enough).
3. Once the new image is built, I can re-run the post-merge test for a pass.
4. If it breaks, it will fail on everybody's PRs, so I'll jump in quickly to help deal with that if needed.

Make sense?

[cevich]

Only looked at Makefile changes: They all LGTM

[mheon]

OK, these seem like they ought to be used - did we start hardcoding them when we have constants?

[saschagrunert]

Make sense?

@cevich sure, thanks for the explanation. I update the test image as suggested. I guess the machines running ci/prow/lint would need some update too, from which source are they getting their dependencies?

[mheon]

I want to do a quick look at some of the constants we're removing here and see if we've just hardcoded them (instead of using the actual constant), but I love the actual linter change.

[cevich]

I guess the machines running ci/prow/lint would need some update too

I believe that's the ci/prow/images job, super @baude knows how that pipeline works, it's too complicated for my wimpy brain to manage 😁

[cevich]

@saschagrunert Nice, looks great thanks! You can strip the testing commit...unless you need to play more.

[saschagrunert]

@saschagrunert Nice, looks great thanks! You can strip the testing commit...unless you need to play more.

Alright, I reverted the test change.

[cevich]

/hold cancel

[cevich]

@mheon this will have to be manually merged. Tests won't pass without image...image won't build until after merge. They were all green though, with the test-commit.

[saschagrunert]

Maybe both goals could be accomplished by vendoring the linter (and/or using go-modules) to lock it's version (and dependencies)? That way the results can't change from one run to the next, or one platform to the next.

golangci-lint already vendors its linters, so there should be no difference on different platforms. We could lock golangci-lint to a dedicated version.

[mheon]

Version-locking would be appreciated - we did that with Metalinter, though results seemed... mixed (rarely got the same results on bare metal as CI did)

[saschagrunert]

Version-locking would be appreciated - we did that with Metalinter, though results seemed... mixed (rarely got the same results on bare metal as CI did)

Alright, pinned it to v1.15.0

[cevich]

Suggestion: There are a bunch of linters disabled by default, which we were probably previously using. It might be worthwhile reviewing that list and see if there are any which should be re-enabled in golangci-lint.

$ golangci-lint help linters
...
Disabled by default linters:
depguard: Go linter that checks if package imports are in a list of acceptable packages [fast: true, auto-fix: false]
dupl: Tool for code clone detection [fast: true, auto-fix: false]
gochecknoglobals: Checks that no globals are present in Go code [fast: true, auto-fix: false]
gochecknoinits: Checks that no init functions are present in Go code [fast: true, auto-fix: false]
goconst: Finds repeated strings that could be replaced by a constant [fast: true, auto-fix: false]
gocritic: The most opinionated Go source code linter [fast: true, auto-fix: false]
gocyclo: Computes and checks the cyclomatic complexity of functions [fast: true, auto-fix: false]
gofmt: Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification [fast: true, auto-fix: true]
goimports: Goimports does everything that gofmt does. Additionally it checks unused imports [fast: true, auto-fix: true]
golint: Golint differs from gofmt. Gofmt reformats Go source code, whereas golint prints out style mistakes [fast: true, auto-fix: false]
gosec (gas): Inspects source code for security problems [fast: true, auto-fix: false]
interfacer: Linter that suggests narrower interface types [fast: false, auto-fix: false]
lll: Reports long lines [fast: true, auto-fix: false]
maligned: Tool to detect Go structs that would take less memory if their fields were sorted [fast: true, auto-fix: false]
misspell: Finds commonly misspelled English words in comments [fast: true, auto-fix: true]
nakedret: Finds naked returns in functions greater than a specified function length [fast: true, auto-fix: false]
prealloc: Finds slice declarations that could potentially be preallocated [fast: true, auto-fix: false]
scopelint: Scopelint checks for unpinned variables in go programs [fast: true, auto-fix: false]
stylecheck: Stylecheck is a replacement for golint [fast: false, auto-fix: false]
unconvert: Remove unnecessary type conversions [fast: true, auto-fix: false]
unparam: Reports unused function parameters [fast: false, auto-fix: false]

[saschagrunert]

Suggestion: There are a bunch of linters disabled by default, which we were probably previously using. It might be worthwhile reviewing that list and see if there are any which should be re-enabled in golangci-lint.

I verified that every additional linter I would now enable via the config will report issues. To keep the PRs small my usual approach would be to follow up on enabling the linters and fixing the issues in dedicated PRs. WDYT?

[rhatdan]

SGTM
bot, retest this please

[rh-atomic-bot]

☔ The latest upstream changes (presumably #2839) made this pull request unmergeable. Please resolve the merge conflicts.

[saschagrunert]

Rebase on top of the latest master branch.

[openshift-ci-robot]

@saschagrunert: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/lint	023eba2	link	/test lint

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

[cevich]

@mheon @baude we need to manually merge this, and I already LGTM'd a test-run. Is there anything left or can I/we/someone press the magic button?

[rhatdan]

rhatdan prays this works.

[rhatdan]

@cevich @saschagrunert Manually merged. Thanks.

[cevich]

@rhatdan praying is fine here, it's not practical to setup automated testing for these kinds of changes. We did manually test and it was green. Post-merge it's not, but no worries, I'm here. I'll work on mitigating the fallout...which seems relatively minor.

[saschagrunert]

Hey @cevich and @rhatdan, thank you for the merge and I hope it does not imply to many manual adaptions with the build process.

[stevekuznetsov]

FWIW "manually merge even though tests are clearly broken" is not really a great approach -- also surprising @cevich that you have added another parallel set of tests and lints to those provided by Prow? How were the latter lacking?

[rhatdan]

@cevich Said we had a Catch 22 where we could not fix one without force merging the other. Is there a way to clear this up in the OpenShift testers without reverting?

[baude]

what needs to be done is we need to edit the openshift stuff to have both linters ... then do the testing and swap on our ci ... then clean up openshift.