Skip to content

Conversation

kyu08
Copy link
Contributor

@kyu08 kyu08 commented Aug 23, 2025

PR Description

This PR includes 2 changes:

  1. Bump golangci-lint version to v2.4.0 from v.2.2.1
  2. Bump Go version to 1.25.0:
    • Bump Go version to 1.25 in go.mod
    • Bump Go version to 1.25 in ci.yml
    • Bump base image to golang:1.25 in Dockerfile

Reference

Please check if the PR fulfills these requirements

  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
  • Docs have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@stefanhaller
Copy link
Collaborator

In the PR description I'm missing a motivation for making this change. Is the make format thing the only motivation? In that case I vote against doing this now. It's a potentially disruptive change for developers (as some of the discussion in #4393 illustrates), so we should have a really good reason to bump the requirement, and the make format thing is not nearly important enough. Any other reasons?

@kyu08
Copy link
Contributor Author

kyu08 commented Aug 24, 2025

Thanks for the review!

In the PR description I'm missing a motivation for making this change. Is the make format thing the only motivation? In that case I vote against doing this now.

The biggest motivations for this PR are below:

  • Improve development productivity by using new Go feature.
  • Keeping dependencies in a state that doesn't block bumping libraries allows for quick adoption of security patches and new features.

It's a potentially disruptive change for developers (as some of the discussion in #4393 illustrates),

What do you mean by "potentially disruptive change for developers" in this context?
If my understanding is correct, you're claiming that specifying a version without patch version like go 1.25 in go.mod leads to confusion for developers who using Go which is lower version than 1.22.

If so, I think specifying the version with the patch version in go.mod like go 1.25.0 would solve above problem.

If you think that bumping to Go 1.25 would introduce more elements that could lead to developer confusion than we currently have, I'd like to know about that.

@stefanhaller
Copy link
Collaborator

The CI failures you saw here should be fixed with #4910. (Please rebase on master, don't merge master into your branch, thanks.)

The biggest motivations for this PR are below:

  • Improve development productivity by using new Go feature.

This should be the other way round. If there's a useful new go feature that you want to use, then that's a reason to bump the go version. But we don't bump it just in case somebody might want to use a new feature, whatever it might be.

Concretely, the only potentially useful new thing I see in 1.25 is WaitGroup.Go, but that's really just syntactic sugar.

  • Keeping dependencies in a state that doesn't block bumping libraries allows for quick adoption of security patches and new features.

The last time such a situation came up, we bumped the version when it was necessary (#4377, needed to be able to update the go-git dependency). Sure, it's a little more work then, but it doesn't happen often in my experience.

What do you mean by "potentially disruptive change for developers" in this context?

It can be a hassle for users who have an older go version, because go's supposedly automatic toolchain management doesn't work nearly as well as it should. The discussions in #4393 and #4377 have several examples of this. We should spare users the hassle unless we have a really good reason.

@kyu08
Copy link
Contributor Author

kyu08 commented Sep 20, 2025

The CI failures you saw here should be fixed with #4910. (Please rebase on master, don't merge master into your branch, thanks.)

Thanks for the fix! I dropped the merge commit and rebased on master. 🙇

I understand that you prefer bumping dependencies when needed.
Also, I fully understand that there are advantages to your approach of deferring changes until they become necessary.

I updated the PR Body.
I will create another PR for replace this workaround with the way using ignore directive after merging this PR, since the new version of gofumpt which is supporting ignore directive is released as v0.9.0.

Could you please review this PR again when you have time?

Thank you.

@kyu08 kyu08 marked this pull request as ready for review September 20, 2025 13:46
@stefanhaller
Copy link
Collaborator

Looks great, thanks. Just one tiny nitpick about the commit history.

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.

2 participants