patch error while posting comment too large #1908
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: https://app.greptile.com/review/github.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request modifies the error handling logic within the Changes
Sequence Diagram(s)sequenceDiagram
participant R as RunJobs
participant Rep as Reporter
participant PR as PRService
participant L as Logger
R->>Rep: Call Flush()
alt Flush succeeds
R->>R: Continue normal execution
else Flush fails
R->>R: Retrieve currentJob from jobs list
R->>PR: Publish warning comment on PR
alt Publish succeeds
R->>R: Proceed (comment added)
else Publish fails
PR->>L: Log error
R->>R: Return formatted error message
end
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cli/pkg/digger/digger.go (2)
134-139: Fix error variable in log message and improve error handling robustness.The code uses
errinstead ofcmt_errin the error log message at line 136. Also, the message format could provide clearer context about the sequence of failures.- cmt, cmt_err := prService.PublishComment(*currentJob.PullRequestNumber, fmt.Sprintf(":yellow_circle: Warning: failed to post comment for project %v, recieved error: %v.\n\n you may review details in the job logs", currentJob.ProjectName, err)) - if cmt_err != nil { - log.Printf("Error while posting error comment: %v", err) - return false, false, fmt.Errorf("failed to post reporter error comment, aborting. Error: %v", err) + cmt, cmt_err := prService.PublishComment(*currentJob.PullRequestNumber, fmt.Sprintf(":yellow_circle: Warning: failed to post comment for project %v, received error: %v.\n\n You may review details in the job logs", currentJob.ProjectName, err)) + if cmt_err != nil { + log.Printf("Error while posting error comment: %v", cmt_err) + return false, false, fmt.Errorf("failed to post reporter error comment, aborting. Original error: %v, Publish error: %v", err, cmt_err)
134-134: Fix typo in warning message.There's a typo in the warning message: "recieved" should be "received".
- cmt, cmt_err := prService.PublishComment(*currentJob.PullRequestNumber, fmt.Sprintf(":yellow_circle: Warning: failed to post comment for project %v, recieved error: %v.\n\n you may review details in the job logs", currentJob.ProjectName, err)) + cmt, cmt_err := prService.PublishComment(*currentJob.PullRequestNumber, fmt.Sprintf(":yellow_circle: Warning: failed to post comment for project %v, received error: %v.\n\n You may review details in the job logs", currentJob.ProjectName, err))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/pkg/digger/digger.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-and-push-image
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (1)
cli/pkg/digger/digger.go (1)
129-140: Good implementation of graceful error handling for large comments.Instead of failing silently when comments are too large, the code now attempts to publish a smaller, informative comment to notify users about the issue. This is a good usability improvement that helps users understand what happened.
| @@ -126,13 +126,19 @@ func RunJobs(jobs []orchestrator.Job, prService ci.PullRequestService, orgServic | |||
| } | |||
|
|
|||
| if allAppliesSuccess == true && reportFinalStatusToBackend == true { | |||
| currentJob := jobs[0] | |||
There was a problem hiding this comment.
Check if jobs array is empty before accessing index 0.
The code retrieves the first job from the jobs array without verifying if the array is empty. If the jobs array is empty, accessing jobs[0] will cause a panic.
Add a check to ensure the jobs array is not empty:
- currentJob := jobs[0]
+ if len(jobs) == 0 {
+ return false, false, fmt.Errorf("error while sending job comments: %v", err)
+ }
+ currentJob := jobs[0]Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cli/pkg/digger/digger.go (1)
129-130:⚠️ Potential issueCheck if jobs array is empty before accessing index 0.
The code retrieves the first job from the jobs array without verifying if the array is empty. If the jobs array is empty, accessing
jobs[0]will cause a panic.Apply this fix:
- currentJob := jobs[0] + if len(jobs) == 0 { + return false, false, fmt.Errorf("error while sending job comments: %v", err) + } + currentJob := jobs[0]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/pkg/digger/digger.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
cli/pkg/digger/digger.go (3)
cli/pkg/github/github.go (1)
jobs(249-249)cli/cmd/digger/root.go (3)
err(107-107)reporter(33-33)prService(31-31)libs/comment_utils/reporting/reporting.go (5)
err(65-65)reporter(89-92)reporter(94-96)reporter(98-100)reporter(102-104)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
cli/pkg/digger/digger.go
Outdated
| cmt, cmt_err := prService.PublishComment(*currentJob.PullRequestNumber, fmt.Sprintf(":yellow_circle: Warning: failed to post report for project %v, recieved error: %v.\n\n you may review details in the job logs", currentJob.ProjectName, err)) | ||
| if cmt_err != nil { | ||
| log.Printf("Error while posting error comment: %v", err) | ||
| return false, false, fmt.Errorf("failed to post reporter error comment, aborting. Error: %v", err) | ||
| } | ||
| jobPrCommentUrl = cmt.Url | ||
| } |
There was a problem hiding this comment.
Fix error logging and improve error handling.
There are two issues in this error handling block:
- Line 136 is logging the wrong error variable - it uses
err(from reporter.Flush()) instead ofcmt_err(from PublishComment) - Before accessing
currentJob.PullRequestNumber, there should be a nil check to prevent potential panic
Apply these fixes:
- cmt, cmt_err := prService.PublishComment(*currentJob.PullRequestNumber, fmt.Sprintf(":yellow_circle: Warning: failed to post report for project %v, recieved error: %v.\n\n you may review details in the job logs", currentJob.ProjectName, err))
- if cmt_err != nil {
- log.Printf("Error while posting error comment: %v", err)
- return false, false, fmt.Errorf("failed to post reporter error comment, aborting. Error: %v", err)
- }
- jobPrCommentUrl = cmt.Url
+ if currentJob.PullRequestNumber == nil {
+ return false, false, fmt.Errorf("error while sending job comments: %v", err)
+ }
+ cmt, cmt_err := prService.PublishComment(*currentJob.PullRequestNumber, fmt.Sprintf(":yellow_circle: Warning: failed to post report for project %v, received error: %v.\n\n you may review details in the job logs", currentJob.ProjectName, err))
+ if cmt_err != nil {
+ log.Printf("Error while posting error comment: %v", cmt_err)
+ return false, false, fmt.Errorf("failed to post reporter error comment, aborting. Original error: %v, Comment error: %v", err, cmt_err)
+ }
+ jobPrCommentUrl = cmt.UrlAlso note there's a typo in "recieved" which I've corrected to "received" in the suggested fix.
📝 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.
| cmt, cmt_err := prService.PublishComment(*currentJob.PullRequestNumber, fmt.Sprintf(":yellow_circle: Warning: failed to post report for project %v, recieved error: %v.\n\n you may review details in the job logs", currentJob.ProjectName, err)) | |
| if cmt_err != nil { | |
| log.Printf("Error while posting error comment: %v", err) | |
| return false, false, fmt.Errorf("failed to post reporter error comment, aborting. Error: %v", err) | |
| } | |
| jobPrCommentUrl = cmt.Url | |
| } | |
| if currentJob.PullRequestNumber == nil { | |
| return false, false, fmt.Errorf("error while sending job comments: %v", err) | |
| } | |
| cmt, cmt_err := prService.PublishComment(*currentJob.PullRequestNumber, fmt.Sprintf(":yellow_circle: Warning: failed to post report for project %v, received error: %v.\n\n you may review details in the job logs", currentJob.ProjectName, err)) | |
| if cmt_err != nil { | |
| log.Printf("Error while posting error comment: %v", cmt_err) | |
| return false, false, fmt.Errorf("failed to post reporter error comment, aborting. Original error: %v, Comment error: %v", err, cmt_err) | |
| } | |
| jobPrCommentUrl = cmt.Url | |
| } |
When a plan is too large it could be larger than the allowed size allowed by github (66535 chars). Current behaviour was that digger would make the job fail and not post a comment to the PR causing confusion to the user especially in the apply phase. In this PR we catch error posting report and post a warning comment as below then proceed so the job would succeed despite this publishing failure
Summary by CodeRabbit