Skip to content

Golangci lint version detection #4784

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

Merged
merged 5 commits into from
May 21, 2025

Conversation

tbourrely
Copy link
Contributor

@tbourrely tbourrely commented May 12, 2025

What?

Update golangci-lint version check & delete removed flag.

Why?

Following contributing guide i ran into two issues.

First, golangci-lint version not properly detected as correct:

 ~/p/k6  master  make check-linter-version
Your installation of golangci-lint is different from the one that is specified in k6's linter config (there it's v2.1.6). Results could be different in the CI.
 ~/p/k6  master  golangci-lint version
golangci-lint has version 2.1.6 built with go1.24.2 from eabc2638 on 2025-05-04T15:41:19Z

Second, golangci-lint run ... fails because of an unrecognized flag :

 ~/p/k6  master  make lint
Your installation of golangci-lint is different from the one that is specified in k6's linter config (there it's v2.1.6). Results could be different in the CI.
Running linters...
Error: unknown flag: --out-format
Failed executing command with error: unknown flag: --out-format
make: *** [Makefile:25: lint] Error 3

If I'm not mistaken, the argument --out-format has been removed in golangci-lint@v2 : https://golangci-lint.run/product/migration-guide/#command-line-flags.

Checklist

  • I have performed a self-review of my code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests for my changes.
  • I have run linter and tests locally (make check) and all pass.

Checklist: Documentation (only for k6 maintainers and if relevant)

Please do not merge this PR until the following items are filled out.

  • I have added the correct milestone and labels to the PR.
  • I have updated the release notes: link
  • I have updated or added an issue to the k6-documentation: grafana/k6-docs#NUMBER if applicable
  • I have updated or added an issue to the TypeScript definitions: grafana/k6-DefinitelyTyped#NUMBER if applicable

@tbourrely tbourrely requested a review from a team as a code owner May 12, 2025 19:14
@tbourrely tbourrely requested review from inancgumus and ankur22 and removed request for a team May 12, 2025 19:14
@tbourrely
Copy link
Contributor Author

Regarding the version pattern matching, I have no idea if golangci-lint version output is platform / architecture dependent.

@inancgumus inancgumus requested review from mstoykov and removed request for ankur22 May 12, 2025 21:03
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution 🙇

The Makefile changes seem fine, and I probably should've done them originally 🤦

The other changes though seem to not be necessary. I can't figure out what they are fixing, so can you please expand on why it was done?

.golangci.yml Outdated
@@ -1,4 +1,4 @@
# v2.1.6
# 2.1.6
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think this is needed?

It was prefixed with v before the update to v2 and it seems you need to update another place to make it work.

I tried locally and the make works only with the other changes, so I am not certain why this is needed.

Copy link
Contributor Author

@tbourrely tbourrely May 13, 2025

Choose a reason for hiding this comment

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

Of course !
It is because of the check-linter-version target which extracts the golangci-lint version from the configuration file and "compares" it which the installed version using grep.

With v2.1.6, in the configuration file, the make target expects v2.1.6 to be in the golangci-lint version output. Although on my system, golangci-lint version output does not contain v2.1.6 but 2.1.6:

golangci-lint has version 2.1.6 built with go1.24.2 from eabc2638 on 2025-05-04T15:41:19Z

Therefore the make target considers the version to be invalid even when it is the right one.

Although I do not know if golangci-lint version output the version like v<version> on some builds and only <version> on others 🤔

The related change prefixes the version with v as golangci-lint github action seems to require the v<version> format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can totally rollback this change if needed and keep only the removal of the deprecated flag ☺️

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, apparently I can not read, sorry about that 🙇

Can you provide some data how you are installing this and what is your OS?

Copy link
Contributor Author

@tbourrely tbourrely May 14, 2025

Choose a reason for hiding this comment

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

no worries.
Here is the os infos:

 ~  cat /etc/os-release | grep PRETTY_NAME
PRETTY_NAME="Ubuntu 24.10"

I followed golangci-lint install guide (binaries section): https://golangci-lint.run/welcome/install/#binaries

Removing the binary and doing it again leads to the same result:

 ~  golangci-lint version
Command 'golangci-lint' not found, but can be installed with:
sudo snap install golangci-lint
~  # binary will be $(go env GOPATH)/bin/golangci-lint
       curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.1.6

       golangci-lint --version
golangci/golangci-lint info checking GitHub for tag 'v2.1.6'
golangci/golangci-lint info found version: 2.1.6 for v2.1.6/linux/amd64
golangci/golangci-lint info installed /home/keiko/go/bin/golangci-lint
golangci-lint has version 2.1.6 built with go1.24.2 from eabc2638 on 2025-05-04T15:41:19Z

I also tried with snap:

 ~  sudo snap install golangci-lint --classic
golangci-lint 2.1.6 from GolangCI-Lint (golangci✓) installed
 ~  golangci-lint version
golangci-lint has version 2.1.6 built with go1.24.2 from eabc2638 on 2025-05-04T15:41:19Z
 ~  which golangci-lint
/snap/bin/golangci-lint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you wish, I can isolate the removal of the deprecated flag in another PR and we can keep digging about the version matching here.

If on your side golangci-lint version uses the v<version> format, maybe the proper solution would be to rework the grep call in order to match both version <version> and version v<version> by using a regex, what do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed this change as a third commit (i'll squash if we keep it 😄)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - can we split thsi in two.

I think this might be a bug with the snap versioning. I can't find anything in the workflow of how it is built, but it seems strange.

Can you potentially report it and ask why it is differing from if you do go install ? (which is how i have it installed)

mstoykov
mstoykov previously approved these changes May 20, 2025
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

It seems like the binary releases all have it without the v .

And after some more checking - it seems that it has always been like this. It seems like we just never did check that and nobody used this so far with binary release.

@mstoykov mstoykov merged commit e0cd9fb into grafana:master May 21, 2025
32 checks passed
@tbourrely
Copy link
Contributor Author

Hello sorry for not being so active the last few days on this PR.
Thanks for the details, fixes, approves and merge 😄

@mstoykov mstoykov added this to the v1.1.0 milestone May 22, 2025
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.

3 participants