Bump golang to 1.25#123
Conversation
- Update go.mod from go 1.24 to go 1.25 - Update go-test.yml Go version from 1.21 to 1.25 - Update golangci-lint.yml Go version from 1.24 to 1.25 - Upgrade golangci-lint-action from v6.5.0 to v7.0.1 - Upgrade golangci-lint from v1.64 to v2.4 (required for Go 1.25+) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the repository’s Go toolchain baseline and CI tooling to align with Go 1.25, along with small adjustments intended to satisfy newer linting behavior.
Changes:
- Bump the module Go version to 1.25 (
go.mod). - Update GitHub Actions workflows to use Go 1.25 and upgrade the golangci-lint action + golangci-lint version.
- Add/adjust error-ignoring patterns and linter suppressions in a few Go files.
Show a summary per file
| File | Description |
|---|---|
go.mod |
Updates the module’s go version to 1.25. |
.github/workflows/golangci-lint.yml |
Runs lint on Go 1.25; upgrades golangci-lint action and linter version. |
.github/workflows/go-test.yml |
Runs tests on Go 1.25. |
internal/spokes/spokes.go |
Adds nolint directives for errcheck/staticcheck around cleanup/close and a user-facing error message. |
internal/governor/conn.go |
Switches socket close calls to explicit error discard (_ = sock.Close()). |
internal/config/git_test.go |
Adds //nolint:errcheck on temp-dir cleanup defers. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (3)
internal/config/git_test.go:49
- To ignore
RemoveAll's return value, the codebase tends to use an explicit discard (e.g.,_ = os.RemoveAll(...)in test cleanups) rather than//nolint:errcheck. Consider switching tot.Cleanup(func(){ _ = os.RemoveAll(localRepo) })ordefer func(){ _ = os.RemoveAll(localRepo) }()so the intent is clear without a nolint directive.
localRepo, err := os.MkdirTemp("", "repo")
defer os.RemoveAll(localRepo) //nolint:errcheck
internal/config/git_test.go:70
- To ignore
RemoveAll's return value, the codebase tends to use an explicit discard (e.g.,_ = os.RemoveAll(...)in test cleanups) rather than//nolint:errcheck. Consider switching tot.Cleanup(func(){ _ = os.RemoveAll(localRepo) })ordefer func(){ _ = os.RemoveAll(localRepo) }()so the intent is clear without a nolint directive.
localRepo, err := os.MkdirTemp("", "repo")
defer os.RemoveAll(localRepo) //nolint:errcheck
internal/config/git_test.go:89
- To ignore
RemoveAll's return value, the codebase tends to use an explicit discard (e.g.,_ = os.RemoveAll(...)in test cleanups) rather than//nolint:errcheck. Consider switching tot.Cleanup(func(){ _ = os.RemoveAll(localRepo) })ordefer func(){ _ = os.RemoveAll(localRepo) }()so the intent is clear without a nolint directive.
localRepo, err := os.MkdirTemp("", "repo")
defer os.RemoveAll(localRepo) //nolint:errcheck
- Files reviewed: 6/6 changed files
- Comments generated: 4
867e291 to
cc94d8a
Compare
There was a problem hiding this comment.
Pull request overview
Updates the repository’s Go toolchain to Go 1.25 and aligns CI workflows accordingly, with a few accompanying lint-related tweaks.
Changes:
- Bump
go.modlanguage version to Go 1.25. - Update GitHub Actions workflows to run tests/lint with Go 1.25.
- Update
golangci-lintGitHub Action + address new/stricter lint findings (via new ignore directives / explicit discards).
Show a summary per file
| File | Description |
|---|---|
go.mod |
Updates module Go version to 1.25. |
.github/workflows/go-test.yml |
Runs unit/integration tests using Go 1.25. |
.github/workflows/golangci-lint.yml |
Runs golangci-lint with Go 1.25 and updates lint action/version. |
internal/governor/conn.go |
Switches Close() calls to explicit ignored-error discards (_ = ...). |
internal/spokes/spokes.go |
Adds lint suppressions / ignored-error handling around cleanup/close paths. |
internal/config/git_test.go |
Adds lint suppression on temp-dir cleanup in tests. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (4)
internal/config/git_test.go:50
- Prefer explicitly discarding intentionally-ignored errors (e.g.,
t.Cleanup(func(){ _ = os.RemoveAll(localRepo) })or_ = os.RemoveAll(localRepo)) instead of//nolint:errcheck, which is inconsistent with the codebase convention.
localRepo, err := os.MkdirTemp("", "repo")
defer func() { _ = os.RemoveAll(localRepo) }()
assert.NoError(t, err, fmt.Sprintf("unable to create the local Git repo: %s", err))
internal/config/git_test.go:71
- Prefer explicitly discarding intentionally-ignored errors (e.g.,
t.Cleanup(func(){ _ = os.RemoveAll(localRepo) })or_ = os.RemoveAll(localRepo)) instead of//nolint:errcheck, which is inconsistent with the codebase convention.
localRepo, err := os.MkdirTemp("", "repo")
defer func() { _ = os.RemoveAll(localRepo) }()
assert.NoError(t, err, fmt.Sprintf("unable to create the local Git repo: %s", err))
internal/config/git_test.go:90
- Prefer explicitly discarding intentionally-ignored errors (e.g.,
t.Cleanup(func(){ _ = os.RemoveAll(localRepo) })or_ = os.RemoveAll(localRepo)) instead of//nolint:errcheck, which is inconsistent with the codebase convention.
localRepo, err := os.MkdirTemp("", "repo")
defer func() { _ = os.RemoveAll(localRepo) }()
assert.NoError(t, err, fmt.Sprintf("unable to create the local Git repo: %s", err))
.github/workflows/golangci-lint.yml:25
- This PR description only mentions bumping Go, but this workflow also upgrades
golangci-lint-actionand thegolangci-lintmajor version. If this is required for Go 1.25 compatibility, it would help to call that out explicitly; otherwise consider keeping the lint toolchain change in a separate PR to reduce upgrade surface area.
- uses: golangci/golangci-lint-action@9fae48acfc02a90574d7c304a1758ef9895495fa # v7.0.1
with:
# Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version.
version: v2.4
- Files reviewed: 6/6 changed files
- Comments generated: 2
There was a problem hiding this comment.
Pull request overview
This PR updates the repository’s Go toolchain to 1.25 and aligns CI/lint tooling and code with newer lint expectations as part of an incremental upgrade path before 1.26.
Changes:
- Bump module Go version to
go 1.25and update GitHub Actions workflows to use Go 1.25. - Upgrade
golangci-lintGitHub Action and configuredgolangci-lintversion. - Adjust several call sites to explicitly discard ignored errors (e.g.,
_ = Close()/_ = RemoveAll()), and tweak a user-facing error formatting string.
Show a summary per file
| File | Description |
|---|---|
go.mod |
Updates the module’s Go version directive to 1.25. |
.github/workflows/go-test.yml |
Runs unit/integration CI with Go 1.25. |
.github/workflows/golangci-lint.yml |
Runs lint with Go 1.25 and upgraded golangci-lint action/version. |
internal/spokes/spokes.go |
Minor error-message formatting change and explicit discards for ignored errors. |
internal/governor/conn.go |
Explicitly discards Close() errors on several error paths. |
internal/config/git_test.go |
Wraps deferred RemoveAll calls to explicitly discard errors. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (3)
internal/config/git_test.go:52
- Same pattern here: cleanup is deferred and
localRepois used even ifos.MkdirTempreturns an error (sinceassert.NoErrordoesn’t abort). Please fail fast onerrbefore deferring cleanup and before running git commands inlocalRepo.
func TestGetConfigEntryValues(t *testing.T) {
localRepo, err := os.MkdirTemp("", "repo")
defer func() { _ = os.RemoveAll(localRepo) }()
assert.NoError(t, err, fmt.Sprintf("unable to create the local Git repo: %s", err))
cmd := commandBuilderInDir(localRepo)
internal/config/git_test.go:74
- Same issue:
os.MkdirTemperror handling should be fatal before usinglocalRepoand before deferringRemoveAll. With the current flow, a temp-dir creation failure can cause the test to run git commands in the repo’s working directory.
func TestGetConfigEntryMultipleValues(t *testing.T) {
localRepo, err := os.MkdirTemp("", "repo")
defer func() { _ = os.RemoveAll(localRepo) }()
assert.NoError(t, err, fmt.Sprintf("unable to create the local Git repo: %s", err))
cmd := commandBuilderInDir(localRepo)
internal/config/git_test.go:93
- Same pattern in this test: fail fast on
os.MkdirTemperror before deferring cleanup and before usinglocalRepoas the command working directory, otherwise commands may run in the current directory whenlocalRepois empty.
func TestGetPrefixParsesArgs(t *testing.T) {
localRepo, err := os.MkdirTemp("", "repo")
defer func() { _ = os.RemoveAll(localRepo) }()
assert.NoError(t, err, fmt.Sprintf("unable to create the local Git repo: %s", err))
cmd := commandBuilderInDir(localRepo)
- Files reviewed: 6/6 changed files
- Comments generated: 1
| localRepo, err := os.MkdirTemp("", "repo") | ||
| defer os.RemoveAll(localRepo) | ||
| defer func() { _ = os.RemoveAll(localRepo) }() | ||
|
|
||
| assert.NoError(t, err, fmt.Sprintf("unable to create the local Git repo: %s", err)) |
There was a problem hiding this comment.
defer runs even when os.MkdirTemp fails, and assert.NoError doesn’t stop the test. If err != nil, localRepo may be empty and the subsequent git commands can run in the current working directory, making the test flaky/dangerous. Recommend failing fast (e.g., require.NoError / t.Fatal) before using localRepo, and only deferring cleanup after that.
This issue also appears in the following locations of the same file:
- line 46
- line 67
- line 86
I just want to take smaller steps 1.26 enables Green Tea Garbage Collector by default. So maybe it's better to update to 1.25 first and not skip a major version.