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

fix for the linter #1935

Merged
merged 1 commit into from
Jan 4, 2021
Merged

fix for the linter #1935

merged 1 commit into from
Jan 4, 2021

Conversation

JacobTanenbaum
Copy link
Contributor

@JacobTanenbaum JacobTanenbaum commented Jan 4, 2021

there seems to be a regression in golangci-lint where the "--modules-download-mode=vendor"
is not supported. Remove the modules-download-mode until the regression
is fixed

Regression Issue: golangci/golangci-lint#1502

Signed-off-by: Jacob Tanenbaum jtanenba@redhat.com

- What this PR does and why is it needed

- Special notes for reviewers

- How to verify it

- Description for the changelog

@coveralls
Copy link

coveralls commented Jan 4, 2021

Coverage Status

Coverage increased (+0.01%) to 54.263% when pulling 0a9ef9f on JacobTanenbaum:linter-fix into c97eff0 on ovn-org:master.

@abhat
Copy link
Contributor

abhat commented Jan 4, 2021

Should we pin to 1.33.2 of golangci-lint instead?

@JacobTanenbaum
Copy link
Contributor Author

that might be a better solution. @abhat would it be better to use docker to launch the linter and pin the version that way so that no matter what linter people have locally the same version as CI will be run?

@girishmg
Copy link
Member

girishmg commented Jan 4, 2021

@JacobTanenbaum that is what I did downstream over the weekend to fix the issue.

[gmoodalbail@gmoodalbail-mlt:~/gowork/src/github.com/ovn-org/ovn-kubernetes/go-controller]
>git diff
diff --git a/go-controller/Makefile b/go-controller/Makefile
index a77e15f9..4133a874 100644
--- a/go-controller/Makefile
+++ b/go-controller/Makefile
@@ -10,6 +10,9 @@ GOPATH ?= $(shell go env GOPATH)
 TEST_REPORT_DIR?=$(CURDIR)/_artifacts
 export TEST_REPORT_DIR
+# Hack for SDN-663
+TAG=v1.33.0
+
 .PHONY: all build check test
 # Example:
@@ -45,7 +48,7 @@ clean:
 .PHONY: install.tools lint gofmt
 install.tools:
-       curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | bash -s -- -b ${GOPATH}/bin
+       curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | bash -s -- -b ${GOPATH}/bin ${TAG}
 lint:
        @GOPATH=${GOPATH} ./hack/lint.sh

@abhat
Copy link
Contributor

abhat commented Jan 4, 2021

@JacobTanenbaum that is what I did downstream over the weekend to fix the issue.

[gmoodalbail@gmoodalbail-mlt:~/gowork/src/github.com/ovn-org/ovn-kubernetes/go-controller]
>git diff
diff --git a/go-controller/Makefile b/go-controller/Makefile
index a77e15f9..4133a874 100644
--- a/go-controller/Makefile
+++ b/go-controller/Makefile
@@ -10,6 +10,9 @@ GOPATH ?= $(shell go env GOPATH)
 TEST_REPORT_DIR?=$(CURDIR)/_artifacts
 export TEST_REPORT_DIR
+# Hack for SDN-663
+TAG=v1.33.0
+
 .PHONY: all build check test
 # Example:
@@ -45,7 +48,7 @@ clean:
 .PHONY: install.tools lint gofmt
 install.tools:
-       curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | bash -s -- -b ${GOPATH}/bin
+       curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | bash -s -- -b ${GOPATH}/bin ${TAG}
 lint:
        @GOPATH=${GOPATH} ./hack/lint.sh

Yep, this is exactly what I was thinking of. It may be useful to write a Dockerfile and run the linter that way to create a hermetic environment.

@JacobTanenbaum
Copy link
Contributor Author

@abhat there is already a way to run the linter in docker docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.34.1 golangci-lint run -v from https://golangci-lint.run/usage/install/ I will get this up soon with a tagged version 1.33.2

upgrading the linter hits the regression below
Regression in golangci-lint: golangci/golangci-lint#1502

pin the version of the linter and run the linter in docker to ensure a
single version is used by CI and locally by developers

Signed-off-by: Jacob Tanenbaum <jtanenba@redhat.com>
@JacobTanenbaum
Copy link
Contributor Author

@abhat @girishmg how does this look?

Copy link
Contributor

@trozet trozet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Copy link
Contributor

@abhat abhat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@dcbw dcbw merged commit ebdef0c into ovn-org:master Jan 4, 2021
qinqon pushed a commit to qinqon/ovn-kubernetes that referenced this pull request Oct 31, 2023
OCPBUGS-15538,OCPBUGS-19961: Downstream merge 2023-10-10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants