Run pre-commit with make #147

amolkahat · 2023-02-27T07:39:18Z

Updating pre-commit hook with make. pre-commit-golang repo is no longer maintained. Instead of using
unmaintained repo we can use make to run pre-commit.

marios

looks right

Makefile

marios · 2023-03-22T07:14:50Z

hack/configure_local_webhook.sh


+    cat ${TMPDIR}/tls.crt ${TMPDIR}/tls.key | base64 -w 0 > ${TMPDIR}/bundle.pem
 fi


for reference you can see this issue fail the precommit linting at openshift/release#35864 (comment)

frenzyfriday · 2023-03-22T10:58:18Z

Looks good to me

rlandy

looks good - pls ask cores for review

.pre-commit-config.yaml

amolkahat · 2023-03-27T11:14:35Z

/test cinder-operator-build-deploy-kuttl

amolkahat · 2023-03-27T12:59:46Z

/test cinder-operator-build-deploy-kuttl

.pre-commit-config.yaml

ASBishop · 2023-03-28T20:18:24Z

.pre-commit-config.yaml

@@ -33,6 +45,11 @@ repos:
      args: ['operator-lint']
      pass_filenames: false

+- repo: https://github.com/dnephin/pre-commit-golang


Wait, I thought the whole purpose of this patch is to not use this repo because it's no longer maintained. Isn't it replaced by the golangci-lint hook at L22?

I think Alan's comment is correct, we already have a target in the makefile for it (golint i mean)

cinder-operator/Makefile

Line 339 in a965c23

golint: get-ci-tools

and it uses the openstack-k8s-operators-ci repo

@ASBishop you referred to line 22 but for clarification go-lint and golangci-lint are two different linters

I see Sandeep requested the addition of go-lint in an earlier review comment here

on some operators we have both e.g. in ceilometer [1]

- id: make-golangci name: make-golangci language: system entry: make args: ['golangci'] AND - id: make-golint name: make-golint language: system entry: make args: ['golint'] pass_filenames: false

but in keystone we only have golangci-lint [2]

- id: golangci-lint name: golangci-lint language: golang types: [go] entry: make args: ["golangci-lint"] pass_filenames: false

In the keystone case we do have a make target for golint [3] but it doesn't seem to be wired up to the hooks in [2]

@Sandeepyadav93 is that correct i.e. should we also enable golint in the hooks for keystone?

[1] https://github.com/openstack-k8s-operators/ceilometer-operator/blob/aa06f9938877798a1744614e43fefc0df8da0eb0/.pre-commit-config.yaml#L27-L45
[2] https://github.com/openstack-k8s-operators/keystone-operator/blob/b9019b4533f4049d41677329134d6dddb0ab0a63/.pre-commit-config.yaml#L4-L10
[3] https://github.com/openstack-k8s-operators/keystone-operator/blob/b9019b4533f4049d41677329134d6dddb0ab0a63/Makefile#L306

While going through https://github.com/golang/lint I found golint is deprecated and frozen now, I think we can start using revive as a replacement for golint.

The Nova team is working on replacing it with revive as well[1].

[1] openstack-k8s-operators/nova-operator#313

Updating pre-commit with golangci-lint plugins.

This PR replace deprecated go-linter using revive (https://golangci-lint.run/usage/linters/#revive). We will add "ginkgolinter" in follow up patch once we merged add pre-commit job in openshift/releases. If we add "ginkgolinter" in this patch then it's causes below issue [1] ``` level=info msg="[config_reader] Config search paths: [./ /home/prow/go/src/github.com/openstack-k8s-operators/dataplane-operator /home/prow/go/src/github.com/openstack-k8s-operators /home/prow/go/src/github.com /home/prow/go/src /home/prow/go /home/prow /home /]" level=info msg="[config_reader] Used config file .golangci.yaml" level=error msg="Running error: unknown linters: 'ginkgolinter', run 'golangci-lint help linters' to see the list of supported linters" ``` This patch also Remove fmt dependency from make tidy as per [2]: The fmt dependency cannot be run if the go.mod file needs an update: ``` $ make tidy go fmt ./... go: updates to go.mod needed; to update it: go mod tidy make: *** [Makefile:103: fmt] Error 1 ``` I have also made some changes as per the comment given here [3] [1]: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openstack-k8s-operators_dataplane-operator/147/pull-ci-openstack-k8s-operators-dataplane-operator-main-golangci/1643555465537261568/artifacts/test/build-log.txt [2]: openstack-k8s-operators/placement-operator#153 [3]: openstack-k8s-operators/cinder-operator#147 (comment)

This PR replace deprecated go-linter using revive (https://golangci-lint.run/usage/linters/#revive). We will add "ginkgolinter" in follow up patch once we merged add pre-commit job in openshift/releases. If we add "ginkgolinter" in this patch then it's causes below issue [1] ``` level=info msg="[config_reader] Config search paths: [./ /home/prow/go/src/github.com/openstack-k8s-operators/dataplane-operator /home/prow/go/src/github.com/openstack-k8s-operators /home/prow/go/src/github.com /home/prow/go/src /home/prow/go /home/prow /home /]" level=info msg="[config_reader] Used config file .golangci.yaml" level=error msg="Running error: unknown linters: 'ginkgolinter', run 'golangci-lint help linters' to see the list of supported linters" This patch also Remove fmt dependency from make tidy as per [1]: The fmt dependency cannot be run if the go.mod file needs an update: ``` $ make tidy go fmt ./... go: updates to go.mod needed; to update it: go mod tidy make: *** [Makefile:103: fmt] Error 1 ``` I have also made some changes as per the comment given here [2] [1]: openstack-k8s-operators/placement-operator#153 [2]: openstack-k8s-operators/cinder-operator#147 (comment)

This PR replace deprecated go-linter using revive (https://golangci-lint.run/usage/linters/#revive). We will add "ginkgolinter" in follow up patch once we merged add pre-commit job in openshift/releases. If we add "ginkgolinter" in this patch then it's causes below issue [1] ``` level=info msg="[config_reader] Config search paths: [./ /home/prow/go/src/github.com/openstack-k8s-operators/dataplane-operator /home/prow/go/src/github.com/openstack-k8s-operators /home/prow/go/src/github.com /home/prow/go/src /home/prow/go /home/prow /home /]" level=info msg="[config_reader] Used config file .golangci.yaml" level=error msg="Running error: unknown linters: 'ginkgolinter', run 'golangci-lint help linters' to see the list of supported linters" ``` This patch also Remove fmt dependency from make tidy as per [2]: The fmt dependency cannot be run if the go.mod file needs an update: ``` $ make tidy go fmt ./... go: updates to go.mod needed; to update it: go mod tidy make: *** [Makefile:103: fmt] Error 1 ``` I have also made some changes as per the comment given here [3] [1]: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openstack-k8s-operators_dataplane-operator/147/pull-ci-openstack-k8s-operators-dataplane-operator-main-golangci/1643555465537261568/artifacts/test/build-log.txt [2]: openstack-k8s-operators/placement-operator#153 [3]: openstack-k8s-operators/cinder-operator#147 (comment)

This PR replace deprecated go-linter using revive (https://golangci-lint.run/usage/linters/#revive). This patch also Remove fmt dependency from make tidy as per [1]: The fmt dependency cannot be run if the go.mod file needs an update: ``` $ make tidy go fmt ./... go: updates to go.mod needed; to update it: go mod tidy make: *** [Makefile:103: fmt] Error 1 ``` I have also made some changes as per the comment given here [2] [1]: openstack-k8s-operators/placement-operator#153 [2]: openstack-k8s-operators/cinder-operator#147 (comment)

This PR replace deprecated go-linter using revive (https://golangci-lint.run/usage/linters/#revive). This patch also Remove fmt dependency from make tidy as per [1] The fmt dependency cannot be run if the go.mod file needs an update: ``` $ make tidy go fmt ./... go: updates to go.mod needed; to update it: go mod tidy make: *** [Makefile:103: fmt] Error 1 `` I have also made some changes as per the comment given here [2] [1]: openstack-k8s-operators/placement-operator#153 [2]: openstack-k8s-operators/cinder-operator#147 (comment)

…rators/renovate/k8s.io Update k8s.io

openshift-ci bot requested review from ASBishop and eharney February 27, 2023 07:39

fmount approved these changes Mar 2, 2023

View reviewed changes

openshift-ci bot assigned fmount Mar 2, 2023

openshift-ci bot added the lgtm label Mar 2, 2023

openshift-merge-robot added the needs-rebase label Mar 10, 2023

amolkahat force-pushed the pre_commit_fixes branch from 60f69fc to 2e384e1 Compare March 18, 2023 09:38

openshift-ci bot removed the lgtm label Mar 18, 2023

openshift-merge-robot removed the needs-rebase label Mar 18, 2023

marios approved these changes Mar 21, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

openshift-ci bot assigned marios Mar 21, 2023

openshift-ci bot added the lgtm label Mar 21, 2023

amolkahat mentioned this pull request Mar 21, 2023

Added pre-commit job for cinder-operator openshift/release#35864

Merged

amolkahat force-pushed the pre_commit_fixes branch from 2e384e1 to 8b0dabe Compare March 21, 2023 10:28

openshift-ci bot removed the lgtm label Mar 21, 2023

marios approved these changes Mar 22, 2023

View reviewed changes

openshift-ci bot added the lgtm label Mar 22, 2023

frenzyfriday mentioned this pull request Mar 22, 2023

Adds pre commit job for cinder openshift/release#37559

Closed

rlandy reviewed Mar 22, 2023

View reviewed changes

fmount approved these changes Mar 22, 2023

View reviewed changes

Sandeepyadav93 requested changes Mar 27, 2023

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

openshift-ci bot assigned Sandeepyadav93 Mar 27, 2023

openshift-ci bot removed the lgtm label Mar 27, 2023

amolkahat force-pushed the pre_commit_fixes branch from 8b0dabe to 0b94ebe Compare March 27, 2023 08:49

ASBishop reviewed Mar 27, 2023

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

amolkahat force-pushed the pre_commit_fixes branch from 0b94ebe to a965c23 Compare March 28, 2023 05:53

ASBishop reviewed Mar 28, 2023

View reviewed changes

bshewale mentioned this pull request Apr 10, 2023

Add revive to golanci openstack-k8s-operators/designate-operator#37

Merged

bshewale mentioned this pull request Apr 12, 2023

Add revive to golanci openstack-k8s-operators/mariadb-operator#90

Merged

bshewale mentioned this pull request Apr 20, 2023

Add revive to golanci openstack-k8s-operators/manila-operator#53

Merged

ASBishop pushed a commit to ASBishop/cinder-operator that referenced this pull request Mar 11, 2024

Merge pull request openstack-k8s-operators#147 from openstack-k8s-ope…

613aadb

…rators/renovate/k8s.io Update k8s.io

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run pre-commit with make #147

Run pre-commit with make #147

amolkahat commented Feb 27, 2023

marios left a comment

marios Mar 22, 2023

frenzyfriday commented Mar 22, 2023

rlandy left a comment

amolkahat commented Mar 27, 2023

amolkahat commented Mar 27, 2023

ASBishop Mar 28, 2023

marios Mar 29, 2023

Sandeepyadav93 Mar 29, 2023

amolkahat Mar 29, 2023


		cat ${TMPDIR}/tls.crt ${TMPDIR}/tls.key \| base64 -w 0 > ${TMPDIR}/bundle.pem
		fi

Run pre-commit with make #147

Run pre-commit with make #147

Conversation

amolkahat commented Feb 27, 2023

marios left a comment

Choose a reason for hiding this comment

marios Mar 22, 2023

Choose a reason for hiding this comment

frenzyfriday commented Mar 22, 2023

rlandy left a comment

Choose a reason for hiding this comment

amolkahat commented Mar 27, 2023

amolkahat commented Mar 27, 2023

ASBishop Mar 28, 2023

Choose a reason for hiding this comment

marios Mar 29, 2023

Choose a reason for hiding this comment

Sandeepyadav93 Mar 29, 2023

Choose a reason for hiding this comment

amolkahat Mar 29, 2023

Choose a reason for hiding this comment