Delete old plan comments on PR update #1943
Conversation
WalkthroughThe changes include README formatting and instruction enhancements, addition of a nullable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Database
participant Logger
User->>Database: GetDiggerBatchesForPR(repoFullName, prNumber)
Database->>Database: Query DiggerBatch by repoFullName and prNumber
Database->>Logger: Log error if batch query fails
loop For each batch
Database->>Database: GetDiggerJobsForBatch(batchId)
Database->>Logger: Log error if job retrieval fails
end
Database->>Logger: Log info with PR number, repo, and batch count
Database-->>User: Return batches or error
sequenceDiagram
participant Service
participant GitHubAPI
Service->>Service: DeleteComment(id string)
Service->>GitHubAPI: DeleteComment(int64(id))
GitHubAPI-->>Service: Success/Error
Service-->>Caller: Return error if any
sequenceDiagram
participant Controller
participant Batch
participant PRService
participant BackendAPI
Controller->>Batch: Load DiggerConfig from batch
Batch->>Controller: Return config with DeletePriorComments flag
Controller->>Controller: Check batch status and type, config flag
alt Conditions met for deletion
Controller->>PRService: Get all batches for PR
PRService-->>Controller: Return batches
alt Any batch is apply command
Controller->>Controller: Abort deletion
else
loop For each batch except current
loop For each job in batch
Controller->>PRService: DeleteComment(job.PRCommentId)
PRService-->>Controller: Log error if any
end
end
end
else
Controller->>Controller: Log reason for skipping deletion
end
Possibly related PRs
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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. 🪧 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 (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces functionality to manage comment noise in pull requests by deleting old plan comments when PRs are updated, particularly beneficial for large monorepos.
- Added
DeleteCommentmethod ingithub.goto remove old plan comments via GitHub API - Introduced
PRCommentIdfield inDiggerJobstruct to track comment IDs for deletion - Added
GetDiggerJobsForPRfunction instorage.goto retrieve all jobs associated with a PR - Consider documenting when
PRCommentIdshould be null vs populated - Include
PRCommentIdinMapToJsonStruct()serialization for system-wide consistency
💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!
3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| _, err = svc.Client.Issues.DeleteComment(context.Background(), svc.Owner, svc.RepoName, commentId) | ||
| return err |
There was a problem hiding this comment.
style: Consider adding structured logging here to match the rest of the codebase's logging pattern
backend/models/storage.go
Outdated
| allJobs := make([]DiggerJob, 0) | ||
| for _, batch := range batches { | ||
| jobs, err := db.GetDiggerJobsForBatch(batch.ID) | ||
| if err != nil { | ||
| slog.Error("error fetching digger jobs for batch", | ||
| "batchId", batch.ID, | ||
| "prNumber", prNumber, | ||
| "error", err) | ||
| return nil, err | ||
| } | ||
| allJobs = append(allJobs, jobs...) | ||
| } |
There was a problem hiding this comment.
style: Consider using a single query with JOIN to fetch all jobs instead of multiple queries in a loop to improve performance
![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 (2)
README.md (1)
85-86: Improve grammar in the PostgreSQL SSL note.The phrasing could be improved for better clarity.
-Might need disabling ssl if running default docker image +You might need to disable SSL if running the default Docker image🧰 Tools
🪛 LanguageTool
[uncategorized] ~85-~85: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...irty ``` ## Local postgres Might need disabling ssl if running default docker image ``...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
backend/models/storage.go (1)
1842-1877: Well-implemented method to retrieve all jobs for a PR.The new
GetDiggerJobsForPRmethod effectively aggregates jobs across all batches for a specific PR, supporting the PR's objective of managing plan comments. The implementation includes proper error handling and detailed logging.Consider adding a limit or pagination option if PRs with many batches could lead to performance concerns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(5 hunks)backend/models/scheduler.go(2 hunks)backend/models/storage.go(1 hunks)libs/ci/github/github.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~85-~85: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...irty ``` ## Local postgres Might need disabling ssl if running default docker image ``...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
🪛 markdownlint-cli2 (0.17.2)
README.md
87-87: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (4)
backend/models/scheduler.go (1)
55-55: Added PRCommentId field to track comment associations.The addition of
PRCommentIdfield as a nullable*int64type allows for tracking which GitHub PR comments are associated with each digger job. This complements the new functionality to delete old plan comments on PR updates.libs/ci/github/github.go (1)
203-210: Good implementation of DeleteComment method following existing patterns.The implementation follows the same pattern as the existing
EditCommentmethod, with proper error handling for ID conversion and clear return of any GitHub API errors. This new method enables the deletion of old plan comments as described in the PR objectives.README.md (2)
80-81: Added important flag to migration command.Including the
--allow-dirtyflag in the migration command example provides better guidance for users running migrations.
88-88: Added useful example for DATABASE_URL with SSL disabled.This provides clear guidance for users working with local PostgreSQL instances, which is helpful for developers setting up their environment.
motatoes
force-pushed
the
feat/comment-tracking
branch
from
fca4d34 to
313aaa4
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
libs/ci/github/mocks.go (1)
102-104: Consider tracking comment deletion in the mockThis implementation satisfies the interface, but for a more complete mock, consider actually removing the comment from the
CommentsPerPrmap to more accurately simulate the real behavior.func (t MockCiService) DeleteComment(id string) error { + // Find and remove the comment with this ID from CommentsPerPr map + for prNumber, comments := range t.CommentsPerPr { + for i, c := range comments { + if c.Id == id { + // Remove the comment at index i + t.CommentsPerPr[prNumber] = append(comments[:i], comments[i+1:]...) + return nil + } + } + } return nil }libs/comment_utils/reporting/reporting_test.go (1)
102-104: Missing command tracking for DeleteComment method.The MockCiService implementation currently doesn't track the DeleteComment method calls like it does for other methods (e.g., PublishComment, EditComment, etc.). For consistency with the rest of the mock implementation, consider adding command tracking.
func (svc MockCiService) DeleteComment(id string) error { + t.CommentsPerPr = append(t.commands, "DeleteComment") return nil }backend/controllers/projects.go (4)
1458-1463: Incorrect log messageThe log message incorrectly states "Checking if PR should be auto-merged" but this function is for deleting older PR comments.
-slog.Info("Checking if PR should be auto-merged", +slog.Info("Checking if older PR comments should be deleted", "batchId", batch.ID, "prNumber", batch.PrNumber, "batchStatus", batch.Status, "batchType", batch.BatchType, )
1492-1495: Simplify boolean comparisonsThe boolean comparisons with
== truecan be simplified for better readability.-if (batch.Status == orchestrator_scheduler.BatchJobSucceeded || batch.Status == orchestrator_scheduler.BatchJobFailed) && - batch.BatchType == orchestrator_scheduler.DiggerCommandPlan && - batch.CoverAllImpactedProjects == true && - deleteOlderComments == true { +if (batch.Status == orchestrator_scheduler.BatchJobSucceeded || batch.Status == orchestrator_scheduler.BatchJobFailed) && + batch.BatchType == orchestrator_scheduler.DiggerCommandPlan && + batch.CoverAllImpactedProjects && + deleteOlderComments {🧰 Tools
🪛 golangci-lint (1.64.8)
1494-1494: S1002: should omit comparison to bool constant, can be simplified to
batch.CoverAllImpactedProjects(gosimple)
1495-1495: S1002: should omit comparison to bool constant, can be simplified to
deleteOlderComments(gosimple)
1497-1500: Incorrect log messageThe log message incorrectly refers to auto-merge instead of comment deletion.
-slog.Info("Conditions met for auto-merge, proceeding", +slog.Info("Conditions met for deleting older comments, proceeding", "batchId", batch.ID, "prNumber", batch.PrNumber, )
1511-1518: Incorrect error messageThe error message incorrectly refers to GitHub service instead of batch retrieval.
-slog.Error("Error getting PR service", +slog.Error("Error getting batches for PR", "batchId", batch.ID, "error", err, ) -return fmt.Errorf("error getting github service: %v", err) +return fmt.Errorf("error getting batches for PR: %v", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/controllers/projects.go(4 hunks)backend/models/storage.go(1 hunks)cli/pkg/digger/digger_test.go(1 hunks)libs/ci/azure/azure.go(1 hunks)libs/ci/bitbucket/bitbucket.go(1 hunks)libs/ci/ci.go(1 hunks)libs/ci/github/mocks.go(1 hunks)libs/ci/gitlab/gitlab.go(1 hunks)libs/ci/mocks.go(1 hunks)libs/comment_utils/reporting/reporting_test.go(1 hunks)libs/digger_config/config.go(1 hunks)libs/digger_config/converters.go(1 hunks)libs/digger_config/yaml.go(1 hunks)libs/orchestrator/mock.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- libs/digger_config/config.go
- libs/digger_config/yaml.go
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/models/storage.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
libs/ci/github/mocks.go (1)
libs/comment_utils/reporting/reporting_test.go (1)
MockCiService(9-11)
libs/comment_utils/reporting/reporting_test.go (1)
libs/ci/github/mocks.go (1)
MockCiService(9-11)
🪛 golangci-lint (1.64.8)
backend/controllers/projects.go
1494-1494: S1002: should omit comparison to bool constant, can be simplified to batch.CoverAllImpactedProjects
(gosimple)
1495-1495: S1002: should omit comparison to bool constant, can be simplified to deleteOlderComments
(gosimple)
1522-1522: SA9003: empty branch
(staticcheck)
🔇 Additional comments (6)
libs/ci/ci.go (1)
12-12: LGTM: Clean interface addition for comment deletionThe new
DeleteCommentmethod is a clear and logical extension to thePullRequestServiceinterface, positioned appropriately among other comment management methods.libs/ci/mocks.go (1)
74-76: LGTM: Implementation satisfies the interface requirementThe mock implementation correctly provides the required stubbed behavior for the
DeleteCommentmethod.cli/pkg/digger/digger_test.go (1)
158-160: LGTM: Proper mock implementationThe mock implementation correctly satisfies the interface requirement with appropriate return type.
libs/ci/gitlab/gitlab.go (1)
332-334: Implementation missing for DeleteComment.The method is a stub that always returns nil without performing any actual deletion operation or logging. Since this appears to be part of a larger feature to delete older PR comments, this implementation should be completed.
Consider adding a proper implementation similar to other methods in this file, or at minimum add a TODO comment indicating this needs implementation:
func (gitlabService GitLabService) DeleteComment(id string) error { - return nil + // TODO: implement deleting GitLab comments + slog.Debug("deleting comment (not implemented)", "commentId", id) + return nil }backend/controllers/projects.go (2)
495-495: LGTM: Added PrCommentId field to SetJobStatusRequestThis addition enables tracking of PR comment IDs for deletion later, which aligns with the PR's objective.
571-582: LGTM: Well-handled PR comment ID parsingThe code properly handles potential parsing errors when converting the string comment ID to int64, with appropriate logging and fallback.
| func (mockGithubPullrequestManager *MockGithubPullrequestManager) DeleteComment(id string) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing command tracking in mock implementation.
Unlike other methods in this mock that track command calls via the commands slice, the DeleteComment method doesn't track its invocation, which could affect test verifications.
func (mockGithubPullrequestManager *MockGithubPullrequestManager) DeleteComment(id string) error {
+ mockGithubPullrequestManager.commands = append(mockGithubPullrequestManager.commands, "DeleteComment")
return nil
}🤖 Prompt for AI Agents (early access)
In libs/orchestrator/mock.go around lines 90 to 92, the DeleteComment method in
the MockGithubPullrequestManager does not track its invocation in the commands
slice like other mock methods. Modify DeleteComment to append a descriptive
command string to the commands slice each time it is called, ensuring consistent
tracking of method calls for test verifications.
| func (a *AzureReposService) DeleteComment(id string) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unimplemented DeleteComment method.
The method is currently a stub that returns nil without performing any deletion operation. This is inconsistent with other implemented Azure methods like EditComment, which have proper implementations. Since this is part of a feature to delete PR comments, this implementation should be completed.
func (a *AzureReposService) DeleteComment(id string) error {
- return nil
+ threadId, err := strconv.Atoi(id)
+ if err != nil {
+ return fmt.Errorf("could not convert comment ID to int: %v", err)
+ }
+
+ _, err = a.Client.DeleteComment(context.Background(), git.DeleteCommentArgs{
+ Project: &a.ProjectName,
+ RepositoryId: &a.RepositoryId,
+ ThreadId: &threadId,
+ CommentId: &threadId, // You may need to adjust this depending on API requirements
+ })
+
+ if err != nil {
+ return fmt.Errorf("failed to delete comment: %v", err)
+ }
+
+ return nil
}Alternatively, if the API doesn't directly support comment deletion or if this is intended to be implemented later:
func (a *AzureReposService) DeleteComment(id string) error {
+ // TODO: implement comment deletion for Azure Repos
+ // API documentation: https://learn.microsoft.com/en-us/rest/api/azure/devops/git/pull-request-threads
return nil
}📝 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.
| func (a *AzureReposService) DeleteComment(id string) error { | |
| return nil | |
| } | |
| func (a *AzureReposService) DeleteComment(id string) error { | |
| threadId, err := strconv.Atoi(id) | |
| if err != nil { | |
| return fmt.Errorf("could not convert comment ID to int: %v", err) | |
| } | |
| _, err = a.Client.DeleteComment(context.Background(), git.DeleteCommentArgs{ | |
| Project: &a.ProjectName, | |
| RepositoryId: &a.RepositoryId, | |
| ThreadId: &threadId, | |
| CommentId: &threadId, // You may need to adjust this depending on API requirements | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to delete comment: %v", err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents (early access)
In libs/ci/azure/azure.go around lines 350 to 352, the DeleteComment method is
currently a stub returning nil without deleting any comment. Implement this
method to perform the actual deletion of a PR comment using the appropriate
Azure API call, similar to how EditComment is implemented. If the Azure API does
not support direct comment deletion, document this limitation clearly or plan
for a future implementation.
| func (b BitbucketAPI) DeleteComment(id string) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Stub method needs implementation
The DeleteComment method is currently just a stub that returns nil without any actual implementation. This doesn't fulfill the PR's objective of deleting old plan comments.
Implementation should follow the existing pattern of other methods in this file by:
- Constructing the appropriate Bitbucket API URL with the comment ID
- Sending a DELETE request
- Handling the response and potential errors
func (b BitbucketAPI) DeleteComment(id string) error {
- return nil
+ url := fmt.Sprintf("%s/repositories/%s/%s/pullrequests/comments/%s", bitbucketBaseURL, b.RepoWorkspace, b.RepoName, id)
+
+ resp, err := b.sendRequest("DELETE", url, nil)
+ if err != nil {
+ return err
+ }
+ defer resp.Body.Close()
+
+ if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusOK {
+ return fmt.Errorf("failed to delete comment. Status code: %d", resp.StatusCode)
+ }
+
+ return nil
}📝 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.
| func (b BitbucketAPI) DeleteComment(id string) error { | |
| return nil | |
| } | |
| func (b BitbucketAPI) DeleteComment(id string) error { | |
| url := fmt.Sprintf("%s/repositories/%s/%s/pullrequests/comments/%s", bitbucketBaseURL, b.RepoWorkspace, b.RepoName, id) | |
| resp, err := b.sendRequest("DELETE", url, nil) | |
| if err != nil { | |
| return err | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusOK { | |
| return fmt.Errorf("failed to delete comment. Status code: %d", resp.StatusCode) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents (early access)
In libs/ci/bitbucket/bitbucket.go around lines 186 to 188, the DeleteComment
method is a stub returning nil without performing any deletion. Implement this
method by constructing the Bitbucket API URL using the provided comment ID,
sending an HTTP DELETE request to that URL, and properly handling the response
and any errors, following the pattern used in other API methods in this file.
There was a problem hiding this comment.
PR Summary
(updates since last review)
Based on the PR changes and previous reviews, I'll focus on the most recent and important changes while avoiding repetition of points already covered:
Added functionality to delete old plan comments on PR updates to reduce noise in large monorepos.
- Added
DeleteOlderPRCommentsIfEnabledfunction in projects.go with incorrect log messages and boolean comparisons that need fixing - Incomplete
DeleteCommentimplementations in Azure, GitLab and Bitbucket services need proper implementation - Mock implementations for
DeleteCommentshould track deletions instead of returning nil - Migration adds
pr_comment_idcolumn to support comment tracking and deletion
The key issues are the incomplete VCS provider implementations and incorrect logging messages that need to be fixed. The core functionality is sound but needs consistent implementation across all providers.
24 file(s) reviewed, 15 comment(s)
Edit PR Review Bot Settings | Greptile
| if err != nil { | ||
|
|
||
| } |
There was a problem hiding this comment.
logic: Empty error handling block - need to handle the error case for GetDiggerJobsForBatch
| if err != nil { | |
| } | |
| if err != nil { | |
| slog.Error("Error getting jobs for batch", | |
| "batchId", prBatch.ID, | |
| "error", err, | |
| ) | |
| continue | |
| } |
backend/controllers/projects.go
Outdated
|
|
||
| } | ||
| for _, prJob := range jobs { | ||
| prService.DeleteComment(strconv.FormatInt(*prJob.PRCommentId, 10)) |
There was a problem hiding this comment.
logic: Potential nil pointer dereference if PRCommentId is nil
| prService.DeleteComment(strconv.FormatInt(*prJob.PRCommentId, 10)) | |
| if prJob.PRCommentId != nil { | |
| prService.DeleteComment(strconv.FormatInt(*prJob.PRCommentId, 10)) | |
| } |
backend/controllers/projects.go
Outdated
| slog.Info("Checking if PR should be auto-merged", | ||
| "batchId", batch.ID, |
There was a problem hiding this comment.
logic: Incorrect log message - says 'Checking if PR should be auto-merged' but function is about deleting comments
| slog.Info("Checking if PR should be auto-merged", | |
| "batchId", batch.ID, | |
| slog.Info("Checking if older PR comments should be deleted", | |
| "batchId", batch.ID, |
| slog.Error("Error getting PR service", | ||
| "batchId", batch.ID, | ||
| "error", err, | ||
| ) | ||
| return fmt.Errorf("error getting github service: %v", err) |
There was a problem hiding this comment.
logic: Error message incorrectly refers to 'github service' when it should be about getting PR batches
| slog.Error("Error getting PR service", | |
| "batchId", batch.ID, | |
| "error", err, | |
| ) | |
| return fmt.Errorf("error getting github service: %v", err) | |
| slog.Error("Error getting PR batches", | |
| "batchId", batch.ID, | |
| "error", err, | |
| ) | |
| return fmt.Errorf("error getting PR batches: %v", err) |
| "batchCount", len(batches), | ||
| "jobCount", len(batches)) |
There was a problem hiding this comment.
logic: Incorrect log field - jobCount is showing len(batches) which is the same as batchCount. Should be removed or replaced with actual job count.
| "batchCount", len(batches), | |
| "jobCount", len(batches)) | |
| "batchCount", len(batches)) |
| func (t MockPullRequestManager) DeleteComment(id string) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
style: The DeleteComment method only takes an id parameter but no prNumber. This is inconsistent with other PR-related methods in the interface which typically include prNumber. Consider whether prNumber should be included for consistency.
| func (gitlabService GitLabService) DeleteComment(id string) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
logic: This implementation is incomplete - it needs to use the GitLab API to actually delete the comment using the provided ID. Should use Client.Discussions.DeleteMergeRequestDiscussionNote() with the required parameters.
| func (gitlabService GitLabService) DeleteComment(id string) error { | |
| return nil | |
| } | |
| func (gitlabService GitLabService) DeleteComment(id string) error { | |
| projectId := *gitlabService.Context.ProjectId | |
| mergeRequestIID := *gitlabService.Context.MergeRequestIId | |
| discussionId := gitlabService.Context.DiscussionID | |
| id32, err := strconv.Atoi(id) | |
| if err != nil { | |
| slog.Error("could not convert comment ID to int", "error", err, "id", id) | |
| return fmt.Errorf("could not convert to int: %v", err) | |
| } | |
| _, err = gitlabService.Client.Discussions.DeleteMergeRequestDiscussionNote( | |
| projectId, mergeRequestIID, discussionId, id32) | |
| if err != nil { | |
| slog.Error("failed to delete comment", "error", err, "commentId", id) | |
| return fmt.Errorf("failed to delete comment: %v", err) | |
| } | |
| return nil | |
| } |
| func (mockGithubPullrequestManager *MockGithubPullrequestManager) DeleteComment(id string) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
logic: DeleteComment method should track command execution by appending to commands slice like other methods
| func (mockGithubPullrequestManager *MockGithubPullrequestManager) DeleteComment(id string) error { | |
| return nil | |
| } | |
| func (mockGithubPullrequestManager *MockGithubPullrequestManager) DeleteComment(id string) error { | |
| mockGithubPullrequestManager.commands = append(mockGithubPullrequestManager.commands, "DeleteComment") | |
| return nil | |
| } |
| if diggerYaml.DeletePriorComments != nil { | ||
| diggerConfig.DeletePriorComments = false | ||
| } else { | ||
| diggerConfig.DeletePriorComments = *diggerYaml.DeletePriorComments | ||
| } |
There was a problem hiding this comment.
logic: The logic here is inverted - when DeletePriorComments is explicitly set, it's forced to false, but when it's not set, it tries to dereference a nil pointer. This will cause a panic.
| if diggerYaml.DeletePriorComments != nil { | |
| diggerConfig.DeletePriorComments = false | |
| } else { | |
| diggerConfig.DeletePriorComments = *diggerYaml.DeletePriorComments | |
| } | |
| if diggerYaml.DeletePriorComments != nil { | |
| diggerConfig.DeletePriorComments = *diggerYaml.DeletePriorComments | |
| } else { | |
| diggerConfig.DeletePriorComments = false | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
backend/controllers/projects.go (4)
1457-1463: Incorrect log message in functionThe log message says "Checking if PR should be auto-merged" but this function is for deleting older PR comments.
- slog.Info("Checking if PR should be auto-merged", + slog.Info("Checking if older PR comments should be deleted", "batchId", batch.ID, "prNumber", batch.PrNumber, "batchStatus", batch.Status, "batchType", batch.BatchType, )
1513-1517: Incorrect error message when getting PR batchesThe error message incorrectly refers to "github service" when it's actually about getting PR batches.
- slog.Error("Error getting PR service", + slog.Error("Error getting PR batches", "batchId", batch.ID, "error", err, ) - return fmt.Errorf("error getting github service: %v", err) + return fmt.Errorf("error getting PR batches: %v", err)
1521-1525: Missing error handling in the GetDiggerJobsForBatch blockThere's an empty if block for error handling when getting jobs for batch.
jobs, err := models.DB.GetDiggerJobsForBatch(prBatch.ID) if err != nil { - + slog.Error("Error getting jobs for batch", + "batchId", prBatch.ID, + "error", err, + ) + continue }
1540-1548: Error handling needed for DeleteCommentThe code doesn't check for errors when calling
DeleteCommentand needs to check if PRCommentId is nil before dereferencing.- if prJob.PRCommentId == nil { - slog.Debug("PR comment not found for job, ignoring deletion", "JobID", prJob.ID) - continue - } - err = prService.DeleteComment(strconv.FormatInt(*prJob.PRCommentId, 10)) - if err != nil { - slog.Error("Could not delete comment for job", "jobID", prJob.ID, "commentID", prJob.PRCommentId, "error", err) - allDeletesSuccessful = false - } + if prJob.PRCommentId != nil { + err = prService.DeleteComment(strconv.FormatInt(*prJob.PRCommentId, 10)) + if err != nil { + slog.Error("Could not delete comment for job", + "jobID", prJob.ID, + "commentID", *prJob.PRCommentId, + "error", err) + allDeletesSuccessful = false + } else { + slog.Debug("Successfully deleted comment", + "jobID", prJob.ID, + "commentID", *prJob.PRCommentId) + } + } else { + slog.Debug("PR comment not found for job, ignoring deletion", "JobID", prJob.ID) + }
🧹 Nitpick comments (1)
backend/controllers/projects.go (1)
1494-1495: Simplify boolean comparisonsBoolean comparisons can be simplified by removing explicit comparison with
true.- if (batch.Status == orchestrator_scheduler.BatchJobSucceeded || batch.Status == orchestrator_scheduler.BatchJobFailed) && - batch.BatchType == orchestrator_scheduler.DiggerCommandPlan && - batch.CoverAllImpactedProjects == true && - deleteOlderComments == true { + if (batch.Status == orchestrator_scheduler.BatchJobSucceeded || batch.Status == orchestrator_scheduler.BatchJobFailed) && + batch.BatchType == orchestrator_scheduler.DiggerCommandPlan && + batch.CoverAllImpactedProjects && + deleteOlderComments {🧰 Tools
🪛 golangci-lint (1.64.8)
1494-1494: S1002: should omit comparison to bool constant, can be simplified to
batch.CoverAllImpactedProjects(gosimple)
1495-1495: S1002: should omit comparison to bool constant, can be simplified to
deleteOlderComments(gosimple)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
backend/controllers/projects.go(4 hunks)backend/migrations/20250512213729.sql(1 hunks)backend/models/scheduler.go(2 hunks)libs/digger_config/converters.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/migrations/20250512213729.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/digger_config/converters.go
- backend/models/scheduler.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
backend/controllers/projects.go
1494-1494: S1002: should omit comparison to bool constant, can be simplified to batch.CoverAllImpactedProjects
(gosimple)
1495-1495: S1002: should omit comparison to bool constant, can be simplified to deleteOlderComments
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (2)
backend/controllers/projects.go (2)
495-495: Field added to support comment deletion featureAdding the
PrCommentIdfield to track PR comments is necessary for the PR comment deletion feature.
571-582: Well-structured error handling for comment ID parsingThe code properly handles potential parsing errors when converting the string PR comment ID to an int64 pointer.
motatoes
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/controllers/projects.go (2)
1545-1554:⚠️ Potential issueMissing error handling for DeleteComment
The code doesn't check for errors when calling
DeleteComment. Errors should be properly logged and handled.- if prJob.PRCommentId == nil { - slog.Debug("PR comment not found for job, ignoring deletion", "JobID", prJob.ID) - continue - } - err = prService.DeleteComment(strconv.FormatInt(*prJob.PRCommentId, 10)) - if err != nil { - slog.Error("Could not delete comment for job", "jobID", prJob.ID, "commentID", prJob.PRCommentId, "error", err) - allDeletesSuccessful = false - } + if prJob.PRCommentId == nil { + slog.Debug("PR comment not found for job, ignoring deletion", "JobID", prJob.ID) + continue + } + commentId := strconv.FormatInt(*prJob.PRCommentId, 10) + err := prService.DeleteComment(commentId) + if err != nil { + slog.Warn("Failed to delete comment", + "commentId", commentId, + "jobId", prJob.DiggerJobID, + "error", err, + ) + allDeletesSuccessful = false + } else { + slog.Debug("Successfully deleted comment", + "commentId", commentId, + "jobId", prJob.DiggerJobID, + ) + }
1539-1544:⚠️ Potential issueMissing error handling in GetDiggerJobsForBatch
There's an empty error handling branch for
GetDiggerJobsForBatch. Error should be logged and execution should continue to the next batch.jobs, err := models.DB.GetDiggerJobsForBatch(prBatch.ID) if err != nil { - // won't return error here since can still continue deleting rest of batches + slog.Error("Error getting jobs for batch", + "batchId", prBatch.ID, + "error", err, + ) + continue }
🧹 Nitpick comments (5)
backend/controllers/projects.go (5)
571-583: Wrong variable in log messageThe log statement is referencing the
prCommentIdvariable (which might be nil) instead ofrequest.PrCommentId(the string value being parsed).- slog.Debug("could not parse commentID", "prCommentId", prCommentId, "error", err) + slog.Debug("could not parse commentID", "prCommentId", request.PrCommentId, "error", err)
1463-1464: Incorrect log messageThe log message states "Checking if PR should be auto-merged" but this function is about deleting old PR comments.
- slog.Info("Checking if PR should be auto-merged", + slog.Info("Checking if older PR comments should be deleted", "batchId", batch.ID,
1494-1500: Boolean comparisons can be simplifiedThe explicit comparison to
trueis unnecessary and can be simplified.- if (batch.Status == orchestrator_scheduler.BatchJobSucceeded || batch.Status == orchestrator_scheduler.BatchJobFailed) && - batch.BatchType == orchestrator_scheduler.DiggerCommandPlan && - batch.CoverAllImpactedProjects == true && - deleteOlderComments == true { + if (batch.Status == orchestrator_scheduler.BatchJobSucceeded || batch.Status == orchestrator_scheduler.BatchJobFailed) && + batch.BatchType == orchestrator_scheduler.DiggerCommandPlan && + batch.CoverAllImpactedProjects && + deleteOlderComments {🧰 Tools
🪛 golangci-lint (1.64.8)
1499-1499: S1002: should omit comparison to bool constant, can be simplified to
batch.CoverAllImpactedProjects(gosimple)
1500-1500: S1002: should omit comparison to bool constant, can be simplified to
deleteOlderComments(gosimple)
1502-1505: Incorrect log message about auto-mergeThis log message incorrectly states "Conditions met for auto-merge" when it should be about comment deletion.
- slog.Info("Conditions met for auto-merge, proceeding", + slog.Info("Conditions met for deleting older PR comments, proceeding", "batchId", batch.ID, "prNumber", batch.PrNumber, )
1518-1522: Incorrect error message referring to GitHub serviceThe error message incorrectly refers to "github service" when it's about getting PR batches.
- slog.Error("Error getting PR service", + slog.Error("Error getting PR batches", "batchId", batch.ID, "error", err, ) - return fmt.Errorf("error getting github service: %v", err) + return fmt.Errorf("error getting PR batches: %v", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/controllers/projects.go(5 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
backend/controllers/projects.go
1499-1499: S1002: should omit comparison to bool constant, can be simplified to batch.CoverAllImpactedProjects
(gosimple)
1500-1500: S1002: should omit comparison to bool constant, can be simplified to deleteOlderComments
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (3)
backend/controllers/projects.go (3)
495-495: LGTM: New field added to capture PR comment IDsThe
PrCommentIdfield added to theSetJobStatusRequeststruct will store the comment ID that can be later used for deletion, which aligns with the PR objective of deleting old plan comments.
531-531: LGTM: Proper logging of the new fieldAdding the PR comment ID to the log statement improves observability of the system.
837-840: LGTM: Properly calling the new functionThe new function is called at the right place in the workflow - after updating batch status and attempting auto-merge, with appropriate error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/controllers/projects.go (1)
1516-1523: 🛠️ Refactor suggestionIncorrect error message in PR batches retrieval
The error message incorrectly refers to GitHub service instead of PR batches.
prBatches, err := models.DB.GetDiggerBatchesForPR(batch.RepoFullName, batch.PrNumber) if err != nil { - slog.Error("Error getting PR service", + slog.Error("Error getting PR batches", "batchId", batch.ID, "error", err, ) - return fmt.Errorf("error getting github service: %v", err) + return fmt.Errorf("error getting PR batches: %v", err) }
🧹 Nitpick comments (3)
backend/controllers/projects.go (3)
1462-1476: Misleading log message in PR comment deletion functionThe log message incorrectly refers to auto-merge when the function is about deleting older PR comments.
- slog.Info("Checking if PR should have prior comments deleted", + slog.Info("Checking if older PR comments should be deleted", "batchId", batch.ID, "prNumber", batch.PrNumber, "batchStatus", batch.Status, "batchType", batch.BatchType, )
1499-1500: Simplify boolean comparisonsBoolean expressions can be simplified by omitting the comparison to
true.- if (batch.Status == orchestrator_scheduler.BatchJobSucceeded || batch.Status == orchestrator_scheduler.BatchJobFailed) && - batch.BatchType == orchestrator_scheduler.DiggerCommandPlan && - batch.CoverAllImpactedProjects == true && - deleteOlderComments == true { + if (batch.Status == orchestrator_scheduler.BatchJobSucceeded || batch.Status == orchestrator_scheduler.BatchJobFailed) && + batch.BatchType == orchestrator_scheduler.DiggerCommandPlan && + batch.CoverAllImpactedProjects && + deleteOlderComments {🧰 Tools
🪛 golangci-lint (1.64.8)
1499-1499: S1002: should omit comparison to bool constant, can be simplified to
batch.CoverAllImpactedProjects(gosimple)
1500-1500: S1002: should omit comparison to bool constant, can be simplified to
deleteOlderComments(gosimple)
1545-1554: Improve error handling for comment deletionThe code checks for nil PRCommentId but could improve error handling for the comment deletion operation with clearer logging.
if prJob.PRCommentId == nil { slog.Debug("PR comment not found for job, ignoring deletion", "JobID", prJob.ID) continue } - err = prService.DeleteComment(strconv.FormatInt(*prJob.PRCommentId, 10)) + commentID := strconv.FormatInt(*prJob.PRCommentId, 10) + err = prService.DeleteComment(commentID) if err != nil { - slog.Error("Could not delete comment for job", "jobID", prJob.ID, "commentID", prJob.PRCommentId, "error", err) + slog.Error("Could not delete comment for job", + "jobID", prJob.ID, + "commentID", commentID, + "batchID", prBatch.ID, + "error", err) allDeletesSuccessful = false + } else { + slog.Debug("Successfully deleted comment", "commentID", commentID, "jobID", prJob.ID) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/controllers/projects.go(5 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
backend/controllers/projects.go
1499-1499: S1002: should omit comparison to bool constant, can be simplified to batch.CoverAllImpactedProjects
(gosimple)
1500-1500: S1002: should omit comparison to bool constant, can be simplified to deleteOlderComments
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (4)
backend/controllers/projects.go (4)
495-495: Addition of Pull Request Comment ID fieldThe new
PrCommentIdfield in theSetJobStatusRequeststruct allows tracking and management of PR comments, which is essential for the "Delete old plan comments" functionality.
531-531: Good logging of the new PR comment ID fieldAdding the PR comment ID to the log statement enhances observability and debugging capabilities.
571-582: PR comment ID parsing and error handling looks goodThe code properly parses the string PR comment ID to an int64 pointer and handles conversion errors gracefully by setting it to nil with appropriate logging.
837-840: Good integration of the new comment deletion featureThe call to
DeleteOlderPRCommentsIfEnabledis appropriately placed after the auto-merge attempt and includes proper error logging without interrupting the main workflow.
| jobs, err := models.DB.GetDiggerJobsForBatch(prBatch.ID) | ||
| if err != nil { | ||
| slog.Error("could not get jobs for batch", "batchId", prBatch.ID, "error", err) | ||
| // won't return error here since can still continue deleting rest of batches | ||
| continue | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing error handling for GetDiggerJobsForBatch
There's an empty branch in the error handling for GetDiggerJobsForBatch.
jobs, err := models.DB.GetDiggerJobsForBatch(prBatch.ID)
if err != nil {
slog.Error("could not get jobs for batch", "batchId", prBatch.ID, "error", err)
+ allDeletesSuccessful = false
// won't return error here since can still continue deleting rest of batches
continue
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In backend/controllers/projects.go around lines 1538 to 1543, the error handling
for GetDiggerJobsForBatch logs the error but does not handle it beyond
continuing the loop. Improve this by adding appropriate error handling logic
such as returning the error or handling it in a way that prevents silent
failures, ensuring the function behaves correctly when the call fails.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/controllers/projects.go (3)
1517-1523:⚠️ Potential issueIncorrect error message for GetDiggerBatchesForPR
The error message incorrectly refers to 'github service' when it should be about getting PR batches.
slog.Error("Error getting PR service", "batchId", batch.ID, "error", err, ) - return fmt.Errorf("error getting github service: %v", err) + return fmt.Errorf("error getting PR batches: %v", err)
1544-1557: 🛠️ Refactor suggestionImprove error handling for DeleteComment
The code doesn't check for errors when calling
DeleteComment, which could lead to silent failures.for _, prJob := range jobs { if prJob.PRCommentId == nil { slog.Debug("PR comment not found for job, ignoring deletion", "JobID", prJob.ID) continue } // TODO: this delete will fail with 404 for all previous batches that already have been deleted // for now its okay but maybe better approach is only considering the most recent or have a marker on each batch // on whether or not its comments were deleted yet - err = prService.DeleteComment(strconv.FormatInt(*prJob.PRCommentId, 10)) + commentId := strconv.FormatInt(*prJob.PRCommentId, 10) + err = prService.DeleteComment(commentId) if err != nil { - slog.Error("Could not delete comment for job", "jobID", prJob.ID, "commentID", prJob.PRCommentId, "error", err) + slog.Error("Could not delete comment for job", "jobID", prJob.ID, "commentID", commentId, "batchId", prBatch.ID, "error", err) allDeletesSuccessful = false } + else { + slog.Debug("Successfully deleted comment", "commentId", commentId, "jobId", prJob.ID, "batchId", prBatch.ID) + } }
1538-1543: 🛠️ Refactor suggestionImprove error handling for GetDiggerJobsForBatch
The error handling for
GetDiggerJobsForBatchlogs the error but doesn't update the overall success tracking.jobs, err := models.DB.GetDiggerJobsForBatch(prBatch.ID) if err != nil { slog.Error("could not get jobs for batch", "batchId", prBatch.ID, "error", err) + allDeletesSuccessful = false // won't return error here since can still continue deleting rest of batches continue }
🧹 Nitpick comments (3)
backend/controllers/projects.go (3)
571-579: Improve error message when parsing comment IDThe debug log message incorrectly references the nil pointer variable
prCommentIdrather thanrequest.PrCommentId(the string being parsed), which could make debugging harder.- slog.Debug("could not parse commentID", "prCommentId", prCommentId, "error", err) + slog.Debug("could not parse commentID", "prCommentId", request.PrCommentId, "error", err)
1497-1500: Simplify boolean comparisonsBoolean expressions can be simplified by removing direct comparisons to
true.- if (batch.Status == orchestrator_scheduler.BatchJobSucceeded || batch.Status == orchestrator_scheduler.BatchJobFailed) && - batch.BatchType == orchestrator_scheduler.DiggerCommandPlan && - batch.CoverAllImpactedProjects == true && - deleteOlderComments == true { + if (batch.Status == orchestrator_scheduler.BatchJobSucceeded || batch.Status == orchestrator_scheduler.BatchJobFailed) && + batch.BatchType == orchestrator_scheduler.DiggerCommandPlan && + batch.CoverAllImpactedProjects && + deleteOlderComments {🧰 Tools
🪛 golangci-lint (1.64.8)
1499-1499: S1002: should omit comparison to bool constant, can be simplified to
batch.CoverAllImpactedProjects(gosimple)
1500-1500: S1002: should omit comparison to bool constant, can be simplified to
deleteOlderComments(gosimple)
1560-1562: Consider more informative warning messageThe warning message could be more informative by including details about the failure count.
if !allDeletesSuccessful { - slog.Warn("some of the previous comments failed to delete") + slog.Warn("some PR comments failed to delete", "batchId", batch.ID, "prNumber", batch.PrNumber) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/controllers/projects.go(5 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
backend/controllers/projects.go
1499-1499: S1002: should omit comparison to bool constant, can be simplified to batch.CoverAllImpactedProjects
(gosimple)
1500-1500: S1002: should omit comparison to bool constant, can be simplified to deleteOlderComments
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (3)
backend/controllers/projects.go (3)
495-495: Field addition for tracking PR comment IDsThis new field is essential for the PR's objective of deleting old comment IDs. Good addition!
837-840: Good implementation of new deletion functionalityThe call to
DeleteOlderPRCommentsIfEnabledis well-placed after batch status updates, and error handling is appropriate for this non-critical operation.
1462-1468:⚠️ Potential issueIncorrect log message in function
The log message says "Checking if PR should be auto-merged" but the function is about deleting comments.
- slog.Info("Checking if PR should have prior comments deleted", + slog.Info("Checking if older PR comments should be deleted",Likely an incorrect or invalid review comment.
* Add the ability to auto-nuke previous comments --------- Co-authored-by: motatoes <[email protected]>
Currently on every PR update a bunch of new plan comments are appended to the PR. This creates a lot of noise, especially in large monorepos.
New behavior: delete old plan comments (not summaries!) before starting new batch of plan jobs
Summary by CodeRabbit
Documentation
New Features
Other Changes