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

build: fix usage of local golangci-lint installation #28162

Merged

Conversation

mhofstetter
Copy link
Member

@mhofstetter mhofstetter commented Sep 14, 2023

The make target make lint should prefer the locally installed golangci-lint if the installed version matches the expected one. This comes with the advantage of using the local cache.

It seems that there are differences in the version representation between the upstream binaries (including the v prefix) and the one installed via go install ... (without the prefix). The current implementation doesn't support the upstream binaries with the v prefix - it always falls back to executing the docker version.

This commit introduces support for both formats by stripping any given v prefix before comparison.

@mhofstetter mhofstetter added kind/bug This is a bug in the Cilium logic. release-note/misc This PR makes changes that have no direct user impact. area/build Anything to do with the build, more general then area/CI labels Sep 14, 2023
@mhofstetter mhofstetter requested a review from a team as a code owner September 14, 2023 13:44
@mhofstetter
Copy link
Member Author

/test

Makefile Outdated Show resolved Hide resolved
@tklauser
Copy link
Member

(e.g. golangci-lint has version 1.54.2 built with go1.21.0 from 411e0bbb on 2023-08-21T12:04:32Z (that doesn't contain the version with a prefix v)

Interestingly, my locally installed golangci-lint does have the v prefix:

golangci-lint has version v1.54.2 built with go1.21.1 from (unknown, mod sum: "h1:oR9zxfWYxt7hFqk6+fw6Enr+E7F0SN2nqHhJYyIb0yo=") on (unknown)

I guess this is because I installed it using go install. The binaries provided by upstream don't have the v prefix. Presumably this is why we use findstring instead of an exact match.

@mhofstetter
Copy link
Member Author

(e.g. golangci-lint has version 1.54.2 built with go1.21.0 from 411e0bbb on 2023-08-21T12:04:32Z (that doesn't contain the version with a prefix v)

Interestingly, my locally installed golangci-lint does have the v prefix:

golangci-lint has version v1.54.2 built with go1.21.1 from (unknown, mod sum: "h1:oR9zxfWYxt7hFqk6+fw6Enr+E7F0SN2nqHhJYyIb0yo=") on (unknown)

I guess this is because I installed it using go install. The binaries provided by upstream don't have the v prefix. Presumably this is why we use findstring instead of an exact match.

Interesting find. I wondered why this is "suddenly" no longer working. I was pretty sure that this worked even after introducing automated Renovate upgrades. Currently i have it installed via their recommended "binaries" approach.

What's the output of golangci-lint version --format short in your version? If this is more reliable - we should switch to this instead of the full version.

@tklauser
Copy link
Member

tklauser commented Sep 14, 2023

What's the output of golangci-lint version --format short in your version? If this is more reliable - we should switch to this instead of the full version.

% golangci-lint version --format short
v1.54.2

But I guess we cannot rely on that format. Presumably developers will also use the prebuilt binaries so we should probably assume both version formats for the purpose of the lint target.

@mhofstetter
Copy link
Member Author

What's the output of golangci-lint version --format short in your version? If this is more reliable - we should switch to this instead of the full version.

Oh it seems that they also differ in the short version :( Guess i have to remove the prefix anyway

The make target `make lint` should prefer the locally installed
`golangci-lint` if the installed version matches the expected one.
This comes with the advantage of using the local cache.

This behaviour is broken since cilium#24664 (automate golangci-lint upgrades
with renovate).

This commit re-introduces the feature.

Fixes: cilium#24664

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/local-golangci-lint branch from ae730ae to bbd46f9 Compare September 14, 2023 16:02
@mhofstetter
Copy link
Member Author

/test

tklauser added a commit to cilium/cilium-cli that referenced this pull request Sep 14, 2023
This will print just the version, no Go version or other information not
relevant for the version check.

Ref. cilium/cilium#28162

Suggested-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Tobias Klauser <tobias@cilium.io>
tklauser added a commit to cilium/cilium-cli that referenced this pull request Sep 14, 2023
This will print just the version, no Go version or other information not
relevant for the version check.

Ref. cilium/cilium#28162

Suggested-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser merged commit 66c4479 into cilium:main Sep 14, 2023
59 of 61 checks passed
@mhofstetter mhofstetter deleted the pr/mhofstetter/local-golangci-lint branch September 14, 2023 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Anything to do with the build, more general then area/CI kind/bug This is a bug in the Cilium logic. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants