feat: upgrade all components to go 1.25#1289
Conversation
miklosbarabas
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughBumps Go to 1.25 across CI and go.mod files. Updates GitHub Actions (checkout to v4, setup-go to v5), introduces/aligns Go version matrices, and updates golangci-lint action/config versions in both workflows. No source code or public API changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
miklosbarabas
force-pushed
the
miklos/eng-7608-upgrade-to-go-125
branch
3 times, most recently
from
afbc0c6 to
5540df9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/execution.yml (1)
33-35: Update actions/setup-go to v5 (Node20 deprecation).
actionlint flags v4 as too old; use v5 to stay supported.Apply:
- - name: Set up Go ${{ matrix.go }} - uses: actions/setup-go@v4 + - name: Set up Go ${{ matrix.go }} + uses: actions/setup-go@v5 with: go-version: ^${{ matrix.go }} ... - - name: Set up Go 1.25 - uses: actions/setup-go@v4 + - name: Set up Go 1.25 + uses: actions/setup-go@v5 with: - go-version: 1.25 + go-version: ^1.25Also applies to: 51-55
🧹 Nitpick comments (9)
execution/go.mod (1)
3-3: Go 1.25 directive: looks good; consider pinning toolchain for reproducibility.Optional: add a toolchain directive at the root to ensure everyone uses the same Go toolchain locally and in CI.
Apply at the module root(s):
go 1.25 +toolchain go1.25.0examples/federation/go.mod (1)
3-3: Go 1.25 bump is fine; keep modules tidy.Run go mod tidy in this module after the version bump to avoid stale entries.
go.mod (1)
3-3: Root Go version updated to 1.25: OK; optionally enforce toolchain.To avoid local/CI drift, consider adding a toolchain directive here as the canonical source.
go 1.25 +toolchain go1.25.0v2/go.mod (1)
3-3: Go 1.25 directive: OK; remember to tidy.Please run go mod tidy in v2 to ensure the graph matches the new language version.
.golangci.yml (2)
1-7: Adopting v2 config + formatters is good; avoid double-reporting with gofmt.
You enabled both formatters.gofmt and the gofmt linter. One is enough; keep formatters and drop the linter to prevent duplicate failures.Apply:
linters: enable: - - gofmt # - bodyclose - errcheck - gosimple - govet - ineffassign - staticcheck - typecheck
22-28: GCI config present but linter disabled.
You have linters-settings.gci but the linter is commented out. Either enable it or remove the settings to avoid confusion.Enable:
linters: enable: + - gci # - bodyclose…or remove the gci block under linters-settings if you don’t want it.
.github/workflows/execution.yml (3)
31-31: Bump checkout to v4.
actions/checkout@v4 is the current major; reduces risk of deprecations.- uses: actions/checkout@v3 + uses: actions/checkout@v4Also applies to: 50-50
33-36: Enable Go module caching for faster CI.
setup-go supports caching; speeds both jobs.with: go-version: ^${{ matrix.go }} + cache: true ... with: go-version: ^1.25 + cache: trueAlso applies to: 51-55
23-25: Optional: add macOS to the matrix if it’s a supported target.
Only if you ship binaries or rely on platform-specific behavior.- os: [ubuntu-latest, windows-latest] + os: [ubuntu-latest, windows-latest, macos-latest]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
examples/federation/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.work
📒 Files selected for processing (7)
.github/workflows/execution.yml(2 hunks).github/workflows/v2.yml(2 hunks).golangci.yml(1 hunks)examples/federation/go.mod(1 hunks)execution/go.mod(1 hunks)go.mod(1 hunks)v2/go.mod(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
go.modv2/go.modexecution/go.modexamples/federation/go.mod
🪛 actionlint (1.7.7)
.github/workflows/v2.yml
56-56: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/execution.yml
52-52: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (4)
.github/workflows/v2.yml (2)
60-64: Validate golangci-lint versions and inputs.Confirm golangci/golangci-lint-action@v8.0.0 and linter version v2.4.0 exist for your org’s runners; update if needed.
27-27: Go test matrix set to 1.25: LGTM.Matrix definition is clear and consistent with the module bumps.
.golangci.yml (1)
1-7: Confirm CI behavior for formatters (autofix vs check).
Formatters can auto-fix or just check. Verify the workflow doesn’t pass --fix (current execution.yml shows none). If you want checks only, you’re good; if you want auto-fix, add --fix explicitly..github/workflows/execution.yml (1)
56-60: golangci-lint inputs look consistent with v2 config.
Action v8 + linter v2.4.0 aligns with .golangci.yml version: 2. No changes needed.
miklosbarabas
force-pushed
the
miklos/eng-7608-upgrade-to-go-125
branch
from
5540df9 to
317a0b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/execution.yml (1)
33-36: Upgrade setup-go to v5 in test jobAvoids runner deprecation issues; keep caret for patch updates.
Apply:
- - name: Set up Go ${{ matrix.go }} - uses: actions/setup-go@v4 + - name: Set up Go ${{ matrix.go }} + uses: actions/setup-go@v5 with: go-version: ^${{ matrix.go }} + cache: true
♻️ Duplicate comments (2)
.github/workflows/v2.yml (2)
37-40: actions/setup-go@v4 is deprecated on current runners—upgrade to v5Use v5 to avoid CI breakage; keep caret to pick latest patch automatically.
Apply:
- - name: Set up Go ${{ matrix.go }} - uses: actions/setup-go@v4 + - name: Set up Go ${{ matrix.go }} + uses: actions/setup-go@v5 with: - go-version: ^${{ matrix.go }} + go-version: ^${{ matrix.go }} + cache: true
55-58: Align lint job with test job: upgrade setup-go to v5 and use caret for go-versionPrevents runner incompatibility and auto-pulls latest Go 1.25.x.
Apply:
- - name: Set up Go 1.25 - uses: actions/setup-go@v4 + - name: Set up Go 1.25 + uses: actions/setup-go@v5 with: - go-version: 1.25 + go-version: ^1.25 + cache: true
🧹 Nitpick comments (1)
.golangci.yml (1)
8-15: Enable gci or drop its settings to avoid dead configYou configure linters-settings.gci but the gci linter is disabled. Either enable it or remove the settings.
Apply to enable gci:
linters: enable: # - bodyclose - errcheck - govet - ineffassign - staticcheck + - gci @@ linters-settings: gci: sections: - standard - default - prefix(github.com/wundergraph) - prefix(github.com/wundergraph/graphql-go-tools)Also applies to: 19-26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
examples/federation/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.work
📒 Files selected for processing (7)
.github/workflows/execution.yml(2 hunks).github/workflows/v2.yml(2 hunks).golangci.yml(1 hunks)examples/federation/go.mod(1 hunks)execution/go.mod(1 hunks)go.mod(1 hunks)v2/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- execution/go.mod
- examples/federation/go.mod
- v2/go.mod
- go.mod
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
🪛 actionlint (1.7.7)
.github/workflows/execution.yml
52-52: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/v2.yml
56-56: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (7)
.github/workflows/v2.yml (3)
27-27: LGTM: test matrix bumped to Go 1.25Looks good.
60-64: LGTM: golangci-lint action v8 with linter v2.4.0Nice bump; compatible with Go ≥1.21.
27-27: No stragglers on older Go versions found All go.mod files declarego 1.25, workflows’ matrices andgo-versionfields use1.25or^${{ matrix.go }}(with matrix.go set to1.25), there are no non-1.25 Docker base images, and allactions/setup-gousages are on v5..golangci.yml (2)
1-1: LGTM: config explicitly set to version 2Matches golangci-lint v2.x.
3-7: LGTM: enable goimports and gofmt formattersGood addition for consistent diffs.
.github/workflows/execution.yml (2)
23-24: LGTM: test matrix bumped to Go 1.25Looks good.
56-60: LGTM: golangci-lint action v8 with linter v2.4.0Good upgrade.
miklosbarabas
force-pushed
the
miklos/eng-7608-upgrade-to-go-125
branch
2 times, most recently
from
6a34c0f to
0215d73
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/execution.yml (1)
33-36: Good: setup-go@v5 with caret versionUpgrading to actions/setup-go@v5 and using a caret range for latest patches is correct and future-proof.
.golangci.yml (2)
3-15: Good: enable gci and goimports via formattersAdding gci/goimports (with gci sections) aligns with v2 config and prior feedback.
16-28: Bug: “settings” is nested under linters; must be top-level in v2 schemaIn v2, tool settings live under top-level
settings, notlinters.settings. Staticcheck config will be ignored or fail.Move
settingsto top-level:linters: enable: - errcheck - govet - ineffassign - staticcheck - settings: - staticcheck: - checks: - - all +settings: + staticcheck: + checks: + - all
🧹 Nitpick comments (2)
.github/workflows/execution.yml (1)
33-36: Optional: enable caching for module downloadsCache Go modules to speed up CI.
- name: Set up Go ${{ matrix.go }} uses: actions/setup-go@v5 with: go-version: ^${{ matrix.go }} + cache: true ... - - name: Set up Go 1.25 + - name: Set up Go 1.25 uses: actions/setup-go@v5 with: go-version: ^1.25 + cache: trueAlso applies to: 52-55
.github/workflows/v2.yml (1)
37-40: Optional: enable caching for faster CIEnable module cache in both jobs.
- name: Set up Go ${{ matrix.go }} uses: actions/setup-go@v5 with: go-version: ^${{ matrix.go }} + cache: true ... - - name: Set up Go 1.25 + - name: Set up Go 1.25 uses: actions/setup-go@v5 with: go-version: ^1.25 + cache: trueAlso applies to: 56-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
examples/federation/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.work
📒 Files selected for processing (7)
.github/workflows/execution.yml(2 hunks).github/workflows/v2.yml(2 hunks).golangci.yml(1 hunks)examples/federation/go.mod(1 hunks)execution/go.mod(1 hunks)go.mod(1 hunks)v2/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/federation/go.mod
- go.mod
- execution/go.mod
- v2/go.mod
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
🪛 actionlint (1.7.7)
.github/workflows/v2.yml
55-55: property "go" is not defined in object type {}
(expression)
58-58: property "go" is not defined in object type {}
(expression)
.github/workflows/execution.yml
51-51: property "go" is not defined in object type {}
(expression)
54-54: property "go" is not defined in object type {}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (2)
.golangci.yml (1)
29-36: Bug: “exclusions” is under linters; must be a top-level key
exclusionsshould be top-level in v2. As written, the rules won’t apply.Lift
exclusionsto top-level:- exclusions: - rules: - - linters: - - staticcheck - text: "SA1019: subscription.Message is deprecated: Prefer using TransportClient that is based on byte slices instead of this Message struct." - - linters: - - staticcheck - text: "SA1019: rand\\.Seed.*" +exclusions: + rules: + - linters: [staticcheck] + text: "SA1019: subscription.Message is deprecated: Prefer using TransportClient that is based on byte slices instead of this Message struct." + - linters: [staticcheck] + text: "SA1019: rand\\.Seed.*"Likely an incorrect or invalid review comment.
.github/workflows/v2.yml (1)
37-40: Good: setup-go@v5 with caret in test jobCorrectly upgraded and future-proof.
miklosbarabas
force-pushed
the
miklos/eng-7608-upgrade-to-go-125
branch
from
0215d73 to
e734773
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/execution.yml (1)
35-39: Good: actions upgraded (checkout@v4, setup-go@v5) and caret range usedThis addresses prior deprecation warnings and keeps latest patch within 1.25.
Also applies to: 57-61
.github/workflows/v2.yml (1)
39-43: Good: actions upgraded (checkout@v4, setup-go@v5) and caret range usedMatches the test job and resolves previous warnings.
Also applies to: 61-66
🧹 Nitpick comments (3)
execution/go.mod (1)
6-6: gqlgen version mismatch between require and replaceYou require v0.17.45 but force-replace to v0.17.22. This is confusing and may hide API drift. Align them (either require 0.17.22 or drop replace and move to 0.17.45), then run
go mod tidy.Suggested minimal alignment:
- github.com/99designs/gqlgen v0.17.45 + github.com/99designs/gqlgen v0.17.22Also applies to: 75-76
.github/workflows/execution.yml (1)
37-40: Enable module cache to speed up CI
actions/setup-go@v5supports caching.- name: Set up Go ${{ matrix.go }} uses: actions/setup-go@v5 with: go-version: ^${{ matrix.go }} + cache: true(Apply to both test and lint jobs.)
Also applies to: 59-62
.github/workflows/v2.yml (1)
41-44: Enable module cache to speed up CITurn on caching in both setup-go steps.
- name: Set up Go ${{ matrix.go }} uses: actions/setup-go@v5 with: go-version: ^${{ matrix.go }} + cache: trueAlso applies to: 63-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
examples/federation/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.work
📒 Files selected for processing (7)
.github/workflows/execution.yml(2 hunks).github/workflows/v2.yml(2 hunks).golangci.yml(1 hunks)examples/federation/go.mod(1 hunks)execution/go.mod(1 hunks)go.mod(1 hunks)v2/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/federation/go.mod
- v2/go.mod
- .golangci.yml
- go.mod
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
execution/go.mod
🪛 actionlint (1.7.7)
.github/workflows/execution.yml
27-27: context "env" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
54-54: context "env" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
.github/workflows/v2.yml
31-31: context "env" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
58-58: context "env" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🔇 Additional comments (1)
execution/go.mod (1)
3-3: Go 1.25 directive: LGTMMatches the repo-wide upgrade target.
miklosbarabas
force-pushed
the
miklos/eng-7608-upgrade-to-go-125
branch
from
e734773 to
d04c185
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/execution.yml (1)
27-27: Fix actionlint error: matrix cannot reference env contextUsing
${{ env.GO_VERSION }}inside a matrix is invalid. Replace with a literal orvars.GO_VERSION(define repo var GO_VERSION=1.25).Apply one of:
- go: [ "${{ env.GO_VERSION }}" ] + go: [ "1.25" ]or
- go: [ "${{ env.GO_VERSION }}" ] + go: [ "${{ vars.GO_VERSION }}" ]Also applies to: 54-54
.github/workflows/v2.yml (1)
31-31: Fix actionlint error: matrix cannot reference env contextSame issue here—
${{ env.GO_VERSION }}is not allowed in a matrix. Use a literal orvars.GO_VERSION.- go: [ "${{ env.GO_VERSION }}" ] + go: [ "1.25" ]or
- go: [ "${{ env.GO_VERSION }}" ] + go: [ "${{ vars.GO_VERSION }}" ]Also applies to: 58-58
🧹 Nitpick comments (4)
.github/workflows/execution.yml (2)
37-41: Enable module caching to speed up CIAdd cache to setup-go. Optional but typically cuts minutes off CI.
- name: Set up Go ${{ matrix.go }} uses: actions/setup-go@v5 with: go-version: ^${{ matrix.go }} + cache: trueAlso applies to: 59-62
18-20: Single source of truth for Go versionYou now have
env.GO_VERSION. If you switch matrices tovars.GO_VERSION, consider dropping this env or using it consistently in steps (go-version: ^${{ env.GO_VERSION }}) to avoid drift..github/workflows/v2.yml (2)
41-44: Enable module caching to speed up CIMirror caching in both setup-go steps.
- name: Set up Go ${{ matrix.go }} uses: actions/setup-go@v5 with: go-version: ^${{ matrix.go }} + cache: trueAlso applies to: 63-66
22-24: Consolidate Go version sourceIf you adopt
vars.GO_VERSIONin matrices, consider removing or standardizing the top-levelenv.GO_VERSIONto prevent divergence across workflows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
examples/federation/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.work
📒 Files selected for processing (7)
.github/workflows/execution.yml(2 hunks).github/workflows/v2.yml(2 hunks).golangci.yml(1 hunks)examples/federation/go.mod(1 hunks)execution/go.mod(1 hunks)go.mod(1 hunks)v2/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- go.mod
- examples/federation/go.mod
- execution/go.mod
- v2/go.mod
- .golangci.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
🪛 actionlint (1.7.7)
.github/workflows/v2.yml
31-31: context "env" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
58-58: context "env" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
.github/workflows/execution.yml
27-27: context "env" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
54-54: context "env" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🔇 Additional comments (2)
.github/workflows/execution.yml (1)
35-35: Good upgrades: checkout v4, setup-go v5, golangci-lint action v8 / v2.4.0These align with current runners and toolchain. LGTM.
Also applies to: 57-57, 63-67
.github/workflows/v2.yml (1)
39-39: Upgrades look goodcheckout@v4, setup-go@v5, golangci-lint-action@v8 with golangci-lint v2.4.0 are appropriate. LGTM.
Also applies to: 61-61, 67-71
miklosbarabas
force-pushed
the
miklos/eng-7608-upgrade-to-go-125
branch
from
d04c185 to
b051ce1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
examples/federation/go.mod (1)
3-3: Optional: add a toolchain directive for automatic toolchain selection.If you want devs with Go 1.21+ to auto-download/use Go 1.25 when building this module, add a toolchain directive.
Apply:
go 1.25 +toolchain go1.25.0Note: Pin the exact patch you standardize on.
.github/workflows/v2.yml (3)
35-41: Upgrade to checkout@v4 and setup-go@v5 is correct; enable module cache for speed.Enable the built-in cache to reduce CI time.
Apply:
- name: Set up Go ${{ matrix.go }} uses: actions/setup-go@v5 with: go-version: ^${{ matrix.go }} + cache: true
57-61: Same caching suggestion for lint job.Speed up lints by caching modules.
Apply:
- name: Set up Go ${{ matrix.go }} uses: actions/setup-go@v5 with: - go-version: ^${{ matrix.go }} + go-version: ^${{ matrix.go }} + cache: true
1-21: Optional hardening: restrict default workflow permissions and enable concurrency cancellation.Least-privilege and faster CI on force-pushes.
Add near the top:
name: v2-ci +permissions: + contents: read on:And add a concurrency block:
jobs: + concurrency: + group: v2-ci-${{ github.ref }} + cancel-in-progress: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
examples/federation/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.work
📒 Files selected for processing (7)
.github/workflows/execution.yml(2 hunks).github/workflows/v2.yml(2 hunks).golangci.yml(1 hunks)examples/federation/go.mod(1 hunks)execution/go.mod(1 hunks)go.mod(1 hunks)v2/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (4)
- v2/go.mod
- .golangci.yml
- execution/go.mod
- .github/workflows/execution.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
examples/federation/go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (4)
examples/federation/go.mod (1)
3-3: Go toolchain bump to 1.25 looks consistent.Matches the PR’s objective and aligns with other modules. No issues.
.github/workflows/v2.yml (3)
27-29: Matrix set to Go 1.25 across OSes — good.Clear and explicit. Keeps tests aligned with the PR intent.
52-55: Adding a matrix for the lint job fixes earlier actionlint errors.Good cleanup; avoids undefined matrix keys.
63-67: Verify golangci-lint versions against Go 1.25.Pinned action v8.0.0 and linter v2.4.0 look fine, but please confirm they fully support Go 1.25 features to avoid false positives.
If issues arise, bump the linter to the latest v2.x and keep the action at the latest v8.x:
- uses: golangci/golangci-lint-action@v8.0.0 + uses: golangci/golangci-lint-action@v8 ... - version: v2.4.0 + version: latest
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go (1)
227-230: Fix data race: access client.handlers under lock.This reads client.handlers without synchronization, while other places use handlersMu. Could race under -race.
Apply:
- assert.Eventuallyf(t, func() bool { - return len(client.handlers) == 0 - }, time.Second, time.Millisecond, "client handlers not 0") + assert.Eventuallyf(t, func() bool { + client.handlersMu.Lock() + defer client.handlersMu.Unlock() + return len(client.handlers) == 0 + }, time.Second, time.Millisecond, "client handlers not 0")pkg/testing/federationtesting/gateway/datasource_poller.go (1)
176-181: Nil-pointer risk: set headers only after checking NewRequest error.If http.NewRequestWithContext fails, req is nil and req.Header.Add will panic.
Fix:
- req, err := http.NewRequestWithContext(ctx, http.MethodPost, serviceURL, bytes.NewReader([]byte(ServiceDefinitionQuery))) - req.Header.Add("Content-Type", "application/json") - - if err != nil { + req, err := http.NewRequestWithContext(ctx, http.MethodPost, serviceURL, bytes.NewReader([]byte(ServiceDefinitionQuery))) + if err != nil { return "", fmt.Errorf("create request: %v", err) } + req.Header.Add("Content-Type", "application/json")
♻️ Duplicate comments (1)
.golangci.yml (1)
3-7: gci enabled first: acknowledged and correct.Matches prior review ask to add gci and place it first. (golangci-lint.run)
🧹 Nitpick comments (13)
pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go (5)
286-288: Avoid blocking in Eventually callback.This blocks on <-serverDone; prefer non-blocking select to avoid hanging the polling loop.
Apply:
- assert.Eventuallyf(t, func() bool { - <-serverDone - return true - }, time.Second, time.Millisecond*10, "server did not close") + assert.Eventuallyf(t, func() bool { + select { + case <-serverDone: + return true + default: + return false + } + }, time.Second, time.Millisecond*10, "server did not close")
154-154: 1ms read timeout is brittle; bump to reduce flakiness.1ms is extremely tight and can cause intermittent failures on CI and across Go scheduler changes.
Apply:
- WithReadTimeout(time.Millisecond), + WithReadTimeout(50 * time.Millisecond),Also applies to: 211-211, 267-267
123-123: Use dynamic expected length.Hardcoding 4 ties the helper to this test only.
Apply:
- assert.Len(t, receivedIDs, 4) + assert.Len(t, receivedIDs, len(subscriptionID))
61-61: Compare JSON semantically, not as raw string.Avoid ordering/whitespace brittleness.
Apply:
- assert.Equal(t, tt.want, string(got)) + require.JSONEq(t, tt.want, string(got))
209-209: Remove redundant serverCancel call.You already defer serverCancel() at Line 209; calling it again at Line 226 is unnecessary.
Apply:
- serverCancel()Also applies to: 226-226
.golangci.yml (2)
8-15: Align goimports with gci to avoid formatter churn.Add goimports local-prefixes to match gci prefixes and optionally expand gci sections to include common groups (blank/dot/alias/localmodule) for stable grouping. (golangci-lint.run)
formatters: enable: - gci - goimports - gofmt settings: gci: sections: - standard - default - prefix(github.com/wundergraph) - prefix(github.com/wundergraph/graphql-go-tools) + - blank + - dot + - alias + - localmodule + goimports: + local-prefixes: + - github.com/wundergraph + - github.com/wundergraph/graphql-go-tools
30-37: Exclusion rules: tighten match to reduce accidental silencing.Anchor the regex to the start and shorten to stable fragments to avoid breakage if messages change; keep scope limited to staticcheck. The v2
linters.exclusions.rulesusage is correct. (golangci-lint.run)exclusions: rules: - linters: - staticcheck - text: "SA1019: subscription.Message is deprecated: Prefer using TransportClient that is based on byte slices instead of this Message struct." + text: "^SA1019: subscription\\.Message is deprecated" - linters: - staticcheck - text: "SA1019: rand\\.Seed.*" + text: "^SA1019: rand\\.Seed"pkg/subscription/websocket/client_test.go (1)
111-114: Simplify the defer: assert directly without conditional.The if err != nil guard is redundant; assert.NoError already handles nil.
Apply:
- defer func(connToClient net.Conn) { - err := connToClient.Close() - assert.NoError(t, err) - }(connToClient) + defer func() { assert.NoError(t, connToClient.Close()) }()pkg/testing/federationtesting/gateway/datasource_poller.go (1)
188-193: Don’t assign to outer err in defer; use a dedicated variable.Assigning to err here is confusing and has no effect on the returned error. Prefer a local variable.
Apply:
- defer func(Body io.ReadCloser) { - err = Body.Close() - if err != nil { - log.Printf("Failed to close response body: %s\n", err) - } - }(resp.Body) + defer func(Body io.ReadCloser) { + if cerr := Body.Close(); cerr != nil { + log.Printf("Failed to close response body: %s\n", cerr) + } + }(resp.Body)pkg/testing/federationtesting/graphql_client_test.go (1)
106-111: Streamline Close() assertion in defer.Remove the conditional and assert directly; also no need to pass conn as a parameter.
Apply:
- defer func(conn net.Conn) { - err := conn.Close() - if err != nil { - assert.NoError(t, err) - } - }(conn) + defer func() { assert.NoError(t, conn.Close()) }()pkg/imports/graphql_file.go (2)
55-57: Defer assigns to err but the function doesn’t use a named return; Close() errors are silently droppedMake the return value named and only set it if no earlier error occurred.
-func (g GraphQLFile) renderSelf(printFilePath bool, out io.Writer) error { +func (g GraphQLFile) renderSelf(printFilePath bool, out io.Writer) (err error) { file, err := os.Open(g.RelativePath) if err != nil { return err } - defer func(file *os.File) { - err = file.Close() - }(file) + defer func() { + if cerr := file.Close(); err == nil { + err = cerr + } + }()Also applies to: 50-54
68-71: Don’t swallow non-EOF read errorsBreak only on io.EOF; return other errors to the caller.
- line, _, err := reader.ReadLine() - if err != nil { - break - } + line, _, rerr := reader.ReadLine() + if rerr != nil { + if errors.Is(rerr, io.EOF) { + break + } + return rerr + }Add:
import "errors"pkg/execution/planning.go (1)
408-411: Close() error is written to a local err that’s not part of the return; no effect and risks clobbering later usageThis function doesn’t return an error, so just acknowledge Close() explicitly to satisfy linters without touching other state.
- defer func(reader *os.File) { - err = reader.Close() - }(reader) + defer func() { + _ = reader.Close() // best-effort close; nothing to return from here + }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (29)
.golangci.yml(1 hunks)pkg/ast/ast_node_kind_test.go(1 hunks)pkg/ast/ast_root_operation_type_definition.go(1 hunks)pkg/astnormalization/inject_input_default_values.go(1 hunks)pkg/astnormalization/inject_input_default_values_test.go(1 hunks)pkg/astparser/parser_test.go(1 hunks)pkg/astvalidation/operation_rule_fragments.go(1 hunks)pkg/astvalidation/rule_populated_type_bodies.go(1 hunks)pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go(1 hunks)pkg/engine/datasource/graphql_datasource/graphql_tws_handler_test.go(1 hunks)pkg/engine/datasource/graphql_datasource/graphql_ws_handler_test.go(1 hunks)pkg/engine/datasource/httpclient/httpclient_test.go(1 hunks)pkg/engine/datasource/httpclient/nethttpclient.go(1 hunks)pkg/execution/datasource_pipeline_test.go(1 hunks)pkg/execution/execution_test.go(2 hunks)pkg/execution/planning.go(1 hunks)pkg/federation/schema.go(1 hunks)pkg/federation/schema_test.go(1 hunks)pkg/federation/sdlmerge/sdlmerge.go(1 hunks)pkg/graphql/execution_engine_v2.go(1 hunks)pkg/graphql/execution_engine_v2_norace_test.go(1 hunks)pkg/http/http.go(1 hunks)pkg/imports/graphql_file.go(1 hunks)pkg/operationreport/operationreport_test.go(1 hunks)pkg/subscription/websocket/client_test.go(1 hunks)pkg/testing/federationtesting/federation_integration_test.go(1 hunks)pkg/testing/federationtesting/gateway/datasource_poller.go(1 hunks)pkg/testing/federationtesting/gateway/main.go(1 hunks)pkg/testing/federationtesting/graphql_client_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (18)
- pkg/testing/federationtesting/gateway/main.go
- pkg/astvalidation/rule_populated_type_bodies.go
- pkg/federation/schema.go
- pkg/ast/ast_root_operation_type_definition.go
- pkg/astnormalization/inject_input_default_values_test.go
- pkg/ast/ast_node_kind_test.go
- pkg/engine/datasource/graphql_datasource/graphql_tws_handler_test.go
- pkg/astnormalization/inject_input_default_values.go
- pkg/graphql/execution_engine_v2.go
- pkg/astvalidation/operation_rule_fragments.go
- pkg/operationreport/operationreport_test.go
- pkg/http/http.go
- pkg/federation/sdlmerge/sdlmerge.go
- pkg/astparser/parser_test.go
- pkg/testing/federationtesting/federation_integration_test.go
- pkg/federation/schema_test.go
- pkg/graphql/execution_engine_v2_norace_test.go
- pkg/engine/datasource/graphql_datasource/graphql_ws_handler_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
🪛 GitHub Check: Linters (1.25)
pkg/engine/datasource/httpclient/httpclient_test.go
[failure] 187-187:
Error return value of gzipWriter.Close is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (8)
pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go (1)
16-18: LGTM: import order is consistent.Import grouping/order looks good and aligns with common goimports/gci conventions.
.golangci.yml (3)
1-1: Config v2: good upgrade.
version: '2'matches the current schema and unlocks the newformatters/linters.settings/linters.exclusionslayout. (golangci-lint.run)
16-21: Linter set: confirm intentional exclusions.
unusedandgosecare commented out. Confirm this is intentional for Go 1.25 and repo policies; otherwise consider enabling with targeted exclusions. (golangci-lint.run)
24-29: Staticcheck checks: verify identifiers.Please double-check that
checks: [default, standard]are valid selectors for golangci-lint’s staticcheck integration. If not, preferchecks: ["all"](and subtract specific checks if needed). Staticcheck’s native config uses sets like"all"with optional negations. (staticcheck.dev)settings: staticcheck: checks: - - default - - standard + - all # - '-SA1019'pkg/execution/execution_test.go (2)
2007-2012: LGTM: surfacing Close() errors in handler.Passing r.Body into the deferred closure avoids capture pitfalls; failing the test on Close error is fine here.
2504-2509: LGTM: assert on Body.Close() error.Consistent with the repo-wide pattern; good to fail loudly on cleanup errors.
pkg/execution/datasource_pipeline_test.go (1)
22-27: LGTM: check Close() error on config file.Good defensive cleanup in tests; failing fast on Close errors is appropriate.
pkg/engine/datasource/httpclient/httpclient_test.go (1)
186-189: LGTM: Close() error is asserted in the deferred cleanupThis addresses the errcheck finding and makes the test more robust.
|
@miklosbarabas I was slowly working on the upgrading linter a few week back and forgot to create PR for it (So it is here now #1290) Also I would say that upgrading linter and fixing its errors might be done separately from upgrade to 1.25 |
- fixed issues found by latest golangci-lint
This reverts commit 38c6a8e.
devsergiy
force-pushed
the
miklos/eng-7608-upgrade-to-go-125
branch
from
ab2d37f to
2aa76e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go.mod (1)
3-3: Optional: add a toolchain directive for reproducible buildsIf you want all devs/CI to auto-use the exact Go toolchain, add a toolchain pin (introduced in Go 1.21). Skip if you prefer flexibility.
module github.com/wundergraph/graphql-go-tools -go 1.25 +go 1.25 +toolchain go1.25.0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
examples/federation/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.work
📒 Files selected for processing (6)
.github/workflows/execution.yml(2 hunks).github/workflows/v2.yml(2 hunks)examples/federation/go.mod(1 hunks)execution/go.mod(1 hunks)go.mod(1 hunks)v2/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- execution/go.mod
- v2/go.mod
- examples/federation/go.mod
- .github/workflows/v2.yml
- .github/workflows/execution.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (3)
go.mod (3)
3-3: Go 1.25 module directive LGTMThe version bump to
go 1.25is correct and uses the preferred major.minor form.
3-3: Rungo mod tidy -go=1.25in each module root
Normalizego.sumfor Go 1.25’s resolver to prevent CI drift—run locally and commit any changes.
3-3: [*]
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.225](v2.0.0-rc.224...v2.0.0-rc.225) (2025-09-09) ### Features * add support of field selection reasons extensions ([#1282](#1282)) ([37c9582](37c9582)) * upgrade all components to go 1.25 ([#1289](#1289)) ([6bd2713](6bd2713)) ### Bug Fixes * refactor CoordinateDependencies, FetchReasons ([#1293](#1293)) ([cfebc16](cfebc16)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for field selection reasons extensions. * Upgraded all components to Go 1.25 for improved compatibility and performance. * **Chores** * Bumped release version to 2.0.0-rc.225. * **Documentation** * Updated changelog with a new 2.0.0-rc.225 entry detailing recent features. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Summary by CodeRabbit
Chores
Tests
Checklist