Bump Go, golangci-lint, update GitHub Actions, fix lint issues#91
Conversation
|
Warning Rate limit exceeded@SchSeba has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 9 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (19)
WalkthroughUpgrades Go to 1.25, updates CI/CD workflows and Docker base image, adds a golangci-lint configuration and Makefile lint targets, and applies code/test changes for Go 1.25 compatibility (symlink handling, HTTP server config, SR-IOV reader signatures and related tests). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
SchSeba
force-pushed
the
multiple_bumps
branch
from
4d6d7c9 to
b70d0d0
Compare
SchSeba
changed the title
SchSeba
force-pushed
the
multiple_bumps
branch
5 times, most recently
from
25d611e to
1950108
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: 0
🧹 Nitpick comments (1)
collectors/pod_dev_link.go (1)
16-16: Consider updating gRPC to a more recent version.gRPC 1.56.3 is compatible with Go 1.25, but a much newer version (v1.77.0, November 2025) is available and maintains Go 1.25 support. Consider updating to keep the dependency current.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/build-test-lint.yml.golangci.ymlDockerfileMakefilecmd/sriov-network-metrics-exporter.gocmd/sriov-network-metrics-exporter_test.gocollectors/collectors.gocollectors/collectors_test.gocollectors/pod_cpu_link.gocollectors/pod_cpu_link_test.gocollectors/pod_dev_link.gocollectors/sriovdev.gocollectors/sriovdev_readers.gocollectors/sriovdev_readers_test.gocollectors/sriovdev_test.gogo.modpkg/utils/utils.gopkg/utils/utils_test.gopkg/vfstats/netlink_test.go
⏰ 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: Analyze (go)
- GitHub Check: build-image
🔇 Additional comments (34)
pkg/vfstats/netlink_test.go (2)
28-50: LGTM! Formatting improves readability.The VfInfo struct literals have been reformatted from compact inline format to multi-line format, making the test data easier to read and maintain. No semantic changes.
56-56: Remove this comment—the test file was newly created, not modified.The
pkg/vfstats/netlink_test.gofile was added as a new file in commit 1950108, not changed from a capitalized error message. The lowercase "link not found" was present in the initial creation and aligns with Go conventions.Likely an incorrect or invalid review comment.
pkg/utils/utils_test.go (1)
30-32: LGTM - Error suppression is appropriate here.Ignoring the error from
os.Removein cleanup is acceptable since the symlink may not exist for all test cases. This prevents spurious test failures.collectors/collectors.go (2)
1-4: LGTM - Documentation improvements.The package comment reformatting improves clarity and readability.
27-29: LGTM - Comment formatting improved.The function comment is now more concise and clear.
cmd/sriov-network-metrics-exporter_test.go (1)
24-24: LGTM - Better idiom for empty request bodies.Using
http.NoBodyinstead ofnilis more explicit and aligns with Go best practices. ThenoBodyhandler test correctly retainsnilto verify body checking logic.Also applies to: 38-38, 70-70
cmd/sriov-network-metrics-exporter.go (3)
10-10: LGTM - Time import added for timeout configuration.
19-22: LGTM - Good practice to define constants.The constants improve maintainability and make the timeout values explicit.
47-53: Excellent security improvement with ReadHeaderTimeout.Adding
ReadHeaderTimeoutprotects against Slowloris-style attacks by preventing clients from holding connections open indefinitely. The 10-second timeout is appropriate for a metrics endpoint.Makefile (2)
60-66: LGTM - Simplified and improved lint workflow.The refactored lint targets improve consistency and maintainability by centralizing the golangci-lint binary path and removing redundant installation steps.
54-58: Version v2.7.2 confirmed. golangci-lint v2.7.2 is a legitimate release (December 7, 2025) with recent bug fixes. The major version jump from v1.63.4 to v2.7.2 represents a standard v1-to-v2 upgrade.pkg/utils/utils.go (2)
58-58: LGTM - Simplified EvalSymlinks assignment.Direct assignment to
filepath.EvalSymlinksis cleaner and achieves the same testability goal.
60-82: Well-handled Go 1.25 compatibility fix.The
IsSymLinkimplementation correctly works around the fstest.MapFS behavioral change in Go 1.25 by usingfs.ReadDirinstead offs.Stat. The comment clearly explains the rationale. The logic properly extracts directory and base, iterates entries, and checks for the symlink flag.go.mod (1)
3-3: No action needed. Go 1.25.3 is an officially released and stable version (released October 13, 2025), not a future release.Likely an incorrect or invalid review comment.
Dockerfile (1)
1-1: Go 1.25 is stable and production-ready.Go 1.25 was released on August 12, 2025, and is currently at patch version 1.25.5 (released December 2, 2025). This version is suitable for production use.
Likely an incorrect or invalid review comment.
.golangci.yml (1)
1-100: LGTM! Comprehensive and well-configured linting setup.The golangci-lint configuration is thorough with:
- Appropriate timeout and formatters
- Comprehensive linter suite with nuanced per-linter tuning
- Sensible test file exclusions
collectors/sriovdev_readers_test.go (1)
9-11: LGTM! Clean import reordering.Import organization updated to align with formatting standards. No functional changes.
collectors/sriovdev.go (1)
231-232: LGTM! Parameter naming improvement.The parameter renamed from
filepathtofilePathimproves consistency with Go naming conventions and avoids shadowing thefilepathpackage name.collectors/sriovdev_test.go (2)
85-92: LGTM! Improved test data formatting.VfInfo struct initializations reformatted to compact style, improving readability while maintaining test functionality.
Also applies to: 130-133
156-156: LGTM! Appropriate lint suppression.The
nolint:dupldirective correctly suppresses duplicate code warnings for test table entries, which intentionally share similar structure.collectors/pod_cpu_link.go (2)
25-29: LGTM! Improved flag readability.Flag definitions split across multiple lines improve readability while maintaining functionality.
266-266: LGTM! Appropriate magic number suppression.The
nolint:mnddirective correctly suppresses the magic number detector for2, which represents the expected two-element format of a CPU range (start-end).collectors/pod_cpu_link_test.go (2)
56-71: LGTM! Enhanced test readability.Metric expectations reformatted with explicit map literals, improving clarity of expected test outcomes.
86-87: LGTM! Better error message formatting.Long error messages split across lines for improved readability.
Also applies to: 208-209
collectors/collectors_test.go (2)
9-9: LGTM! Required import for fstest utilities.The
testing/fstestimport is needed for the updated symlink handling logic.
90-114: Well-adapted symlink handling for Go 1.25.The updated
evalSymlinksfunction correctly addresses Go 1.25's fstest.MapFS symlink behavior changes by:
- Enumerating directory entries to detect symlinks by type
- Directly accessing MapFS data for symlink targets
- Providing clear error messages for different failure cases
The approach of casting to
fstest.MapFSand reading the Data field directly is the correct workaround for the new symlink semantics.Note: This change is essential for test compatibility with Go 1.25 and aligns with the PR's stated objectives.
.github/workflows/build-test-lint.yml (1)
101-103: No action needed. The golangci-lint versionv2.7.2is the latest stable release (as of December 2025) and is fully compatible with golangci-lint-action v8 (which requires >= v2.1.0). The version is correct.Likely an incorrect or invalid review comment.
collectors/sriovdev_readers.go (3)
52-54: LGTM - Path construction fix.The change from
filepath.Jointo string concatenation correctly builds the sriov path. Usingfilepath.Join(pf, "/device/sriov")with a leading slash in the second argument would have returned just"/device/sriov"on Unix systems, not"pf/device/sriov"as intended. The new concatenation approach properly combines the paths.
86-105: LGTM - Clean refactor with appropriate lint suppression.The changes improve code clarity:
- Direct return pattern eliminates unnecessary intermediate variable
- The
nolint:gosecdirective for G115 is appropriate here since network statistics are bounded by physical hardware limits and extremely unlikely to overflow int64 (max ~9.2 exabytes)
107-107: LGTM - Consistent parameter ordering.The signature update aligns with the
netlinkReaderimplementation and maintains consistency across both reader types.collectors/pod_dev_link.go (4)
23-26: LGTM! Good practice to define timeout constant.Introducing
kubeletConnTimeouteliminates magic numbers and improves maintainability. The 10-second timeout is appropriate for kubelet gRPC connections.
28-33: LGTM! Proper variable grouping.Grouping related variables follows idiomatic Go style and likely addresses golangci-lint requirements.
102-114: Excellent fix for resource leak!Multiple improvements here:
Critical fix (lines 109-113): Adding deferred connection close prevents resource leaks. The previous code wasn't closing the gRPC connection, which could accumulate over time. The error handling in the defer is also appropriate.
Lines 102-103: Direct string concatenation is cleaner, and using
kubeletConnTimeoutconstant eliminates magic numbers.Line 114: Consistent use of the timeout constant.
144-152: LGTM! Better variable naming.Renaming to
parsedURLavoids shadowing thenet/urlpackage, which is a common golangci-lint issue. This improves code clarity and follows Go best practices.
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)
collectors/sriovdev_readers.go (1)
22-24: Fix parameter order mismatch between interface and implementations.The interface declares
ReadStats(vfID string, pfName string)(line 24), but the implementations at lines 86 and 107 declareReadStats(pfName, vfID string). Although both havestring, stringparameter types, the parameter order differs. When called at line 40 asreader.ReadStats(pfName, vfID), the positional arguments are assigned to interface parameter positions in swapped order—pfNamegoes tovfIDandvfIDgoes topfName. The implementations use the swapped values, causing incorrect logic (e.g., line 87 attempts to convertvfID, which now contains the physical function name).Update either the interface or all implementations to use consistent parameter order. The implementations and call site suggest the intended order is
(pfName, vfID).
🧹 Nitpick comments (3)
pkg/utils/utils_test.go (1)
31-31: Cleanup error handling is acceptable, but could be more precise.Silencing the error during cleanup is reasonable since not all test cases create the symlink. However, this approach also suppresses legitimate errors like permission issues.
🔎 Optional improvement to only ignore "file does not exist" errors
- _ = os.Remove(linkPath) + if err := os.Remove(linkPath); err != nil && !os.IsNotExist(err) { + log.Printf("Failed to clean up symlink: %v", err) + }cmd/sriov-network-metrics-exporter.go (1)
47-54: Excellent security improvement with explicit server configuration.The addition of
ReadHeaderTimeoutis a security best practice that mitigates Slowloris attacks by preventing clients from holding connections open indefinitely with slow header transmission.Optional enhancement: Consider adding other timeouts for defense in depth:
💡 Additional timeouts to consider
server := &http.Server{ Addr: *addr, Handler: handlerWithMiddleware, ReadHeaderTimeout: defaultReadHeaderTimeout, + ReadTimeout: 30 * time.Second, + WriteTimeout: 30 * time.Second, + IdleTimeout: 60 * time.Second, }These additional timeouts provide:
ReadTimeout: Total time to read the entire requestWriteTimeout: Total time to write the responseIdleTimeout: Keep-alive timeout for idle connectionscollectors/sriovdev_readers.go (1)
52-54: Consider using the existing constant for consistency.The sysfs path construction on line 54 hardcodes the path pattern by concatenating
*sysClassNet + "/%s/device/sriov/%s/stats", while line 110 uses the existing constantsriovVFStatsDir. Consider refactoring for consistency, though the current implementation is functionally correct.💡 Alternative approach using the existing pattern
The current approach explicitly includes the base path, which provides clarity. However, if you want consistency with line 110's usage, you could maintain the sriovPath variable for existence checking while keeping the format string construction similar to the constant pattern. The current implementation is acceptable as-is.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/build-test-lint.yml.golangci.ymlDockerfileMakefilecmd/sriov-network-metrics-exporter.gocmd/sriov-network-metrics-exporter_test.gocollectors/collectors.gocollectors/collectors_test.gocollectors/pod_cpu_link.gocollectors/pod_cpu_link_test.gocollectors/pod_dev_link.gocollectors/sriovdev.gocollectors/sriovdev_readers.gocollectors/sriovdev_readers_test.gocollectors/sriovdev_test.gogo.modpkg/utils/utils.gopkg/utils/utils_test.gopkg/vfstats/netlink_test.go
✅ Files skipped from review due to trivial changes (2)
- collectors/sriovdev_readers_test.go
- collectors/collectors.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/sriov-network-metrics-exporter_test.go
- pkg/utils/utils.go
- .golangci.yml
- collectors/pod_dev_link.go
- collectors/pod_cpu_link_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/vfstats/netlink_test.go (1)
pkg/vfstats/netlink.go (1)
PerPF(11-14)
⏰ 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). (1)
- GitHub Check: build-image
🔇 Additional comments (21)
pkg/utils/utils_test.go (1)
70-76: LGTM! Clean test coverage for IsSymLink.The test properly validates both positive and negative cases using
fstest.MapFS, providing good coverage for the newIsSymLinkfunction.cmd/sriov-network-metrics-exporter.go (2)
10-10: LGTM! Standard import organization.The addition of the
timeimport and the reorganization of imports following Go conventions look good.Also applies to: 15-17
27-27: Good refactoring to use the constant.Replacing the hardcoded value with
defaultRateBurstimproves maintainability.collectors/collectors_test.go (3)
9-9: LGTM!The
testing/fstestimport is necessary for the new MapFS-based symlink handling logic inevalSymlinks.
85-85: LGTM!Good simplification. The explicit
time.Duration()conversion is unnecessary since2 * time.Secondalready returns atime.Duration.
90-115: Implementation correctly handles Go 1.25 fstest.MapFS symlink behavior.The code properly works around Go 1.25's change where
fstest.MapFSnow implementsReadLinkFSandReadFilefollows symlinks. By directly accessingmapFS[path].Data, the implementation correctly retrieves the symlink target withoutReadFileattempting to follow the link. The type assertion is safe, and error handling is appropriate for the emulation.Dockerfile (1)
1-1: LGTM! Go version pinning aligns with broader upgrade.The Docker base image update from
golang:alpinetogolang:1.25-alpinecorrectly aligns with the Go 1.25.3 upgrade in go.mod and the CI workflow changes.collectors/sriovdev.go (1)
231-232: LGTM! Parameter rename avoids package shadowing.Renaming the parameter from
filepathtofilePathprevents shadowing thefilepathpackage import. This is a good lint fix with no functional impact.collectors/pod_cpu_link.go (2)
3-4: LGTM! Formatting improvements for lint compliance.The comment and flag definition formatting changes improve readability and address linter requirements without changing functionality.
Also applies to: 25-26, 28-29, 114-114, 160-161
266-266: LGTM! Appropriate use of nolint directive.The
//nolint:mnddirective appropriately suppresses the magic number detector for the constant2, which represents a CPU range with two endpoints (start and end). This is a reasonable semantic constant that doesn't need extraction.pkg/vfstats/netlink_test.go (2)
28-51: LGTM! Test data formatting improved.The multi-line formatting of VfInfo structs improves readability and maintainability of test fixtures.
56-56: The review comment is incorrect.The file
pkg/vfstats/netlink_test.gowas newly created in commit 1950108, not modified. There was no prior version with "Link not found" (capitalized). The lowercasefmt.Errorf("link not found")has been present since the file's creation. Additionally, this is a mock error used in the test—it does not verify the actual error message returned by the netlink library but rather tests howVfStats()handles errors in general.Likely an incorrect or invalid review comment.
collectors/sriovdev_test.go (2)
85-93: LGTM! Test data formatting improved.The VfInfo struct formatting in test fixtures is more compact and readable while maintaining all necessary test data.
Also applies to: 130-134
156-156: LGTM! Appropriate use of nolint directive.The
//nolint:dupldirective with explanatory comment is appropriate for test tables, which intentionally have similar structure. The comment clearly explains the design intent.Makefile (2)
54-62: LGTM! Improved lint workflow structure.The new lint infrastructure centralizes golangci-lint installation and execution. The version v2.7.2 correctly matches the CI workflow configuration, and the 5-minute timeout is appropriate for lint operations.
63-66: LGTM! Cleaner target dependencies.The refactored
go-lintandgo-lint-reporttargets properly depend on the shared golangci-lint binary, improving maintainability.go.mod (1)
3-3: Go 1.25.3 upgrade is appropriate. The version is stable and available (released October 13, 2025 with crypto/x509 fixes). The upgrade aligns cleanly with other changes in the PR..github/workflows/build-test-lint.yml (2)
101-103: No action needed—the .golangci.yml is already properly configured for golangci-lint v2. The config declaresversion: "2", includes the v2formatterssection, and uses the correct v2exclusions.rulessyntax. The upgrade to v2.7.2 is compatible with the current configuration.
12-12: All GitHub Actions and Go versions are valid and compatible.✓
actions/setup-go@v6exists and supports Go 1.25.x (requires runner v2.327.1+)
✓golangci/golangci-lint-action@v8exists and compatible with golangci-lint v2.7.2
✓ Go 1.25 released August 12, 2025No compatibility issues identified.
Also applies to: 18-18, 60-62, 79-81, 96-98, 122-124
collectors/sriovdev_readers.go (2)
86-105: Implementation looks good; gosec suppression is reasonable.The refactored implementation is cleaner with the direct map return. The
gosecG115 suppression for integer overflow on line 94 is reasonable for network statistics, which are extremely unlikely to exceedmath.MaxInt64in practice. However, be aware that this assumes well-behaved network statistics and could theoretically wrap to negative values if the underlying uint64 values are very large.
107-107: LGTM: Signature change improves consistency.The signature update to use the compact form
ReadStats(pfName, vfID string)matches the netlinkReader implementation and is idiomatic Go style.
Signed-off-by: Sebastian Sch <sebassch@gmail.com>
SchSeba
force-pushed
the
multiple_bumps
branch
from
1950108 to
eb45670
Compare
|
Thanks folks merging this one |
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.