-
Notifications
You must be signed in to change notification settings - Fork 36
feat: Split comments if too long #436
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
base: main
Are you sure you want to change the base?
Conversation
|
Temporary image available at |
0cd5aa3 to
251ab64
Compare
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.
Pull Request Overview
Adds support for splitting long VCS comments into multiple chunks with backoff retries and proper link headers.
- Extended the
Clientinterface to include max comment length and PR link template, and updatedUpdateMessageto accept multiple message parts. - Implemented retry logic using exponential backoff in both GitLab and GitHub clients.
- Added sophisticated comment-splitting logic in
BuildCommentto honor VCS size limits and preserve code blocks.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vcs/types.go | Updated Client interface with new methods and changed UpdateMessage signature |
| pkg/vcs/gitlab_client/message.go | Added backoff retry and multi-part UpdateMessage implementation |
| pkg/vcs/github_client/message.go | Added backoff retry and multi-part UpdateMessage implementation |
| pkg/vcs/github_client/backoff.go | Added backoff configuration helper |
| pkg/msg/message.go | Enhanced BuildComment with splitting logic and code-block handling |
| mocks/vcs/mocks/mock_MockClient.go | Updated mocks for new interface methods and UpdateMessage signature |
Comments suppressed due to low confidence (4)
pkg/vcs/types.go:21
- The
UpdateMessagesignature was changed to accept a PullRequest and a slice of strings, but the doc comment wasn’t updated. Please update the comment to reflect the new parameters.
// UpdateMessage update a message with new content
pkg/vcs/types.go:41
- The comment refers to
GetPrLinkTemplatebut the method is namedGetPrCommentLinkTemplate. Update the comment for consistency.
// GetPrLinkTemplate returns the template for the PR link
pkg/msg/message.go:148
- [nitpick] The
BuildCommentfunction is quite large and handles many responsibilities (header/footer, splitting, code‐block preservation). Consider extracting the split logic into smaller helper functions to improve readability and testability.
// BuildComment iterates the map of all apps in this message, building a final comment from their current state
mocks/vcs/mocks/mock_MockClient.go:798
- [nitpick] The parameter name
stringsshadows the Gostringspackage and may be confusing. Consider renaming it tomessagesorcommentChunks.
func (_mock *MockClient) UpdateMessage(context1 context.Context, pullRequest vcs.PullRequest, message *msg.Message, strings []string) error {
|
@MeNsaaH While it works (that's positive!), 2nd part of the comment (overflow) remains expanded and does not look good. I think this should be wrapped with But I think this feature should cut at the application level (like this), wdyt? |
the issue is with github, the max comment limit is 65553 characters which is far less than what's on gitlab (1million chars). With github there are cases, where the diff of single resource (object) could span multiple comments. So, splitting by object-level level would be impossible |
da81a18 to
8a1059d
Compare
|
@Greyeye it's all ready now. when you get sometime could you test and review |
Greyeye
left a comment
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.
left a comment on the use of builtin function and BuildComment function.
|
@MeNsaaH you'll have to run |
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
Signed-off-by: Mmadu Manasseh <[email protected]>
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.
Pull Request Overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| var msg *string | ||
| msg = &check.Details |
Copilot
AI
Sep 14, 2025
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.
Using a pointer to string here is unnecessary complexity. The string can be handled directly since Go strings are already efficient for copying and slicing operations.
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.
I don't want to copy the string value because it could be a huge memory issue
Signed-off-by: Mmadu Manasseh <[email protected]>
Greyeye
left a comment
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.
LGTM
|
Is there a way we can use this branch? We're having exactly this issue for a +250 apps repo and we are in the middle of a migration and would love to see the impact of our PR changes 🤓 Edit: found how :) testing Edit 2: It works 👍 |
I'm seeing some cases where this causes huge memory usage on kubechecks. I want to refactor the code to put this under a flag |
Add feature to split comments across all vcs platforms while retaining codeblocks in between splits
Closes #421 #301 #299