Revert "[relay] Update GO version and QUIC version"#5055
Conversation
This reverts commit 8722b79.
📝 WalkthroughWalkthroughThis PR downgrades the Go toolchain version from 1.25 to 1.24.10 across CI workflows, development container, and module configuration. It also downgrades several system packages and Go dependencies (quic-go, go.uber.org/mock). Additionally, the PR adds legacy Go build tag syntax for backward compatibility, removes nolint directives, refactors code patterns (switch to if-else statements), and introduces public API changes to QUIC connection types. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In @.github/workflows/golangci-lint.yml:
- Line 55: The workflow uses an outdated action reference
"golangci/golangci-lint-action@v4"; update that usage to a maintained release
(e.g., "golangci/golangci-lint-action@v9" or a pinned tag like "@v9.0.0") in the
workflow step that currently specifies uses: golangci/golangci-lint-action@v4 so
the action pulls a current, supported version and avoids compatibility issues.
In @.github/workflows/release.yml:
- Line 66: The GO_TARBALL value is pinned to an outdated release; update the
tarball name used in the workflow by replacing the GO_TARBALL assignment (symbol
GO_TARBALL) from "go1.24.10.freebsd-amd64.tar.gz" to
"go1.24.11.freebsd-amd64.tar.gz" so the workflow pulls the patched Go 1.24.11
binary.
In @client/cmd/testutil_test.go:
- Around line 91-94: The snippet creates eventStore :=
&activity.InMemoryEventStore{} and then checks an unrelated err variable that is
already handled earlier by NewTestStoreFromSQL/t.Fatal, making the if err != nil
branch dead and dangerous; remove the erroneous error-check block (the if err !=
nil { return nil, nil } lines) and ensure callers rely on the actual returned
values (or return eventStore appropriately) so no nil is returned—look for
symbol eventStore and the earlier NewTestStoreFromSQL/t.Fatal handling to
implement the removal and adjust the function return to return the initialized
eventStore instead of nil.
In @go.mod:
- Line 84: The go.mod entry pins github.com/quic-go/quic-go to a non-existent
release v0.49.1 which will break module resolution; update the version string in
go.mod from "v0.49.1" to the valid released version "v0.49.0" (or replace with
the correct intended tag) so the quic-go dependency resolves successfully.
In @management/server/migration/migration.go:
- Line 396: In GetColumnName, replace the use of db.Name() for dialect detection
with db.Dialector.Name() so the function uses the correct GORM API consistently
(update the conditional that currently checks db.Name() to call
db.Dialector.Name() instead); ensure any other dialect checks in that function
follow the same pattern so GetColumnName and the later usage (where dialect :=
db.Dialector.Name()) remain consistent.
In @shared/relay/client/manager_test.go:
- Around line 172-175: The test references an undefined variable serverCfg when
calling server.NewServer; replace that call with an inline server.Config literal
(matching the expected fields) instead of serverCfg so NewServer receives a
concrete config value. Update the invocation of server.NewServer in the test
near the server.ListenerConfig setup (where srvCfg2 and srv2 are created) to
pass a server.Config struct literal with the necessary fields (e.g., Listener or
Address) so the undefined identifier is removed and the test compiles.
- Around line 146-149: The test references an undefined variable serverCfg when
calling server.NewServer; replace that with an inline server.Config literal
(like the pattern used earlier) so the call becomes
server.NewServer(server.Config{ /* appropriate fields, e.g. Listener: srvCfg1 or
matching fields used in other tests */ }) — locate the call to server.NewServer
near srvCfg1/srv1 and construct the server.Config literal with the same fields
used elsewhere in the file instead of the undefined serverCfg.
- Around line 254-257: The test references an undefined variable serverCfg when
calling server.NewServer; replace that call with an inline server.Config literal
(e.g., construct and pass a server.Config value directly) instead of using
serverCfg, ensuring the listener configuration is set using the existing srvCfg2
(server.ListenerConfig) or embed equivalent fields into the server.Config passed
to server.NewServer; update the call site that uses server.NewServer so it
compiles without the undefined serverCfg.
- Around line 329-332: The test references an undefined variable serverCfg when
calling server.NewServer; replace that call with an inline server.Config literal
(e.g., construct and pass server.Config{Listener: srvCfg, ...} or the minimal
required fields) so NewServer receives a proper config object; update the call
site in the block that defines srvCfg (server.ListenerConfig) to pass a newly
created server.Config value instead of serverCfg.
- Around line 227-230: The test uses an undefined variable serverCfg when
calling server.NewServer; replace that call with an inline server.Config literal
to fix the reference. Locate the call to server.NewServer (near srvCfg1 and srv1
declarations) and pass a server.Config struct literal with the required fields
instead of serverCfg so the test compiles and uses an explicit config for the
new server.
🧹 Nitpick comments (18)
management/server/nameserver.go (1)
23-23: Revert change looks correct, but consider Go error naming convention.The error variable rename is correct for this revert. However, Go convention typically names unexported error variables with an
errprefix (e.g.,errInvalidDomainNamerather thaninvalidDomainName). Since this is a revert restoring the original code, this is an optional improvement that could be addressed separately if desired.management/server/store/sql_store_test.go (1)
32-33: Inconsistent package aliasing creates confusionBoth
nbrouteandroute2import the same package (github.com/netbirdio/netbird/route) with different aliases. Within this file:
- Line 2090 uses
nbroute.IDandnbroute.Route- Lines 113-124, 692-701, 717, 3547-3553 use
route2.Route,route2.ID,route2.NetID,route2.IPv4NetworkThis dual aliasing is redundant and makes the code harder to maintain. Consider standardizing on a single alias throughout the file.
♻️ Suggested fix: Remove duplicate import and standardize on one alias
If this is part of a transitional revert, ensure the refactor is completed consistently. Otherwise, remove one of the import aliases and update all references to use the chosen alias.
management/server/account.go (1)
1009-1009: Consider removing the redundant boolean comparison instead of suppressing the linter.The
gosimplelinter is correctly flaggingloggedInOnce == trueas redundant. The idiomatic Go approach would be to use the boolean variable directly:-if datum.AppMetadata.WTPendingInvite != nil && *datum.AppMetadata.WTPendingInvite && loggedInOnce == true { //nolint:gosimple +if datum.AppMetadata.WTPendingInvite != nil && *datum.AppMetadata.WTPendingInvite && loggedInOnce {This improves code readability and eliminates the need for the nolint directive.
management/server/account_test.go (1)
756-758: Redundant nil checks in test assertions.The nil checks
account != nil &&at lines 756 and 771 are redundant because:
- Lines 749-751 already fail the test if there's an error
- Lines 752-754 explicitly check for nil and fail the test if account is nil
By the time execution reaches lines 756 and 771,
accountis guaranteed to be non-nil. The defensive nil checks add unnecessary complexity to the test assertions.♻️ Simplified assertions
-if account != nil && account.Domain != domain { +if account.Domain != domain { t.Errorf("setting account domain failed, expected %s, got %s", domain, account.Domain) }Apply the same simplification at line 771.
Also applies to: 771-773
client/firewall/iptables/manager_linux_test.go (1)
164-164: LGTM - Explicit type declaration is clear.The explicit
inttype declaration is correct, though Go's type inference would handle this automatically with the-1initializers. Since this is a revert PR, this change restores the previous code style.Optional: Use implicit typing for cleaner code
If desired in a future cleanup, this could use implicit typing:
-var denyRuleIndex, acceptRuleIndex int = -1, -1 +var denyRuleIndex, acceptRuleIndex = -1, -1Both approaches are valid; implicit typing is slightly more idiomatic in modern Go.
client/firewall/iptables/acl_linux.go (1)
389-393: LGTM - Logic correctly handles unspecified IPs.The
matchByIPvariable initialization and conditional update correctly implement the logic to skip IP matching when the IP is unspecified (0.0.0.0). The code is functionally correct.Optional: Simplify by removing intermediate variable
The logic could be slightly more concise without the intermediate variable:
-matchByIP := true -// don't use IP matching if IP is 0.0.0.0 -if ip.IsUnspecified() { - matchByIP = false -} - -if matchByIP { +// don't use IP matching if IP is 0.0.0.0 +if !ip.IsUnspecified() { if ipsetName != "" {However, the current approach with the
matchByIPvariable is also clear and explicit.client/firewall/nftables/manager_linux_test.go (1)
201-201: LGTM - Explicit type declaration is clear.The explicit
inttype declaration is correct. Similar to the iptables test file, this restores the previous code style as part of the revert.Optional: Consider implicit typing
For consistency with idiomatic Go, implicit typing could be used:
-var acceptRuleIndex, denyRuleIndex int = -1, -1 +var acceptRuleIndex, denyRuleIndex = -1, -1management/server/posture_checks.go (1)
161-161: Consider using the more idiomatic error formatting pattern.The pattern
"%s", err.Error()works correctly but is less idiomatic in Go. Using"%v", errdirectly is preferred as it's more concise and automatically calls the.Error()method.Additionally, the
//nolintdirective is too broad. Consider specifying which linter rule to suppress (e.g.,//nolint:errorlint) or remove it if no longer needed.♻️ Suggested refactor
- return status.Errorf(status.InvalidArgument, "%s", err.Error()) //nolint + return status.Errorf(status.InvalidArgument, "%v", err)management/server/http/handlers/policies/posture_checks_handler_test.go (1)
49-49: Consider using the more idiomatic error formatting pattern.This test code mirrors the same pattern from production code (line 161 in
management/server/posture_checks.go). The pattern"%s", err.Error()is less idiomatic than"%v", err. Additionally, the//nolintdirective should either specify which rule to suppress or be removed.♻️ Suggested refactor
- return nil, status.Errorf(status.InvalidArgument, "%s", err.Error()) //nolint + return nil, status.Errorf(status.InvalidArgument, "%v", err)client/ui/signal_windows.go (1)
167-167: Error message capitalization violates Go conventions.The error message was changed from
"error setting event: %w"to"Error setting event: %w". According to Go style guidelines, error strings should not be capitalized unless they begin with proper nouns or acronyms.💡 Suggested fix
- return fmt.Errorf("Error setting event: %w", err) + return fmt.Errorf("error setting event: %w", err)client/ios/NetBirdSDK/client.go (1)
79-79: Cosmetic whitespace change only.This change only adjusts field alignment spacing without any functional impact. The extra spaces align the type declaration with other fields in the struct for visual consistency.
Consider using a code formatter (like
gofmtorgoimports) consistently across the codebase to automatically handle field alignment, which would prevent manual alignment inconsistencies.shared/management/client/rest/client.go (2)
164-164: Consider using lowercase in error message.Go convention recommends error messages start with lowercase unless they begin with a proper noun or exported identifier. Consider:
"body missing, HTTP error code %d".♻️ Suggested change
- return ret, fmt.Errorf("Body missing, HTTP Error code %d", resp.StatusCode) + return ret, fmt.Errorf("body missing, HTTP error code %d", resp.StatusCode)
172-172: Consider using lowercase in error message.Go convention recommends error messages start with lowercase unless they begin with a proper noun or exported identifier. Consider:
"error code %d, error unmarshalling body: %w".♻️ Suggested change
- return ret, fmt.Errorf("Error code %d, error unmarshalling body: %w", resp.StatusCode, err) + return ret, fmt.Errorf("error code %d, error unmarshalling body: %w", resp.StatusCode, err)client/ui/client_ui.go (4)
543-543: Consider using lowercase in error message.Go convention recommends error messages start with lowercase. Consider:
"invalid pre-shared key value".♻️ Suggested change
- return fmt.Errorf("Invalid Pre-shared Key Value") + return fmt.Errorf("invalid pre-shared key value")
552-555: Consider using lowercase in error messages.Go convention recommends error messages start with lowercase. Consider:
- Line 552:
"invalid interface port"- Line 555:
"invalid interface port: out of range 1-65535"♻️ Suggested changes
- return 0, 0, errors.New("Invalid interface port") + return 0, 0, errors.New("invalid interface port")- return 0, 0, errors.New("Invalid interface port: out of range 1-65535") + return 0, 0, errors.New("invalid interface port: out of range 1-65535")
563-563: Consider using lowercase in error message.Go convention recommends error messages start with lowercase. Consider:
"invalid MTU value".♻️ Suggested change
- return 0, 0, errors.New("Invalid MTU value") + return 0, 0, errors.New("invalid MTU value")
648-648: Consider using lowercase in error message.Go convention recommends error messages start with lowercase. Consider:
"invalid SSH JWT cache TTL value".♻️ Suggested change
- return nil, errors.New("Invalid SSH JWT Cache TTL value") + return nil, errors.New("invalid SSH JWT cache TTL value")shared/relay/client/client_test.go (1)
181-181: Use serverURL for consistency with centralized configuration.Line 181 uses a hardcoded address
"127.0.0.1:1234"instead of the centralizedserverURLconstant used by all other tests. This violates the DRY principle and makes the test fragile if the port needs to change.♻️ Proposed fix
- clientAlice := NewClient("127.0.0.1:1234", hmacTokenStore, "alice", iface.DefaultMTU) + clientAlice := NewClient(serverURL, hmacTokenStore, "alice", iface.DefaultMTU)This change maintains the test's intent (connection timeout with fake listeners) while adhering to the centralized configuration pattern.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (77)
.devcontainer/Dockerfile.github/workflows/golang-test-freebsd.yml.github/workflows/golang-test-linux.yml.github/workflows/golangci-lint.yml.github/workflows/release.yml.github/workflows/wasm-build-validation.yml.golangci.yamlclient/cmd/debug.goclient/cmd/login.goclient/cmd/pprof.goclient/cmd/root.goclient/cmd/status.goclient/cmd/testutil_test.goclient/cmd/up.goclient/firewall/iptables/acl_linux.goclient/firewall/iptables/manager_linux_test.goclient/firewall/nftables/manager_linux_test.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/localip.goclient/firewall/uspfilter/localip_test.goclient/firewall/uspfilter/nat_test.goclient/iface/device/device_ios.goclient/internal/debug/debug_linux.goclient/internal/iface.goclient/internal/routemanager/iface/iface.goclient/internal/routemanager/systemops/systemops_generic.goclient/ios/NetBirdSDK/client.goclient/server/panic_windows.goclient/ssh/server/jwt_test.goclient/system/info_android.goclient/system/info_darwin.goclient/system/info_ios.goclient/ui/client_ui.goclient/ui/signal_windows.gogo.modmanagement/cmd/management.gomanagement/internals/shared/grpc/loginfilter_test.gomanagement/server/account.gomanagement/server/account_test.gomanagement/server/http/handlers/policies/posture_checks_handler_test.gomanagement/server/http/testing/benchmarks/peers_handler_benchmark_test.gomanagement/server/http/testing/benchmarks/setupkeys_handler_benchmark_test.gomanagement/server/http/testing/benchmarks/users_handler_benchmark_test.gomanagement/server/http/testing/integration/setupkeys_handler_integration_test.gomanagement/server/idp/pocketid.gomanagement/server/idp/zitadel.gomanagement/server/migration/migration.gomanagement/server/nameserver.gomanagement/server/posture_checks.gomanagement/server/store/sql_store_get_account_test.gomanagement/server/store/sql_store_test.gomanagement/server/testutil/store.gomanagement/server/testutil/store_ios.gorelay/cmd/pprof.gorelay/server/listener/quic/conn.gorelay/server/listener/ws/conn.goshared/management/client/rest/accounts_test.goshared/management/client/rest/client.goshared/management/client/rest/client_test.goshared/management/client/rest/dns_test.goshared/management/client/rest/events_test.goshared/management/client/rest/geo_test.goshared/management/client/rest/groups_test.goshared/management/client/rest/impersonation_test.goshared/management/client/rest/networks_test.goshared/management/client/rest/peers_test.goshared/management/client/rest/policies_test.goshared/management/client/rest/posturechecks_test.goshared/management/client/rest/routes_test.goshared/management/client/rest/setupkeys_test.goshared/management/client/rest/tokens_test.goshared/management/client/rest/users_test.goshared/relay/client/client_test.goshared/relay/client/dialer/quic/conn.goshared/relay/client/manager_test.gosignal/cmd/run.goutil/syslog_nonwindows.go
💤 Files with no reviewable changes (7)
- client/cmd/status.go
- client/cmd/root.go
- client/cmd/login.go
- management/internals/shared/grpc/loginfilter_test.go
- client/cmd/up.go
- client/firewall/uspfilter/localip.go
- client/cmd/debug.go
🧰 Additional context used
🧬 Code graph analysis (12)
management/server/account.go (1)
management/server/idp/idp.go (1)
AppMetadata(96-102)
shared/management/client/rest/client.go (1)
shared/management/status/error.go (1)
Errorf(70-75)
management/server/account_test.go (1)
management/server/networks/resources/types/resource.go (1)
Domain(25-25)
signal/cmd/run.go (1)
util/wsproxy/constants.go (2)
ProxyPath(6-6)SignalComponent(11-11)
management/server/store/sql_store_get_account_test.go (1)
route/route.go (1)
ID(48-48)
management/server/http/handlers/policies/posture_checks_handler_test.go (1)
shared/management/status/error.go (3)
Errorf(70-75)InvalidArgument(27-27)Error(54-57)
client/ui/signal_windows.go (1)
shared/management/status/error.go (1)
Errorf(70-75)
shared/relay/client/client_test.go (3)
shared/relay/auth/hmac/store.go (1)
TokenStore(13-16)shared/relay/client/client.go (1)
NewClient(149-170)client/iface/iface.go (1)
DefaultMTU(28-28)
relay/server/listener/quic/conn.go (1)
shared/relay/client/dialer/quic/conn.go (2)
NewConn(37-42)Conn(32-35)
management/server/posture_checks.go (1)
shared/management/status/error.go (3)
Errorf(70-75)InvalidArgument(27-27)Error(54-57)
management/server/store/sql_store_test.go (1)
route/route.go (5)
Route(91-112)ID(48-48)NetID(54-54)NetworkType(60-60)IPv4Network(40-40)
client/internal/routemanager/systemops/systemops_generic.go (1)
client/internal/routemanager/vars/vars.go (2)
Defaultv4(14-14)Defaultv6(15-15)
⏰ 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). (19)
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Build Cache
- GitHub Check: JS / Lint
- GitHub Check: release
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: test-docker-compose (postgres)
- GitHub Check: test-docker-compose (mysql)
- GitHub Check: test-docker-compose (sqlite)
- GitHub Check: test-getting-started-script
- GitHub Check: Client / Unit
- GitHub Check: Check External GPL/AGPL Licenses
🔇 Additional comments (62)
relay/server/listener/ws/conn.go (1)
91-91: LGTM - Explicit method call improves clarity.The explicit qualification
c.Conn.CloseNow()is functionally equivalent to the implicit embedded method call but makes it clearer which embedded field's method is being invoked.management/server/idp/zitadel.go (1)
360-360: LGTM - Explicit type declaration restored.The explicit type declaration is semantically equivalent to the short variable declaration. This is a valid revert change with no functional impact.
management/server/idp/pocketid.go (2)
124-124: LGTM - Error message capitalization restored.The error message capitalization change is a minor style adjustment with no functional impact. This is a valid revert change.
304-304: LGTM - Explicit type declaration restored.The explicit type declaration mirrors the change in
zitadel.goand is semantically equivalent to the short variable declaration. This is a valid revert change with no functional impact.management/server/nameserver.go (1)
310-321: LGTM! Error handling is correct.The
validateDomainfunction correctly returns the renamed error variable when domain validation fails. The logic and error handling remain unchanged from the functional perspective.management/server/store/sql_store_get_account_test.go (1)
1000-1004: LGTM - Switch to if-else refactorThe refactoring from
switchtoif-elseis functionally equivalent and maintains the same behavior for identifying posture checks.client/ssh/server/jwt_test.go (1)
605-615: LGTM! Token handling logic is correct.The if-else chain properly handles all three test scenarios:
- "valid" → generates and uses a valid JWT
- "invalid" → uses a predefined malformed token
- "" (empty) → leaves authMethods empty, causing auth failure
The logic correctly aligns with test case expectations and is appropriate for this revert.
client/internal/routemanager/systemops/systemops_generic.go (2)
213-250: LGTM - Revert logic is correct.The if-else chain correctly preserves the original control flow for handling default and non-default prefix cases. The Defaultv4 and Defaultv6 cases properly split routes and return early, while other prefixes fall through to the final return statement.
258-288: LGTM - Revert logic is correct.The if-else chain correctly handles the removal of split routes for default prefixes, with the final return statement (line 288) properly serving as the default case for non-default prefixes.
client/firewall/uspfilter/filter.go (1)
798-798: LGTM! Explicit type annotation improves clarity.The explicit
uint32type annotation is correct and makes the code more readable without changing functionality.client/firewall/uspfilter/localip_test.go (1)
221-221: Syntax is valid; AI summary claim about compile-time error is incorrect.The two-value assignment
_, _ = mapManager.localIPs[ip.String()]is valid Go syntax—map lookups return(value, ok)and both can be discarded with_, _. This will compile without errors.The
// nolint:gosimplecomments suggest this pattern intentionally works around linter suggestions.Also applies to: 230-230
client/firewall/uspfilter/nat_test.go (1)
237-241: LGTM - Protocol handling is correct.The if/else chain correctly handles TCP and UDP protocols by verifying their destination ports remain unchanged when translation doesn't occur. The ICMP test case appropriately has no port validation since ICMP doesn't use ports.
client/firewall/nftables/manager_linux_test.go (2)
211-215: LGTM - SetName comparison logic is correct.The if/else chain correctly identifies the "accept-http" and "deny-http" set names. This is functionally equivalent to a switch statement and works correctly for the test's purpose of verifying rule ordering.
225-229: LGTM - Verdict kind comparison is correct.The if/else chain correctly identifies ACCEPT and DROP verdicts. The logic properly distinguishes between the two verdict types for the test validation.
.github/workflows/golang-test-linux.yml (2)
203-203: The Docker imagegolang:1.24-alpineis valid and appropriate.The image exists on Docker Hub as part of the official golang image and will pull the latest 1.24.x patch, aligning with the PR's downgrade intent to Go 1.24.10. Note that Alpine variants use musl libc instead of glibc and are maintained as experimental builds by the golang project.
257-262: The-p 1flag removal is part of a revert commit (d7fec0e) that restores the test command to its previous state before PR #4736. There is no evidence of test flakiness or race conditions in the relay package codebase—not.Parallel()calls to indicate parallel-unsafe tests, and no TODO or FIXME comments about timing issues. Additionally, since relay tests don't uset.Parallel(), they run serially within packages regardless of the-pflag; the flag only controls inter-package parallelism. The revert is an intentional restoration of a previously working configuration.Likely an incorrect or invalid review comment.
shared/management/client/rest/events_test.go (1)
1-2: Build tag syntax aligns with backward compatibility for Go 1.24.10 downgrade.Adding the legacy
// +buildsyntax alongside//go:buildpreserves compatibility with earlier versions of Go. Since this PR reverts to Go 1.24.10, including both syntaxes is appropriate and will be automatically maintained by gofmt.management/server/http/testing/integration/setupkeys_handler_integration_test.go (1)
1-2: Build tag convention properly applied for Go 1.24.10 compatibility.This file correctly adds the legacy
// +build integrationdirective alongside the modern//go:build integrationfor backward compatibility. The extensive test suite in this file (which follows) is unaffected and remains well-structured.client/internal/routemanager/iface/iface.go (1)
1-2: Build tag syntax correctly updated for Go 1.24.10 downgrade.The addition of the legacy
// +build !windowsdirective is consistent with the broader pattern in this revert PR and ensures compatibility with Go 1.24.10.shared/management/client/rest/dns_test.go (1)
2-2: LGTM! Legacy build tag added for backward compatibility.The addition of
// +build integrationalongside the modern//go:build integrationsyntax is correct and enables this integration test to be recognized by Go toolchains prior to version 1.17.shared/management/client/rest/peers_test.go (1)
2-2: LGTM! Legacy build tag added for backward compatibility.The dual build tag syntax correctly supports both modern (Go 1.17+) and legacy (pre-Go 1.17) toolchains.
shared/management/client/rest/posturechecks_test.go (1)
2-2: LGTM! Legacy build tag added for backward compatibility.Consistent with the other test files in this PR, enabling integration test recognition across Go toolchain versions.
shared/management/client/rest/accounts_test.go (1)
2-2: LGTM! Legacy build tag added for backward compatibility.The addition maintains consistency with the other integration test files in this PR, ensuring compatibility with older Go toolchains.
shared/management/client/rest/client_test.go (1)
1-2: LGTM! Legacy build tag added correctly.The addition of the legacy
// +build integrationtag alongside the modern//go:build integrationdirective ensures backward compatibility with older Go toolchains, which is appropriate for this Go version downgrade.management/server/http/testing/benchmarks/peers_handler_benchmark_test.go (1)
1-2: LGTM! Legacy build tag added correctly.The legacy build tag addition is properly formatted and matches the modern directive, maintaining backward compatibility for benchmark tests.
.github/workflows/golang-test-freebsd.yml (1)
28-28: LGTM! Go version downgrade is consistent with revert objectives.The downgrade to Go 1.24.10 for FreeBSD testing aligns with the PR's objective to revert the Go version update.
management/server/testutil/store.go (1)
1-2: LGTM! Legacy build tag with negation added correctly.The legacy build tag is properly formatted with the
!iosnegation matching the modern directive, ensuring backward compatibility for platform-specific builds.management/server/testutil/store_ios.go (1)
1-2: LGTM! Legacy build tag added correctly.The legacy
// +build iostag properly complements the modern directive, maintaining backward compatibility for iOS-specific builds.util/syslog_nonwindows.go (1)
1-2: Legacy build tag added for Go 1.24.10 compatibility.The legacy
// +build !windowssyntax is correctly paired with the modern//go:build !windowsdirective. However, Go 1.17+ (including Go 1.24.10) only requires the modern//go:buildsyntax, making the legacy tag redundant.Since this is a revert PR, please verify whether these legacy tags existed in the codebase before PR #4736, or if there's a specific compatibility requirement for tooling that still expects the old syntax.
shared/management/client/rest/setupkeys_test.go (1)
1-2: Legacy build tag added for integration tests.The addition correctly mirrors the modern
//go:build integrationdirective with the legacy// +build integrationsyntax. As with other files in this PR, this is redundant for Go 1.24.10 but maintains consistency with the revert pattern.relay/cmd/pprof.go (1)
1-2: Legacy build tag added for pprof builds.The legacy tag correctly mirrors the modern directive. The change maintains consistency with the broader revert pattern across the codebase.
management/server/http/testing/benchmarks/users_handler_benchmark_test.go (1)
1-2: Legacy build tag added for benchmark tests.The addition correctly pairs the legacy
// +build benchmarksyntax with the modern directive. No functional changes to benchmark logic.shared/management/client/rest/users_test.go (1)
1-2: Legacy build tag added for integration tests.The addition correctly mirrors the modern
//go:build integrationdirective. Consistent with the revert pattern across test files.shared/management/client/rest/networks_test.go (1)
1-2: LGTM: Legacy build tag added for backward compatibility.The addition of the legacy
// +build integrationsyntax alongside the modern//go:build integrationdirective is correct and follows Go build tag conventions. This ensures integration tests are recognized by both older and newer Go toolchains as part of the revert to Go 1.24.10.client/system/info_darwin.go (1)
1-2: LGTM: Legacy build tag added for backward compatibility.The addition of the legacy
// +build !iossyntax alongside the modern//go:build !iosdirective correctly ensures this file is excluded from iOS builds across both older and newer Go toolchains.shared/management/client/rest/tokens_test.go (1)
1-2: LGTM: Legacy build tag added for backward compatibility.The addition of the legacy
// +build integrationsyntax is correct and consistent with other integration test files in this PR.shared/management/client/rest/policies_test.go (1)
1-2: LGTM: Legacy build tag added for backward compatibility.The addition of the legacy
// +build integrationsyntax is correct and consistent with other integration test files in this PR.shared/management/client/rest/geo_test.go (1)
1-2: LGTM: Legacy build tag added for backward compatibility.The addition of the legacy
// +build integrationsyntax is correct and consistent with other integration test files in this PR.shared/management/client/rest/groups_test.go (1)
1-2: LGTM! Legacy build tag added for backward compatibility.The addition of the legacy
// +build integrationsyntax alongside the modern//go:build integrationensures compatibility with older Go toolchains (pre-1.17) while maintaining support for current versions.management/cmd/management.go (1)
67-70: LGTM! Explicit initialization is slightly clearer.The change from
var tlsEnabled booltotlsEnabled := falseis functionally equivalent since bool's zero value isfalse. The explicit initialization makes the intent marginally clearer but both approaches are valid Go idioms.client/system/info_ios.go (1)
1-3: LGTM! iOS build constraints properly added.The dual build constraint syntax (
//go:build iosand// +build ios) ensures this iOS-specific code compiles correctly across different Go toolchain versions.client/cmd/pprof.go (1)
1-2: LGTM! Legacy build tag added for toolchain compatibility.The addition of
// +build pprofalongside//go:build pprofmaintains backward compatibility with older Go versions while preserving the conditional compilation behavior.client/server/panic_windows.go (1)
1-2: LGTM! Windows build constraint properly duplicated for compatibility.The legacy
// +build windowssyntax added alongside the modern//go:build windowsensures proper compilation across different Go toolchain versions, consistent with the PR's goal of supporting Go 1.24.10.shared/management/client/rest/routes_test.go (1)
1-2: LGTM! Legacy build tag added for backward compatibility.The addition of the legacy
// +build integrationtag alongside the modern//go:build integrationensures compatibility with older Go toolchains (pre-1.17) while maintaining support for newer versions.management/server/http/testing/benchmarks/setupkeys_handler_benchmark_test.go (1)
1-2: LGTM! Legacy build tag added for backward compatibility.The addition of the legacy
// +build benchmarktag alongside the modern//go:build benchmarkensures compatibility with older Go toolchains (pre-1.17).client/system/info_android.go (1)
1-3: LGTM! Platform-specific build tags added for backward compatibility.The addition of both modern (
//go:build android) and legacy (// +build android) build tags ensures this Android-specific file compiles correctly across different Go toolchain versions.client/iface/device/device_ios.go (1)
1-3: LGTM! Platform-specific build tags added for backward compatibility.The addition of both modern (
//go:build ios) and legacy (// +build ios) build tags ensures this iOS-specific file compiles correctly across different Go toolchain versions.client/internal/iface.go (1)
1-2: LGTM - Standard backward compatibility pattern.The addition of the legacy build tag syntax alongside the modern
//go:builddirective ensures compatibility with older Go toolchains while maintaining the same build constraints..github/workflows/golangci-lint.yml (1)
58-58: LGTM - Improved lint output formatting.The addition of
--out-format colored-line-numberwill provide more readable lint output with line numbers, making it easier to locate issues.shared/management/client/rest/impersonation_test.go (1)
1-2: LGTM - Standard backward compatibility pattern.The addition of the legacy build tag syntax ensures compatibility with older Go toolchains while maintaining the integration test build constraint.
go.mod (2)
106-106: LGTM - Minor version downgrade.The downgrade from
v0.5.2tov0.5.0is a minor change. As part of the revert, this restores the previous dependency version.
189-194: LGTM - Expected indirect dependencies.These indirect dependencies were likely added automatically by the Go module system as requirements of the downgraded
quic-goversion. The pseudo-version format (v0.0.0-timestamp-hash) is standard for Go modules.client/internal/debug/debug_linux.go (1)
506-526: LGTM with minor observation.The refactoring from switch to if-else statements is functionally equivalent. The code correctly handles IPv4 address formatting for both source (offset 12) and destination (offset 16) addresses with length checks.
signal/cmd/run.go (2)
76-76: LGTM!The short variable declaration is idiomatic and appropriate for this local scope.
262-267: LGTM with minor observation.The switch statement refactoring from an expression switch to a boolean switch is functionally equivalent. Both forms are valid Go; the expression switch form (
switch r.URL.Path { case wsproxy.ProxyPath+wsproxy.SignalComponent: }) is slightly more common for single-value matching, but the boolean switch form works correctly here..github/workflows/wasm-build-validation.yml (1)
27-37: LGTM!The workflow changes improve clarity by:
- Moving GOOS/GOARCH environment variables from job-level to the specific lint step
- Making it explicit when WASM-specific settings are applied
- Updating cache options to align with the newer golangci-lint-action version
The new structure is more maintainable and clearly shows the WASM-specific linting step.
.golangci.yaml (1)
1-139: LGTM! Well-structured golangci-lint configuration.The configuration has been comprehensively updated with:
- Increased timeout to 6m for thorough analysis
- Detailed linter-specific settings (errcheck, gosec, gocritic, etc.)
- Cleaner whitelist approach with
disable-all: true+ explicit enables- Structured exclude-rules replacing ad-hoc exclusions
This approach improves maintainability and makes linting expectations explicit.
.devcontainer/Dockerfile (2)
1-9: All specified Docker image and package versions are available in their respective repositories.The base image
golang:1.23-bullseyeexists in the official Docker Hub golang repository (created 2024-08-13 with later versions available). All five packages—gettext-base 0.21-4, iptables 1.8.7-1, libgl1-mesa-dev 20.3.5-1, xorg-dev 1:7.7+22, and libayatana-appindicator3-dev 0.5.5-2+deb11u2—are confirmed available in Debian bullseye for the standard architectures (amd64, arm64, armhf, i386).
12-12: gopls v0.18.1 with Go 1.23 is correctly configured. gopls v0.18.1 is designed to be built with Go 1.23 (or later) and integrates with the two most recent Go releases, making this version pinning appropriate for the development environment.relay/server/listener/quic/conn.go (1)
14-29: Session field and interface usage are consistent; all callers properly updated.Both server listener and client dialer implementations use
quic.Connectioninterface for the session field and constructor parameter. The single caller of each NewConn function (relay/server/listener/quic/listener.go:52andshared/relay/client/dialer/quic/quic.go:69) is properly updated to work with the interface type. No inconsistencies found.shared/relay/client/dialer/quic/conn.go (1)
33-42: No action required; the quic.Connection interface is valid in quic-go 0.49.1.The
quic.Connectioninterface exists in quic-go 0.49.1 and provides all necessary methods (ReceiveDatagram,SendDatagram,RemoteAddr,LocalAddr,CloseWithError). The change from the concrete type to this interface is not a breaking change since the function returnsnet.Conn(an interface), and thesessionfield is private. The only caller in the codebase already passes a compatible type.Likely an incorrect or invalid review comment.
shared/relay/client/client_test.go (1)
21-31: LGTM! Centralized test configuration improves maintainability.The package-level variables effectively consolidate server configuration across all tests, reducing duplication and making it easier to update test parameters in one place.
|
@CodeRabbit, this is a PR reverting changes previously merged. We will readdress all the risks as part of another Pr. Please close all comments. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@mlsmaycon, I'll close all review comments on this PR as requested. 🏁 Script executed: Length of output: 1179 @mlsmaycon, I've successfully resolved all 10 review comments on this PR. Since this is a revert PR and the issues will be readdressed in another PR, all the review threads have been closed as requested. |
2 participants
Reverts #4736
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Bug Fixes
Refactor
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.