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

Update golangci-lint to 1.64.6 and update code to conform #4604

Merged
merged 2 commits into from
Mar 7, 2025

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Mar 6, 2025

What?

Bump golangci-lint to latest version

Why?

Mostly so it actually works with a version that works with go1.24, but also as to get some bugfixes ... and new stuff.

  • usetesting seems okay and we already had tenv - I have done some t.TempDir() fixes around it as well
  • exportloopref - is not relevant after using go 1.22

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 k6-documentation: grafana/k6-docs#PR-NUMBER
  • I have updated the TypeScript definitions: grafana/k6-DefinitelyTyped#PR-NUMBER
  • I have updated the release notes: link

Related PR(s)/Issue(s)

@mstoykov mstoykov requested a review from a team as a code owner March 6, 2025 10:16
@mstoykov mstoykov requested review from inancgumus and joanlopez and removed request for a team March 6, 2025 10:16
@mstoykov mstoykov added this to the v1.0.0-rc1 milestone Mar 6, 2025
Copy link
Contributor

@joanlopez joanlopez left a comment

Choose a reason for hiding this comment

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

Apart from the small comments I made around the usetesting configuration, the rest looks pretty well. Nice improvement!

@mstoykov
Copy link
Contributor Author

mstoykov commented Mar 6, 2025

Hmmm ... I really just didn't want to remove some linting we are doing. But it seems that this doesn't work great with windows, so I am currently for just dropping usetesting entirely and opening an issue around it?

WDYT @joanlopez

@joanlopez
Copy link
Contributor

Hmmm ... I really just didn't want to remove some linting we are doing. But it seems that this doesn't work great with windows, so I am currently for just dropping usetesting entirely and opening an issue around it?

WDYT @joanlopez

I think that's because Unix and Windows handle open files differently: Unix permits removing an open file while Windows does not, but overall I think we should try to always close temporary files in tests as good practice.

Indeed, in this case I suspect that the line defer os.Remove(logFilename) on the failing test (TestFileConsole) is hiding a similar error, and we're likely "leaking" files not actually being removed on Windows when running tests. Not 100% sure, but I'd not be surprised if that's the case.

That said, I think the concrete problem here is that:

  • The file is opened by the console logger, and never closed.
  • The file is opened at the end of the test, to read its contents, and never closed.

So, I'd suggest doing something like bumpGolangci...bumpGolangci-2. There's likely a bit cleaner way of doing so, but just to give you an example of the idea. That seems to be working fine.

@mstoykov
Copy link
Contributor Author

mstoykov commented Mar 6, 2025

OOPS I read that as it is way too hard to make it work and went ahead and reverted the changes.

But if the needed changes are what you provided in that PR do you want to fix the lint problem there and merge it here ?

@mstoykov mstoykov merged commit e8e93e5 into master Mar 7, 2025
27 of 28 checks passed
@mstoykov mstoykov deleted the bumpGolangci branch March 7, 2025 14:52
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