chore: upgrade golangci-lint to v2#1290
Conversation
Fix the remaining issues from the linter and use upgraded config for v2. Suppress deprecated error messages.
WalkthroughUpdated CI workflows and golangci-lint config; added lint targets in Makefiles; closed HTTP response bodies in client and tests; added WebSocket staticcheck suppressions and a private WS message helper; refactored two loops to explicit condition-based forms; many files had import/order or formatting-only edits. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client.go (3)
908-915: Mirror TLS unwrap on client unsubscribe before Remove.- _ = c.netPoll.Remove(conn.netConn) + netConn := conn.netConn + if tlsConn, ok := netConn.(*tls.Conn); ok { + netConn = tlsConn.NetConn() + } + _ = c.netPoll.Remove(netConn)
925-933: Mirror TLS unwrap on server unsubscribe before Remove.- _ = c.netPoll.Remove(conn.netConn) + netConn := conn.netConn + if tlsConn, ok := netConn.(*tls.Conn); ok { + netConn = tlsConn.NetConn() + } + _ = c.netPoll.Remove(netConn)
859-863: Unwrap TLS before calling netPoll.Remove in every code path
Use the same underlying net.Conn that was passed to Add—i.e. unwrap any *tls.Conn—when removing from the poller. This must be applied not only in close() but also in the client-close and server-unsubscribe handlers:
- In close(): replace
with_ = c.netPoll.Remove(conn.netConn)netConn := conn.netConn if tlsConn, ok := netConn.(*tls.Conn); ok { netConn = tlsConn.NetConn() } _ = c.netPoll.Remove(netConn)- In the client-close path (after deleting triggers): unwrap before c.netPoll.Remove(conn.netConn)
- In handleServerUnsubscribe: unwrap before c.netPoll.Remove(conn.netConn)
Consider extracting a helper to DRY the unwrap+Remove logic.
♻️ Duplicate comments (1)
.github/workflows/v2.yml (1)
63-63: Same as execution workflow: verify tag; optional action upgrades.Mirror the verification and optional upgrades suggested in execution.yml.
🧹 Nitpick comments (7)
.github/workflows/execution.yml (1)
59-59: Confirm golangci-lint v2.3.1 & version input support
- v2.3.1 tag exists in the golangci-lint repo (golangci-lint.run, releasealert.dev).
- The action’s
versioninput has been supported since v1 and is available in v3 (pkg.go.dev, github.com).Optional: upgrade for Node20/runtime parity
- actions/checkout → v4 (github.com)
- actions/setup-go → v5 (github.com)
- golangci/golangci-lint-action → v5 (github.com)
v2/pkg/astparser/parser.go (1)
373-373: LGTM: loop now clearly bounded by next token '@'.Semantics unchanged and readability improved.
If you prefer consistency with surrounding code, you could use
peekEquals(keyword.AT)here as elsewhere, though current form is fine.execution/engine/execution_engine_test.go (1)
96-103: Also close gzip/deflate readers to avoid leaks.Great addition closing response bodies. Please close the decompressor readers too.
@@ - reader, err := gzip.NewReader(response.Body) + reader, err := gzip.NewReader(response.Body) require.NoError(t, err) + defer reader.Close() @@ - reader := flate.NewReader(response.Body) + reader := flate.NewReader(response.Body) + defer reader.Close()Also applies to: 117-129, 135-145
v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client.go (1)
701-741: Stop the ack wait timer to prevent a timer leak.
time.NewTimershould be stopped on all non-timeout exits.func waitForAck(conn net.Conn, readTimeout, writeTimeout time.Duration) error { - timer := time.NewTimer(ackWaitTimeout) + timer := time.NewTimer(ackWaitTimeout) + defer timer.Stop()v2/Makefile (1)
28-31: Declare lint as a phony target and recurse explicitly.Minor make hygiene and consistent recursion.
+.PHONY: lint lint: - golangci-lint run - cd ../execution && golangci-lint run + golangci-lint run ./... + cd ../execution && golangci-lint run ./...execution/engine/graphql_client_test.go (1)
77-88: Use expectedStatusCode in assertion.The helper ignores the parameter.
func (g *GraphqlClient) QueryStatusCode(ctx context.Context, addr, queryFilePath string, variables queryVariables, expectedStatusCode int, t *testing.T) []byte { @@ - responseBodyBytes, err := io.ReadAll(resp.Body) + responseBodyBytes, err := io.ReadAll(resp.Body) require.NoError(t, err) + assert.Equal(t, expectedStatusCode, resp.StatusCode) return responseBodyBytes }.golangci.yml (1)
29-39: Either enable gci formatter or drop its settings.Settings under formatters.gci are inert unless gci is enabled.
formatters: enable: - # - gci + - gci - gofmt
📜 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 (10)
.github/workflows/execution.yml(1 hunks).github/workflows/v2.yml(1 hunks).golangci.yml(1 hunks)execution/engine/execution_engine_test.go(3 hunks)execution/engine/graphql_client_test.go(5 hunks)execution/federationtesting/gateway/http/ws.go(2 hunks)execution/subscription/legacy_handler_test.go(1 hunks)v2/Makefile(1 hunks)v2/pkg/astparser/parser.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client.go(1 hunks)
⏰ 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.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (3)
execution/subscription/legacy_handler_test.go (1)
693-693: LGTM: response body is now closed.Good test hygiene; avoids leaking idle TCP connections in CI.
v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client.go (1)
832-849: Loop termination made explicit — LGTM.Switching to
for waitForEvents > 0reads clearer and preserves semantics.execution/engine/graphql_client_test.go (1)
61-75: LGTM on closing response bodies.Good fix; prevents leaks flagged by bodyclose.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/execution.yml (2)
56-60: Upgrade is correct; align lint job actions + enable module cache.The golangci-lint action v8 with golangci-lint v2.3.1 is good. Please also bump checkout/setup-go in the lint job and enable module caching for speed.
- - name: Check out code into the Go module directory - uses: actions/checkout@v3 + - name: Check out code into the Go module directory + uses: actions/checkout@v4 @@ - - name: Set up Go 1.23 - uses: actions/setup-go@v4 - with: - go-version: 1.23 + - name: Set up Go 1.23 + uses: actions/setup-go@v5 + with: + go-version: 1.23 + cache: true
1-2: Harden workflows: least-privilege permissions and run cancellation.Optional but recommended for CI security and cost.
name: execution +permissions: + contents: read + pull-requests: read on: @@ jobs: + # Cancel superseded runs for the same ref + # (use a top-level 'concurrency' block if you want to cancel across all jobs) +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true.github/workflows/v2.yml (2)
60-64: Good: golangci-lint action v8 with v2.3.1; align other actions + cache.Mirror the test job’s majors in the lint job and cache modules.
- - name: Check out code into the Go module directory - uses: actions/checkout@v3 + - name: Check out code into the Go module directory + uses: actions/checkout@v4 @@ - - name: Set up Go 1.23 - uses: actions/setup-go@v4 - with: - go-version: 1.23 + - name: Set up Go 1.23 + uses: actions/setup-go@v5 + with: + go-version: 1.23 + cache: true
1-2: Apply least-privilege and cancel superseded runs.Same rationale as the execution workflow.
name: v2-ci +permissions: + contents: read + pull-requests: read on: @@ jobs: +concurrency: + group: ${{ github.workflow }}-${{ 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 selected for processing (2)
.github/workflows/execution.yml(2 hunks).github/workflows/v2.yml(2 hunks)
⏰ 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.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (2)
.github/workflows/execution.yml (1)
31-33: Good: Actions bumped to latest majors in test job.Checkout v4 and setup-go v5 look right and unblock Node20 deprecation warnings.
.github/workflows/v2.yml (1)
35-38: Good: Test job uses checkout v4 and setup-go v5.Matches current best practice and Go toolcache behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.golangci.yml (3)
3-11: Consider enabling a few low-noise, high-signal linters.The minimal set is fine; optionally add errorlint for robust error handling.
Apply:
enable: - errcheck - govet - ineffassign - staticcheck - bodyclose + - errorlint
29-39: GCI formatter config: simplify prefixes and consider local-prefixes.You likely don’t need both specific and parent prefixes; one parent prefix is sufficient.
Apply:
sections: - standard - default - - prefix(github.com/wundergraph) - - prefix(github.com/wundergraph/graphql-go-tools) + - prefix(github.com/wundergraph)Optionally add local-prefixes to future-proof:
sections: - standard - default - prefix(github.com/wundergraph) + local-prefixes: github.com/wundergraphIf you intend auto-fixing in CI, ensure the workflow runs golangci-lint with --fix so formatters can rewrite files.
14-28: Tighten staticcheck exclusion to QF1008 only
- Scope the path-based rule so it suppresses only the “QF1008:” code, avoiding blanket silencing of future staticcheck findings in pkg/astvalidation/reference.
rules: - linters: - staticcheck text: "QF1008:" - - path: pkg/astvalidation/reference - linters: - - staticcheck + - path: pkg/astvalidation/reference + linters: + - staticcheck + text: "QF1008:"
📜 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 (1)
.golangci.yml(1 hunks)
⏰ 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.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (1)
.golangci.yml (1)
1-1: Confirm golangci-lint CLI v2 on CI and locally
Workflows at.github/workflows/execution.yml:56andv2.yml:60both usegolangci/golangci-lint-action@v8.0.0. Ensure that this action (or an explicitversion: "v2") runs golangci-lint CLI v2 so yourversion: "2"config is honored, and update your local dev environment to use CLI v2 as well.
…and-enable-staticcheck
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
v2/pkg/engine/datasource/graphql_datasource/graphql_sse_handler.go (5)
72-85: Fix potential hang: send failure to errCh when subscription request failsCurrently on request error you call updater.Update and return, leaving StartBlocking waiting forever on errCh.
Apply:
func (h *gqlSSEConnectionHandler) subscribe(dataCh, errCh chan []byte) { resp, err := h.performSubscriptionRequest() if err != nil { h.log.Error("failed to perform subscription request", log.Error(err)) - if h.requestContext.Err() != nil { - // request context was canceled do not send an error as channel will be closed - return - } - - h.updater.Update([]byte(internalError)) - - return + // Respect cancellations; otherwise signal the loop to exit. + select { + case <-h.requestContext.Done(): + return + case <-h.engineContext.Done(): + return + case errCh <- []byte(internalError): + return + } }
144-147: Colon-line check uses msg instead of lineThis prevents proper handling of SSE comment lines (": ...").
Apply:
- case bytes.HasPrefix(msg, []byte(":")): + case bytes.HasPrefix(line, []byte(":")): // according to the spec, we ignore messages starting with a colon continue
233-239: Close/flush body on non-200 to avoid connection leakNon-OK responses return without closing the body, wasting connections and fds.
Apply:
switch resp.StatusCode { case http.StatusOK: return resp, nil default: - return nil, fmt.Errorf("failed to connect to stream unexpected resp status code: %d", resp.StatusCode) + // drain & close to allow connection reuse and avoid leaks + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + return nil, fmt.Errorf("failed to connect to stream unexpected resp status code: %d", resp.StatusCode) }
90-90: Avoid unbounded SSE buffer (math.MaxInt) — cap to a sane limitAn unbounded buffer risks memory bloat/DoS. Use a configurable or conservative cap.
Apply:
- reader := sse.NewEventStreamReader(resp.Body, math.MaxInt) + reader := sse.NewEventStreamReader(resp.Body, defaultSSEBufferSize)Add near the top:
const defaultSSEBufferSize = 1 << 20 // 1 MiB; consider making this configurable
254-258: Don’t ignore JSON marshal errors for variables/extensionsSilent failures mask malformed inputs and lead to confusing behavior.
Apply:
if h.options.Body.Variables != nil { - variables, _ := h.options.Body.Variables.MarshalJSON() - - query.Add("variables", string(variables)) + varsJSON, err := h.options.Body.Variables.MarshalJSON() + if err != nil { + return nil, err + } + query.Add("variables", string(varsJSON)) } if h.options.Body.Extensions != nil { - extensions, _ := h.options.Body.Extensions.MarshalJSON() - - query.Add("extensions", string(extensions)) + extJSON, err := h.options.Body.Extensions.MarshalJSON() + if err != nil { + return nil, err + } + query.Add("extensions", string(extJSON)) }Also applies to: 264-268
🧹 Nitpick comments (5)
v2/pkg/grpctest/cmd/main.go (1)
43-47: Bind test server to loopback and log the actual address.Prevents exposing a test port on all interfaces and makes the log reflect the real listener address.
- l, err := net.Listen("tcp", ":9009") + l, err := net.Listen("tcp", "127.0.0.1:9009")- log.Printf("Starting gRPC server on port 9009") + log.Printf("Starting gRPC server on %s", l.Addr())Also applies to: 54-55
execution/Makefile (2)
18-19: Switch to golangci-lint fmt looks good; make config path robust.Hard-code of
../.golangci.ymlcan break if invoked viamake -Cfrom other dirs. Prefer a resolved repo-root path variable.Use a config var (add near the top of the Makefile):
MAKEFILE_DIR := $(dir $(abspath $(lastword $(MAKEFILE_LIST)))) REPO_ROOT := $(abspath $(MAKEFILE_DIR)/..) GOLANGCI_CONFIG := $(REPO_ROOT)/.golangci.ymlThen update this recipe:
- golangci-lint fmt --config ../.golangci.yml + golangci-lint fmt --config $(GOLANGCI_CONFIG)
20-21: Add .PHONY for lint and reuse the config var.
lintshould be phony, and reusing the config var keeps parity withformat.+ .PHONY: lint -lint: - golangci-lint run --config ../.golangci.yml . +lint: + golangci-lint run --config $(GOLANGCI_CONFIG) .Optional: add
--timeout=5m(or whatever CI uses) to avoid sporadic CI timeouts.v2/pkg/engine/jsonschema/variables_schema_test.go (1)
496-496: Fix copy-paste in failure messageThis parses the schema, but the message says “operation parsing failed”.
- require.False(t, report1.HasErrors(), "operation parsing failed") + require.False(t, report1.HasErrors(), "schema parsing failed")v2/pkg/engine/datasource/graphql_datasource/graphql_sse_handler.go (1)
298-301: Connection header on HTTP/2 is superfluousSetting “Connection: keep-alive” is ignored on HTTP/2 and can be omitted.
Consider removing it, or only setting for HTTP/1.1 if you detect/provide a knob.
📜 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 (69)
execution/Makefile(1 hunks)execution/engine/config_factory_federation.go(1 hunks)execution/engine/config_factory_federation_test.go(1 hunks)execution/engine/engine_config.go(1 hunks)execution/engine/execution_engine.go(1 hunks)execution/engine/execution_engine_grpc_test.go(1 hunks)execution/engine/execution_engine_test.go(3 hunks)v2/Makefile(1 hunks)v2/pkg/astminify/minify.go(1 hunks)v2/pkg/astminify/minify_test.go(1 hunks)v2/pkg/astnormalization/input_coercion_for_list.go(1 hunks)v2/pkg/astnormalization/input_coercion_for_list_test.go(1 hunks)v2/pkg/astnormalization/uploads/upload_finder_test.go(1 hunks)v2/pkg/astnormalization/variables_unused_deletion_test.go(1 hunks)v2/pkg/astvalidation/operation_rule_values.go(1 hunks)v2/pkg/astvalidation/operation_validation_test.go(1 hunks)v2/pkg/engine/argument_templates/argument_templates.go(1 hunks)v2/pkg/engine/argument_templates/argument_templates_test.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_sse_handler.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_tws_handler.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_tws_handler_test.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_ws_handler.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_ws_handler_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/compiler.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/compiler_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_composite_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/json_builder.go(1 hunks)v2/pkg/engine/datasource/introspection_datasource/planner_test.go(1 hunks)v2/pkg/engine/datasource/introspection_datasource/source.go(0 hunks)v2/pkg/engine/datasource/pubsub_datasource/kafka_event_manager.go(1 hunks)v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource_test.go(1 hunks)v2/pkg/engine/datasourcetesting/datasourcetesting.go(1 hunks)v2/pkg/engine/jsonschema/variables_schema_test.go(1 hunks)v2/pkg/engine/plan/analyze_plan_kind_test.go(1 hunks)v2/pkg/engine/plan/datasource_filter_visitor_test.go(1 hunks)v2/pkg/engine/plan/planner_test.go(1 hunks)v2/pkg/engine/postprocess/add_missing_nested_dependencies_test.go(1 hunks)v2/pkg/engine/postprocess/create_parallel_nodes_test.go(1 hunks)v2/pkg/engine/postprocess/deduplicate_single_fetches_test.go(1 hunks)v2/pkg/engine/postprocess/postprocess_test.go(1 hunks)v2/pkg/engine/resolve/authorization_test.go(1 hunks)v2/pkg/engine/resolve/fetchtree_test.go(1 hunks)v2/pkg/engine/resolve/inputtemplate_test.go(1 hunks)v2/pkg/engine/resolve/loader.go(1 hunks)v2/pkg/engine/resolve/loader_hooks_test.go(1 hunks)v2/pkg/engine/resolve/loader_test.go(1 hunks)v2/pkg/engine/resolve/resolvable.go(1 hunks)v2/pkg/engine/resolve/resolvable_custom_field_renderer_test.go(1 hunks)v2/pkg/engine/resolve/resolve_test.go(1 hunks)v2/pkg/engine/resolve/response.go(1 hunks)v2/pkg/engine/resolve/subscription_filter.go(1 hunks)v2/pkg/engine/resolve/subscription_filter_test.go(1 hunks)v2/pkg/engine/resolve/variables_renderer.go(1 hunks)v2/pkg/fastjsonext/fastjsonext_test.go(1 hunks)v2/pkg/grpctest/cmd/main.go(1 hunks)v2/pkg/grpctest/mockservice.go(1 hunks)v2/pkg/grpctest/plugin/plugin_service.go(1 hunks)v2/pkg/grpctest/schema.go(1 hunks)v2/pkg/netpoll/netpoll.go(1 hunks)v2/pkg/variablesvalidation/variablesvalidation.go(1 hunks)v2/pkg/variablesvalidation/variablesvalidation_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- v2/pkg/engine/datasource/introspection_datasource/source.go
✅ Files skipped from review due to trivial changes (59)
- v2/pkg/astminify/minify.go
- v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go
- v2/pkg/engine/postprocess/create_parallel_nodes_test.go
- v2/pkg/astnormalization/input_coercion_for_list_test.go
- v2/pkg/engine/resolve/resolvable.go
- v2/pkg/grpctest/schema.go
- v2/pkg/engine/resolve/inputtemplate_test.go
- v2/pkg/engine/resolve/loader_test.go
- v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
- v2/pkg/engine/resolve/authorization_test.go
- v2/pkg/engine/datasource/grpc_datasource/compiler.go
- v2/pkg/engine/argument_templates/argument_templates_test.go
- v2/pkg/engine/postprocess/deduplicate_single_fetches_test.go
- v2/pkg/astminify/minify_test.go
- v2/pkg/astnormalization/variables_unused_deletion_test.go
- v2/pkg/engine/resolve/subscription_filter_test.go
- v2/pkg/engine/plan/analyze_plan_kind_test.go
- v2/pkg/engine/resolve/response.go
- v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
- v2/pkg/engine/datasource/grpc_datasource/json_builder.go
- v2/pkg/fastjsonext/fastjsonext_test.go
- v2/pkg/engine/resolve/resolvable_custom_field_renderer_test.go
- v2/pkg/engine/plan/datasource_filter_visitor_test.go
- v2/pkg/netpoll/netpoll.go
- v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource_test.go
- v2/pkg/engine/resolve/subscription_filter.go
- v2/pkg/engine/resolve/fetchtree_test.go
- v2/pkg/engine/datasource/introspection_datasource/planner_test.go
- v2/pkg/engine/datasource/pubsub_datasource/kafka_event_manager.go
- v2/pkg/astnormalization/input_coercion_for_list.go
- v2/pkg/engine/postprocess/add_missing_nested_dependencies_test.go
- execution/engine/config_factory_federation.go
- v2/pkg/engine/datasource/grpc_datasource/execution_plan_composite_test.go
- execution/engine/execution_engine.go
- v2/pkg/grpctest/plugin/plugin_service.go
- v2/pkg/engine/argument_templates/argument_templates.go
- v2/pkg/grpctest/mockservice.go
- v2/pkg/engine/resolve/loader_hooks_test.go
- v2/pkg/astnormalization/uploads/upload_finder_test.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_tws_handler_test.go
- execution/engine/config_factory_federation_test.go
- execution/engine/execution_engine_grpc_test.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go
- v2/pkg/engine/resolve/variables_renderer.go
- v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go
- v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
- v2/pkg/astvalidation/operation_rule_values.go
- v2/pkg/engine/postprocess/postprocess_test.go
- v2/pkg/variablesvalidation/variablesvalidation.go
- v2/pkg/engine/datasource/grpc_datasource/compiler_test.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_ws_handler_test.go
- v2/pkg/engine/resolve/loader.go
- v2/pkg/engine/plan/planner_test.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_ws_handler.go
- v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_tws_handler.go
- v2/pkg/variablesvalidation/variablesvalidation_test.go
- v2/pkg/engine/datasourcetesting/datasourcetesting.go
🚧 Files skipped from review as they are similar to previous changes (2)
- execution/engine/execution_engine_test.go
- v2/Makefile
🧰 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:
v2/pkg/astvalidation/operation_validation_test.gov2/pkg/engine/resolve/resolve_test.go
🔇 Additional comments (9)
execution/engine/engine_config.go (1)
8-9: Import reordering LGTMgrpc is grouped correctly between stdlib and local imports, duplicate removal avoids lint noise. No behavioral changes.
v2/pkg/engine/resolve/resolve_test.go (1)
20-20: LGTM: import grouping/blank line is consistent with the new linter config.Pure formatting; no functional impact.
v2/pkg/astvalidation/operation_validation_test.go (2)
10-10: Import reorder for apollocompatibility looks goodOrder matches goimports/golangci-lint v2 expectations, and the import is used via WithApolloCompatibilityFlags below.
15-15: Import relocate for errorcodes is correctNeeded for GraphQLValidationFailed usage; placement within the module import block is fine.
v2/pkg/grpctest/cmd/main.go (1)
17-18: Imports normalized; looks good.grpc is now imported once and grouped correctly (std → third‑party → local). No functional change.
execution/Makefile (1)
11-14: Update Makefile comment to reflect renamed target
- In execution/Makefile at line 13, change the comment to reference
update-fixturesinstead ofupdateTestFixtures:-# updateTestFixtures will update all! golden fixtures +# update-fixtures will update all! golden fixturesLikely an incorrect or invalid review comment.
v2/pkg/engine/jsonschema/variables_schema_test.go (1)
9-11: LGTM: added astparser import is correctThe import grouping/order looks good, and the package path matches the v2 module. No further changes needed.
v2/pkg/engine/datasource/graphql_datasource/graphql_sse_handler.go (2)
15-15: Import path bump verified
go.mod requires github.com/r3labs/sse/v2 v2.8.1 and no lingering v1 imports were found.
15-15: Toolchain compatibility verifiedGo module specifies Go 1.23.0 (≥1.17) so math.MaxInt is supported, and sse/v2@v2.8.1 is required and correctly imported. No further action needed.
Fix the remaining issues from the linter and use
upgraded config for v2.
Suppress deprecated error messages.
This is something I worked between tasks a few weeks ago.
The code fixes are legit, but I did not look closely into github workflows.
Summary by CodeRabbit
Checklist