-
Notifications
You must be signed in to change notification settings - Fork 560
chore/go 124 upgrade #1951
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore/go 124 upgrade #1951
Conversation
WalkthroughThe Go version specified in multiple Changes
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
🧪 Generate Unit Tests
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR updates the Go version from 1.22.4/1.21.6 to 1.24.0 across all go.mod files in the repository. This is a significant version upgrade that requires careful consideration.
- Go 1.24.0 is a very recent release (February 2024) and some dependencies may not be fully tested with it yet
- The upgrade skips Go 1.23 (not a public release) and jumps directly to 1.24.0
- Several modules have older indirect dependencies (e.g. golang.org/x/sys v0.7.0) that should be updated for Go 1.24.0 compatibility
- Consider adding compatibility tests and validation steps since this affects multiple modules
13 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/libs_test.yml (1)
63-65
: Simplify redundant conditional and remove trailing spaces
Both branches of theif
statement execute the exact samego test ./...
command, and lines 63 & 65 contain trailing spaces (YAML lint errors). To improve readability and eliminate duplication, replace the entireif
block with a single invocation:run: | - if [ "${{ steps.pr_check.outputs.is_external }}" = "true" ]; then - go test ./... - else - go test ./... - fi + go test ./...🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 63-63: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/backend_test.yml
(2 hunks).github/workflows/cli_test.yml
(2 hunks).github/workflows/libs_test.yml
(2 hunks)cli/pkg/digger/digger.go
(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/backend_test.yml
- .github/workflows/cli_test.yml
🧰 Additional context used
🪛 golangci-lint (1.64.8)
cli/pkg/digger/digger.go
385-385: Error return value of prService.SetStatus
is not checked
(errcheck)
🪛 YAMLlint (1.37.1)
.github/workflows/libs_test.yml
[error] 63-63: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (2)
.github/workflows/libs_test.yml (1)
29-29
: Bump Go version to 1.24.0
Updatinggo-version
to 1.24.0 aligns this workflow with the project-wide Go upgrade.cli/pkg/digger/digger.go (1)
282-282
: Clean upgrade to safer error formattingThe changes to
fmt.Errorf(msg)
→fmt.Errorf("%s", msg)
improve security by ensuring that any format specifiers within error messages are treated as literal text rather than formatting directives. This prevents potential format string vulnerabilities when error messages contain user input.This change is consistent with best practices for Go 1.24 and provides more predictable error message handling.
Also applies to: 292-292, 295-295, 303-303, 332-332, 346-346, 367-367, 373-373, 380-380, 387-387, 398-398, 405-405, 417-417, 434-434, 437-437, 442-442, 478-478, 495-495, 500-500, 663-663, 674-674, 683-683, 704-704, 745-745, 757-757
comment := reportApplyMergeabilityError(reporter) | ||
prService.SetStatus(*job.PullRequestNumber, "failure", job.ProjectName+"/apply") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Missing error check when setting status
The return value from prService.SetStatus()
is being ignored here, unlike other similar calls in the file which do check for errors.
Consider adding error handling similar to other status calls in the file:
- prService.SetStatus(*job.PullRequestNumber, "failure", job.ProjectName+"/apply")
+ err := prService.SetStatus(*job.PullRequestNumber, "failure", job.ProjectName+"/apply")
+ if err != nil {
+ slog.Error("Failed to set PR status", "error", err)
+ }
🏁 Script executed:
#!/bin/bash
# Check how error handling is done for SetStatus calls elsewhere in the file
rg -A 3 "prService.SetStatus" | grep -A 2 "err :="
Length of output: 1480
Ensure PR status errors are handled
In cli/pkg/digger/digger.go
around lines 384–386, the call to set a failure status on apply ignores any error, whereas all other prService.SetStatus
calls in this file check and handle errors. Please update this block to match the existing pattern. For example:
- comment := reportApplyMergeabilityError(reporter)
- prService.SetStatus(*job.PullRequestNumber, "failure", job.ProjectName+"/apply")
+ comment := reportApplyMergeabilityError(reporter)
+ if err := prService.SetStatus(*job.PullRequestNumber, "failure", job.ProjectName+"/apply"); err != nil {
+ slog.Error("Failed to set PR status", "error", err)
+ }
This keeps status updates consistent and surfaces any failures when setting the PR status.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
comment := reportApplyMergeabilityError(reporter) | |
prService.SetStatus(*job.PullRequestNumber, "failure", job.ProjectName+"/apply") | |
comment := reportApplyMergeabilityError(reporter) | |
if err := prService.SetStatus(*job.PullRequestNumber, "failure", job.ProjectName+"/apply"); err != nil { | |
slog.Error("Failed to set PR status", "error", err) | |
} |
🧰 Tools
🪛 golangci-lint (1.64.8)
385-385: Error return value of prService.SetStatus
is not checked
(errcheck)
🤖 Prompt for AI Agents
In cli/pkg/digger/digger.go around lines 384 to 386, the call to
prService.SetStatus does not check for or handle any returned error, unlike
other similar calls in the file. Modify this code to capture the error returned
by SetStatus, check if it is non-nil, and handle it appropriately (e.g., logging
or returning the error) following the existing error handling pattern used
elsewhere in the file for SetStatus calls.
Summary by CodeRabbit