Re-add go-lint to pre-commit

[gibizer]

The go-lint was accidentally dropped form pre-commit by ff52881. This patch adds it back and fixes the issue found by it.

[gibizer]

After this lands we can drop @openshift-ci's ci/prow/golint job as pre-commit will cover it.

[openshift-ci]

@gibizer: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/nova-operator-build-deploy	5fe8668	link	false	/test nova-operator-build-deploy

Full PR test history. Your PR dashboard.

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.

[gibizer]

go-lint now run in the pre-commit job https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openstack-k8s-operators_nova-operator/302/pull-ci-openstack-k8s-operators-nova-operator-master-precommit-check/1638819821648875520/artifacts/test/build-log.txt

go lint..................................................................Passed
- hook id: go-lint
- duration: 0.16s

[mrkisaolamb]

@gibizer: Do we want to add a deprecated checker to our code analysis (https://github.com/golang/lint)? We could use golint, which includes a deprecated checker, but it has been removed from the official Go tools and is no longer actively maintained. Alternatively, we could use other linters like staticcheck (plus already used golangci-lint)

[gibizer]

@gibizer: Do we want to add a deprecated checker to our code analysis (https://github.com/golang/lint)? We could use golint, which includes a deprecated checker, but it has been removed from the official Go tools and is no longer actively maintained. Alternatively, we could use other linters like staticcheck (plus already used golangci-lint)

This is a good question. We had a golint job from the start so we did enforced the rules in golint on the code. As far as I see golint has rules that are not covered by other lints today, hence the fixes needed in the commit.

I'm OK either ways:

• remove golint job and pre-commit call to golint across all the operators
• move golint to pre-commit (as proposed here) and remove the separate golint job form prow to save resources as golint will be run by the pre-commit job

Btw, feel free to try out staticcheck and add it to pre-commit if make sense.

[mrkisaolamb]

• remove golint job and pre-commit call to golint across all the operators

I don't have a really strong opinion on that, but personally, I prefer to remove deprecated libraries from our codebase.

Btw, feel free to try out staticcheck and add it to pre-commit if make sense.

Sure, I will check it out and let you know what I find.

[SeanMooney]

oh i tought that was still there since i have it locally.

[amolkahat]

@gibizer: Do we want to add a deprecated checker to our code analysis (https://github.com/golang/lint)? We could use golint, which includes a deprecated checker, but it has been removed from the official Go tools and is no longer actively maintained. Alternatively, we could use other linters like staticcheck (plus already used golangci-lint)

@mrkisaolamb +1, golint is depricated, so instead of using golint most of the rpos[1][2] are added with golangci-lint.

[1] openstack-k8s-operators/ovn-operator#25
[2] openstack-k8s-operators/glance-operator#171

[SeanMooney]

we had this conversation a few month ago.

golangci-lint. does not actually test the same set of lining rules.

at the time i wanted to remove ti but since all the other repos were not using pre-commit and were using a dedicated golint job we kept it.

what we agreed was if we remove golint form pre-commit we need to remove it form prow/github action and do so on all the repos

we currently still have a ci/prow/golint check job and its required so either we read golint or we need to remove that.

[gibizer]

Yepp I agree with Sean

[mrkisaolamb]

Okay, to me it sounds like the best option is to merge this PR and then remove golint from all repositories and from prow, and replace it with staticcheck. Based on the results from staticcheck, I think this is a promising alternative for golint

[sambork@unknown20c19bd69e21 nova-operator]$ staticcheck -checks="all" ./... controllers/common.go:17:1: at least one file in a package should have a package comment (ST1000) controllers/common.go:64:2: const DbSyncHash should be DBSyncHash (ST1003) controllers/common.go:114:5: error strings should not be capitalized (ST1005) controllers/nova_controller.go:17:1: at least one file in a package should have a package comment (ST1000) controllers/nova_controller.go:197:25: error strings should not be capitalized (ST1005) controllers/nova_controller.go:227:26: error strings should not be capitalized (ST1005) controllers/nova_controller.go:275:25: error strings should not be capitalized (ST1005) controllers/nova_controller.go:303:26: error strings should not be capitalized (ST1005) controllers/novaapi_controller.go:17:1: at least one file in a package should have a package comment (ST1000) controllers/novacell_controller.go:17:1: at least one file in a package should have a package comment (ST1000) controllers/novaconductor_controller.go:17:1: at least one file in a package should have a package comment (ST1000) controllers/novaexternalcompute_controller.go:17:1: at least one file in a package should have a package comment (ST1000) controllers/novametadata_controller.go:17:1: at least one file in a package should have a package comment (ST1000) controllers/novanovncproxy_controller.go:17:1: at least one file in a package should have a package comment (ST1000) controllers/novascheduler_controller.go:17:1: at least one file in a package should have a package comment (ST1000) pkg/nova/common.go:17:1: at least one file in a package should have a package comment (ST1000) pkg/nova/volumes.go:17:1: at least one file in a package should have a package comment (ST1000) pkg/novaapi/const.go:17:1: at least one file in a package should have a package comment (ST1000) pkg/novaapi/deployment.go:17:1: at least one file in a package should have a package comment (ST1000) pkg/novaconductor/const.go:17:1: at least one file in a package should have a package comment (ST1000) pkg/novaconductor/dbsync.go:17:1: at least one file in a package should have a package comment (ST1000) pkg/novaconductor/deployment.go:17:1: at least one file in a package should have a package comment (ST1000) pkg/novaconductor/initcontainer.go:16:1: at least one file in a package should have a package comment (ST1000) pkg/novametadata/const.go:17:1: at least one file in a package should have a package comment (ST1000) pkg/novametadata/deployment.go:17:1: at least one file in a package should have a package comment (ST1000) pkg/novascheduler/const.go:17:1: at least one file in a package should have a package comment (ST1000) pkg/novascheduler/deployment.go:17:1: at least one file in a package should have a package comment (ST1000) test/functional/nova_controller_test.go:175:4: var keystoneApiName should be keystoneAPIName (ST1003) test/functional/nova_controller_test.go:177:4: var keystoneApi should be keystoneAPI (ST1003) test/functional/nova_controller_test.go:428:4: var keystoneApiName should be keystoneAPIName (ST1003) test/functional/nova_controller_test.go:430:4: var keystoneApi should be keystoneAPI (ST1003) test/functional/nova_controller_test.go:514:4: var keystoneApiName should be keystoneAPIName (ST1003) test/functional/nova_controller_test.go:516:4: var keystoneApi should be keystoneAPI (ST1003) test/functional/nova_controller_test.go:642:4: var keystoneApiName should be keystoneAPIName (ST1003) test/functional/nova_controller_test.go:644:4: var keystoneApi should be keystoneAPI (ST1003) test/functional/nova_controller_test.go:705:4: var keystoneApiName should be keystoneAPIName (ST1003) test/functional/nova_controller_test.go:707:4: var keystoneApi should be keystoneAPI (ST1003) test/functional/nova_multicell_test.go:197:4: var keystoneApiName should be keystoneAPIName (ST1003) test/functional/nova_multicell_test.go:199:4: var keystoneApi should be keystoneAPI (ST1003) test/functional/nova_multicell_test.go:600:4: var keystoneApiName should be keystoneAPIName (ST1003) test/functional/nova_multicell_test.go:602:4: var keystoneApi should be keystoneAPI (ST1003) test/functional/nova_reconfiguration_test.go:141:2: var keystoneApiName should be keystoneAPIName (ST1003) test/functional/nova_reconfiguration_test.go:143:2: var keystoneApi should be keystoneAPI (ST1003)

[mrkisaolamb]

I run precommit all is good. lgtm!

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bogdando, gibizer, mrKisaoLamb

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:

• OWNERS [bogdando,gibizer]

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