From a2facd830ee80d25845e4d9dcd163ae64c33041f Mon Sep 17 00:00:00 2001 From: andridzi Date: Thu, 7 Nov 2024 10:24:54 +0200 Subject: [PATCH 1/9] feat: Split Github comments instead of trimming --- pkg/vcs/github_client/message.go | 134 ++++++++++++++++++++++++++----- 1 file changed, 116 insertions(+), 18 deletions(-) diff --git a/pkg/vcs/github_client/message.go b/pkg/vcs/github_client/message.go index 41064a7f..ef2066a1 100644 --- a/pkg/vcs/github_client/message.go +++ b/pkg/vcs/github_client/message.go @@ -17,6 +17,63 @@ import ( const MaxCommentLength = 64 * 1024 +const sepEnd = "\n```\n" + +"\n
\n\n**Warning**: Output length greater than max comment size. Continued in next comment." + +const sepStart = "Continued from previous comment.\n
Show Output\n\n" + +// SplitComment splits comment into a slice of comments that are under maxSize. +// It appends sepEnd to all comments that have a following comment. +// It prepends sepStart to all comments that have a preceding comment. +func SplitComment(comment string, maxSize int, sepEnd string, sepStart string) []string { + if len(comment) <= maxSize { + return []string{comment} + } + + var comments []string + var builder strings.Builder + remaining := comment + maxWithSep := maxSize - len(sepEnd) - len(sepStart) + sepStartWithCode := sepStart + "```diff\n" + + for len(remaining) > 0 { + if builder.Len() > 0 && builder.Len()+len(sepEnd) > maxWithSep { + comments = append(comments, builder.String()+sepEnd) + builder.Reset() + builder.WriteString(sepStartWithCode) + } + + lineEnd := strings.LastIndex(remaining[:min(len(remaining), maxWithSep-builder.Len())], "\n") + if lineEnd == -1 { + lineEnd = min(len(remaining), maxWithSep-builder.Len()) + } else { + lineEnd++ + } + + builder.WriteString(remaining[:lineEnd]) + remaining = remaining[lineEnd:] + + if builder.Len() >= maxWithSep { + comments = append(comments, builder.String()+sepEnd) + builder.Reset() + builder.WriteString(sepStartWithCode) + } + } + + if builder.Len() > 0 { + comments = append(comments, builder.String()) + } + + return comments +} + +func min(a, b int) int { + if a < b { + return a + } + return b +} + func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message string) *msg.Message { _, span := tracer.Start(ctx, "PostMessageToMergeRequest") defer span.End() @@ -26,6 +83,11 @@ func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message st message = message[:MaxCommentLength] } + if err := c.deleteLatestRunningComment(ctx, pr); err != nil { + log.Error().Err(err).Msg("failed to delete latest 'kubechecks running' comment") + return nil + } + log.Debug().Msgf("Posting message to PR %d in repo %s", pr.CheckID, pr.FullName) comment, _, err := c.googleClient.Issues.CreateComment( ctx, @@ -43,34 +105,70 @@ func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message st return msg.NewMessage(pr.FullName, pr.CheckID, int(*comment.ID), c) } -func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, msg string) error { +func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, message string) error { _, span := tracer.Start(ctx, "UpdateMessage") defer span.End() - if len(msg) > MaxCommentLength { - log.Warn().Int("original_length", len(msg)).Msg("trimming the comment size") - msg = msg[:MaxCommentLength] + comments := SplitComment(message, MaxCommentLength, sepEnd, sepStart) + repoNameComponents := strings.Split(m.Name, "/") + + pr := vcs.PullRequest{ + Owner: repoNameComponents[0], + Name: repoNameComponents[1], + CheckID: m.CheckID, + FullName: fmt.Sprintf("%s/%s", repoNameComponents[0], repoNameComponents[1]), } - log.Info().Msgf("Updating message for PR %d in repo %s", m.CheckID, m.Name) + log.Debug().Msgf("Updating message in PR %d in repo %s", pr.CheckID, pr.FullName) - repoNameComponents := strings.Split(m.Name, "/") - comment, resp, err := c.googleClient.Issues.EditComment( - ctx, - repoNameComponents[0], - repoNameComponents[1], - int64(m.NoteID), - &github.IssueComment{Body: &msg}, - ) + if err := c.deleteLatestRunningComment(ctx, pr); err != nil { + return err + } + for _, comment := range comments { + comment, _, err := c.googleClient.Issues.CreateComment( + ctx, + repoNameComponents[0], + repoNameComponents[1], + m.CheckID, + &github.IssueComment{Body: &comment}, + ) + if err != nil { + telemetry.SetError(span, err, "Update Pull Request comment") + log.Error().Err(err).Msg("could not post updated message comment to PR") + return err + } + m.NoteID = int(*comment.ID) + } + + return nil +} + +func (c *Client) deleteLatestRunningComment(ctx context.Context, pr vcs.PullRequest) error { + _, span := tracer.Start(ctx, "deleteLatestRunningComment") + repoNameComponents := strings.Split(pr.FullName, "/") + + existingComments, _, err := c.googleClient.Issues.ListComments(ctx, repoNameComponents[0], repoNameComponents[1], pr.CheckID, &github.IssueListCommentsOptions{ + Sort: pkg.Pointer("created"), + Direction: pkg.Pointer("asc"), + }) if err != nil { - telemetry.SetError(span, err, "Update Pull Request comment") - log.Error().Err(err).Msgf("could not update message to PR, msg: %s, response: %+v", msg, resp) - return err + telemetry.SetError(span, err, "List Pull Request comments") + log.Error().Err(err).Msg("could not retrieve existing comments for PR") + return fmt.Errorf("failed to list comments: %w", err) } - // update note id just in case it changed - m.NoteID = int(*comment.ID) + for _, existingComment := range existingComments { + if existingComment.Body != nil && strings.Contains(*existingComment.Body, ":hourglass: kubechecks running ... ") { + log.Debug().Msgf("Deleting 'kubechecks running' comment with ID %d", *existingComment.ID) + if _, err := c.googleClient.Issues.DeleteComment(ctx, repoNameComponents[0], repoNameComponents[1], *existingComment.ID); err != nil { + telemetry.SetError(span, err, "Delete Pull Request comment") + log.Error().Err(err).Msg("failed to delete 'kubechecks running' comment") + return fmt.Errorf("failed to delete 'kubechecks running' comment: %w", err) + } + break + } + } return nil } From d26f8415fb852d9bd24d13ed137f97c537cff810 Mon Sep 17 00:00:00 2001 From: andridzi Date: Wed, 13 Nov 2024 10:45:36 +0200 Subject: [PATCH 2/9] make `splitComment` unexportable --- pkg/vcs/github_client/message.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/vcs/github_client/message.go b/pkg/vcs/github_client/message.go index ef2066a1..102e806a 100644 --- a/pkg/vcs/github_client/message.go +++ b/pkg/vcs/github_client/message.go @@ -22,10 +22,10 @@ const sepEnd = "\n```\n
" + const sepStart = "Continued from previous comment.\n
Show Output\n\n" -// SplitComment splits comment into a slice of comments that are under maxSize. +// splitComment splits comment into a slice of comments that are under maxSize. // It appends sepEnd to all comments that have a following comment. // It prepends sepStart to all comments that have a preceding comment. -func SplitComment(comment string, maxSize int, sepEnd string, sepStart string) []string { +func splitComment(comment string, maxSize int, sepEnd string, sepStart string) []string { if len(comment) <= maxSize { return []string{comment} } @@ -109,7 +109,7 @@ func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, message stri _, span := tracer.Start(ctx, "UpdateMessage") defer span.End() - comments := SplitComment(message, MaxCommentLength, sepEnd, sepStart) + comments := splitComment(message, MaxCommentLength, sepEnd, sepStart) repoNameComponents := strings.Split(m.Name, "/") pr := vcs.PullRequest{ From 9f66c8a52bdb9c15262769cb16a6e47c7f69769d Mon Sep 17 00:00:00 2001 From: andridzi Date: Thu, 14 Nov 2024 10:30:53 +0200 Subject: [PATCH 3/9] resolve conflict --- pkg/vcs/github_client/message.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/vcs/github_client/message.go b/pkg/vcs/github_client/message.go index 102e806a..fa6430f6 100644 --- a/pkg/vcs/github_client/message.go +++ b/pkg/vcs/github_client/message.go @@ -74,7 +74,7 @@ func min(a, b int) int { return b } -func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message string) *msg.Message { +func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message string) (*msg.Message, error) { _, span := tracer.Start(ctx, "PostMessageToMergeRequest") defer span.End() From 1d92c7f0ed4612fa64442fdd15bbc269a6115af4 Mon Sep 17 00:00:00 2001 From: andridzi Date: Thu, 14 Nov 2024 12:17:58 +0200 Subject: [PATCH 4/9] fix return values --- pkg/vcs/github_client/message.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/vcs/github_client/message.go b/pkg/vcs/github_client/message.go index ab5f63b0..5e55116a 100644 --- a/pkg/vcs/github_client/message.go +++ b/pkg/vcs/github_client/message.go @@ -86,7 +86,7 @@ func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message st if err := c.deleteLatestRunningComment(ctx, pr); err != nil { log.Error().Err(err).Msg("failed to delete latest 'kubechecks running' comment") - return nil + return nil, err } log.Debug().Msgf("Posting message to PR %d in repo %s", pr.CheckID, pr.FullName) From 5bb5fd144f685406c8c292a2b0a6e85b35742640 Mon Sep 17 00:00:00 2001 From: "Volodymyr Shcherbinin (vovin)" Date: Mon, 3 Feb 2025 20:38:32 +0200 Subject: [PATCH 5/9] Refactor splitComment and add unit tests --- pkg/vcs/github_client/message.go | 91 +++++++------- pkg/vcs/github_client/nessage_test.go | 163 ++++++++++++++++++++++++++ 2 files changed, 210 insertions(+), 44 deletions(-) create mode 100644 pkg/vcs/github_client/nessage_test.go diff --git a/pkg/vcs/github_client/message.go b/pkg/vcs/github_client/message.go index 5e55116a..4cdc2f33 100644 --- a/pkg/vcs/github_client/message.go +++ b/pkg/vcs/github_client/message.go @@ -19,60 +19,63 @@ import ( const MaxCommentLength = 64 * 1024 const sepEnd = "\n```\n
" + -"\n
\n\n**Warning**: Output length greater than max comment size. Continued in next comment." + "\n
\n\n**Warning**: Output length greater than maximum allowed comment size. Continued in next comment." const sepStart = "Continued from previous comment.\n
Show Output\n\n" -// splitComment splits comment into a slice of comments that are under maxSize. -// It appends sepEnd to all comments that have a following comment. -// It prepends sepStart to all comments that have a preceding comment. +// splitComment splits the given comment into chunks from the beginning, +// ensuring that each decorated chunk does not exceed maxSize. +// - The first chunk has no prefix but, if not the only chunk, is suffixed with sepEnd. +// - Subsequent chunks are prefixed with sepStart. If they’re not final, they are also suffixed with sepEnd. +// This forward‐splitting approach preserves the beginning of the comment. func splitComment(comment string, maxSize int, sepEnd string, sepStart string) []string { + // Guard: If the comment fits in one chunk, return it unsplit. if len(comment) <= maxSize { return []string{comment} } - - var comments []string - var builder strings.Builder - remaining := comment - maxWithSep := maxSize - len(sepEnd) - len(sepStart) - sepStartWithCode := sepStart + "```diff\n" - - for len(remaining) > 0 { - if builder.Len() > 0 && builder.Len()+len(sepEnd) > maxWithSep { - comments = append(comments, builder.String()+sepEnd) - builder.Reset() - builder.WriteString(sepStartWithCode) - } - - lineEnd := strings.LastIndex(remaining[:min(len(remaining), maxWithSep-builder.Len())], "\n") - if lineEnd == -1 { - lineEnd = min(len(remaining), maxWithSep-builder.Len()) - } else { - lineEnd++ - } - - builder.WriteString(remaining[:lineEnd]) - remaining = remaining[lineEnd:] - - if builder.Len() >= maxWithSep { - comments = append(comments, builder.String()+sepEnd) - builder.Reset() - builder.WriteString(sepStartWithCode) - } + // Guard: if maxSize is too small to accommodate even one raw character with decorations, + // return the unsplit comment. + if maxSize < len(sepEnd)+1 || maxSize < len(sepStart)+1 { + return []string{comment} } - if builder.Len() > 0 { - comments = append(comments, builder.String()) + // Check if we have capacity for subsequent chunks + if maxSize-len(sepStart)-len(sepEnd) <= 0 { + // No room for raw text if we try to use both prefix and suffix + // => fallback to unsplit + return []string{comment} } - return comments -} + var parts []string + + // Process the first chunk. + // For the first chunk (if a split is needed) we reserve space for sepEnd only. + firstRawCapacity := maxSize - len(sepEnd) + firstChunkRaw := comment[0:firstRawCapacity] + parts = append(parts, firstChunkRaw+sepEnd) + i := firstRawCapacity -func min(a, b int) int { - if a < b { - return a + // Process subsequent chunks. + for i < len(comment) { + remaining := len(comment) - i + + // If the remaining text fits in one final chunk (with only the sepStart prefix), + // then create that final chunk without a trailing sepEnd. + if remaining <= maxSize-len(sepStart) { + parts = append(parts, sepStart+comment[i:]) + break + } else { + // Otherwise, for a non-final chunk, reserve space for both prefix and suffix. + rawCapacity := maxSize - len(sepStart) - len(sepEnd) + // The following slice is guaranteed to be in range because we only land here + // if remaining > maxSize - len(sepStart). Consequently, rawCapacity <= remaining, + // ensuring i+rawCapacity is within the comment's length. + chunk := sepStart + comment[i:i+rawCapacity] + sepEnd + parts = append(parts, chunk) + i += rawCapacity + } } - return b + return parts } func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message string) (*msg.Message, error) { @@ -114,9 +117,9 @@ func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, message stri repoNameComponents := strings.Split(m.Name, "/") pr := vcs.PullRequest{ - Owner: repoNameComponents[0], - Name: repoNameComponents[1], - CheckID: m.CheckID, + Owner: repoNameComponents[0], + Name: repoNameComponents[1], + CheckID: m.CheckID, FullName: fmt.Sprintf("%s/%s", repoNameComponents[0], repoNameComponents[1]), } diff --git a/pkg/vcs/github_client/nessage_test.go b/pkg/vcs/github_client/nessage_test.go new file mode 100644 index 00000000..1938e844 --- /dev/null +++ b/pkg/vcs/github_client/nessage_test.go @@ -0,0 +1,163 @@ +package github_client + +import ( + "reflect" + "strings" + "testing" +) + +func TestSplitComment(t *testing.T) { + t.Parallel() + + thousandA := strings.Repeat("a", 1000) + + tests := []struct { + name string + comment string + maxSize int + sepEnd string + sepStart string + want []string + }{ + { + name: "EmptyComment", + comment: "", + maxSize: 10, + sepEnd: "-E", + sepStart: "-S", + want: []string{""}, + }, + { + name: "ExactFit_NoSplit", + comment: "exact_fit", + maxSize: len("exact_fit"), + sepEnd: "-E", + sepStart: "-S", + want: []string{"exact_fit"}, + }, + { + name: "UnderMax_NoSplit", + comment: "comment under max size", + maxSize: len("comment under max size") + 1, + sepEnd: "sepEnd", + sepStart: "sepStart", + want: []string{"comment under max size"}, + }, + { + name: "TwoComments", + comment: thousandA, + // Force a split by choosing maxSize just under the full length. + // Calculation: + // For a 1000-character comment and maxSize = 1000 - 1 = 999: + // - The first chunk raw capacity = 999 - len(sepEnd). + // With sepEnd = "-sepEnd" (length 7), firstRawCapacity = 999 - 7 = 992. + // Thus, first chunk = thousandA[0:992] + "-sepEnd". + // - The remaining raw text is thousandA[992:1000] (8 characters). + // For the final chunk, we only need to add the prefix. + // Final chunk = sepStart + thousandA[992:]. + maxSize: len(thousandA) - 1, // 999 + sepEnd: "-sepEnd", + sepStart: "-sepStart", + want: func() []string { + firstRawCapacity := 999 - len("-sepEnd") // 999 - 7 = 992 + firstChunk := thousandA[:firstRawCapacity] + "-sepEnd" + secondChunk := "-sepStart" + thousandA[firstRawCapacity:] + return []string{firstChunk, secondChunk} + }(), + }, + { + name: "FourComments", + comment: thousandA, + sepEnd: "-sepEnd", + sepStart: "-sepStart", + // For splitting into four chunks: + // Set maxSize = (len(thousandA)/4) + len(sepEnd) + len(sepStart). + // For thousandA of length 1000, with sepEnd length 7 and sepStart length 9: + // maxSize = (1000/4) + 7 + 9 = 250 + 16 = 266. + // Then: + // - First chunk raw capacity = 266 - len(sepEnd) = 266 - 7 = 259. + // First chunk = thousandA[0:259] + "-sepEnd". + // - Subsequent non-final chunks have raw capacity = 266 - 9 - 7 = 250. + // - Second chunk = "-sepStart" + thousandA[259:259+250] + "-sepEnd". + // - Third chunk = "-sepStart" + thousandA[509:509+250] + "-sepEnd". + // - Final chunk = "-sepStart" + thousandA[759:]. + maxSize: (1000 / 4) + 7 + 9, // 266 + want: func() []string { + firstRawCapacity := 266 - 7 // 259 + chunk1 := thousandA[:firstRawCapacity] + "-sepEnd" + // For subsequent chunks, raw capacity = 266 - 9 - 7 = 250. + rawCapacity := 266 - 9 - 7 // 250 + chunk2 := "-sepStart" + thousandA[firstRawCapacity:firstRawCapacity+rawCapacity] + "-sepEnd" + chunk3 := "-sepStart" + thousandA[firstRawCapacity+rawCapacity:firstRawCapacity+2*rawCapacity] + "-sepEnd" + chunk4 := "-sepStart" + thousandA[firstRawCapacity+2*rawCapacity:] + return []string{chunk1, chunk2, chunk3, chunk4} + }(), + }, + { + name: "MaxSizeTooSmall_ReturnUnsplit", + comment: "Hello, world!", + // When maxSize is too small to fit even one raw character plus the decorations, + // the function should return the original unsplit comment. + // For example, if sepEnd = "ZZ" (length 2) and sepStart = "TOP" (length 3), + // then we require at least maxSize >= 2+1 = 3 for first chunk + // and maxSize >= 3+1 = 4 for subsequent chunks. + // Here we set maxSize to 5 (which is borderline) and expect unsplit output. + maxSize: 5, + sepEnd: "ZZ", + sepStart: "TOP", + want: []string{"Hello, world!"}, + }, + { + name: "MaxSizeTooSmall_UnsplitFallback", + comment: "abc", + // sepEnd="YYZ" => length=3 => we need at least 4 to store 1 raw char + suffix + // maxSize=2 => triggers the fallback to unsplit. + maxSize: 2, + sepEnd: "YYZ", + sepStart: "S", + want: []string{"abc"}, + }, + { + name: "NewlinesInComment", // Test with newlines to verify they are preserved. + comment: "line1\nline2\nline3", + maxSize: 20, // Comment fits unsplit. + sepEnd: "--E--", + sepStart: "--S--", + want: []string{"line1\nline2\nline3"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := splitComment(tt.comment, tt.maxSize, tt.sepEnd, tt.sepStart) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("%s:\n got: %#v\nwant: %#v", tt.name, got, tt.want) + } + }) + } +} + +// TestSplitComment_RealSeparators uses real production separator values and maximum comment length. +// This integration-style test verifies that with the actual production values: +// - No chunk exceeds MaxCommentLength. +// - The first chunk does not have the prefix (sepStart). +// - The final chunk does not have the suffix (sepEnd). +func TestSplitComment_RealSeparators(t *testing.T) { + // Create a long comment that exceeds MaxCommentLength. + comment := strings.Repeat("a", MaxCommentLength+100) + got := splitComment(comment, MaxCommentLength, sepEnd, sepStart) + // Verify that none of the chunks exceed MaxCommentLength. + for i, part := range got { + if len(part) > MaxCommentLength { + t.Errorf("Chunk %d exceeds MaxCommentLength: len=%d", i, len(part)) + } + } + // Verify that the first chunk does not have the sepStart prefix. + if len(got) > 0 && strings.HasPrefix(got[0], sepStart) { + t.Errorf("First chunk should not start with sepStart") + } + // Verify that the final chunk does not have the sepEnd suffix. + if len(got) > 0 && strings.HasSuffix(got[len(got)-1], sepEnd) { + t.Errorf("Final chunk should not end with sepEnd") + } +} From 925860c3c35ec070fed5d999f35c95a8ff36319d Mon Sep 17 00:00:00 2001 From: "Volodymyr Shcherbinin (vovin)" Date: Mon, 3 Feb 2025 20:54:20 +0200 Subject: [PATCH 6/9] Avoid variable shadowing --- pkg/vcs/github_client/message.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/vcs/github_client/message.go b/pkg/vcs/github_client/message.go index 4cdc2f33..cb3911e1 100644 --- a/pkg/vcs/github_client/message.go +++ b/pkg/vcs/github_client/message.go @@ -130,7 +130,7 @@ func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, message stri } for _, comment := range comments { - comment, _, err := c.googleClient.Issues.CreateComment( + cc, _, err := c.googleClient.Issues.CreateComment( ctx, repoNameComponents[0], repoNameComponents[1], @@ -142,7 +142,7 @@ func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, message stri log.Error().Err(err).Msg("could not post updated message comment to PR") return err } - m.NoteID = int(*comment.ID) + m.NoteID = int(*cc.ID) } return nil From f39b6009c0860f836427c1da63d38d2dc4962dc0 Mon Sep 17 00:00:00 2001 From: "Volodymyr Shcherbinin (vovin)" Date: Mon, 3 Feb 2025 23:28:16 +0200 Subject: [PATCH 7/9] itches and scratches --- pkg/vcs/github_client/message.go | 50 ++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/pkg/vcs/github_client/message.go b/pkg/vcs/github_client/message.go index cb3911e1..3e68cc78 100644 --- a/pkg/vcs/github_client/message.go +++ b/pkg/vcs/github_client/message.go @@ -109,18 +109,25 @@ func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message st return msg.NewMessage(pr.FullName, pr.CheckID, int(*comment.ID), c), nil } -func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, message string) error { +func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, msg string) error { _, span := tracer.Start(ctx, "UpdateMessage") defer span.End() - comments := splitComment(message, MaxCommentLength, sepEnd, sepStart) - repoNameComponents := strings.Split(m.Name, "/") + comments := splitComment(msg, MaxCommentLength, sepEnd, sepStart) + + owner, repo, ok := strings.Cut(m.Name, "/") + if !ok { + e := fmt.Errorf("invalid GitHub repository name: no '/' in %q", m.Name) + telemetry.SetError(span, e, "Invalid GitHub full repository name") + log.Error().Err(e).Msg("invalid GitHub repository name") + return e + } pr := vcs.PullRequest{ - Owner: repoNameComponents[0], - Name: repoNameComponents[1], + Owner: owner, + Name: repo, CheckID: m.CheckID, - FullName: fmt.Sprintf("%s/%s", repoNameComponents[0], repoNameComponents[1]), + FullName: m.Name, } log.Debug().Msgf("Updating message in PR %d in repo %s", pr.CheckID, pr.FullName) @@ -131,11 +138,7 @@ func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, message stri for _, comment := range comments { cc, _, err := c.googleClient.Issues.CreateComment( - ctx, - repoNameComponents[0], - repoNameComponents[1], - m.CheckID, - &github.IssueComment{Body: &comment}, + ctx, owner, repo, m.CheckID, &github.IssueComment{Body: &comment}, ) if err != nil { telemetry.SetError(span, err, "Update Pull Request comment") @@ -150,25 +153,28 @@ func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, message stri func (c *Client) deleteLatestRunningComment(ctx context.Context, pr vcs.PullRequest) error { _, span := tracer.Start(ctx, "deleteLatestRunningComment") - repoNameComponents := strings.Split(pr.FullName, "/") + defer span.End() - existingComments, _, err := c.googleClient.Issues.ListComments(ctx, repoNameComponents[0], repoNameComponents[1], pr.CheckID, &github.IssueListCommentsOptions{ - Sort: pkg.Pointer("created"), - Direction: pkg.Pointer("asc"), - }) + existingComments, resp, err := c.googleClient.Issues.ListComments( + ctx, pr.Owner, pr.Name, pr.CheckID, &github.IssueListCommentsOptions{ + Sort: pkg.Pointer("created"), + Direction: pkg.Pointer("asc"), + }, + ) if err != nil { telemetry.SetError(span, err, "List Pull Request comments") - log.Error().Err(err).Msg("could not retrieve existing comments for PR") + log.Error().Err(err).Msgf("could not retrieve existing PR comments, response: %+v", resp) return fmt.Errorf("failed to list comments: %w", err) } + // Find and delete the first running comment. for _, existingComment := range existingComments { if existingComment.Body != nil && strings.Contains(*existingComment.Body, ":hourglass: kubechecks running ... ") { log.Debug().Msgf("Deleting 'kubechecks running' comment with ID %d", *existingComment.ID) - if _, err := c.googleClient.Issues.DeleteComment(ctx, repoNameComponents[0], repoNameComponents[1], *existingComment.ID); err != nil { - telemetry.SetError(span, err, "Delete Pull Request comment") - log.Error().Err(err).Msg("failed to delete 'kubechecks running' comment") - return fmt.Errorf("failed to delete 'kubechecks running' comment: %w", err) + if r, e := c.googleClient.Issues.DeleteComment(ctx, pr.Owner, pr.Name, *existingComment.ID); e != nil { + telemetry.SetError(span, e, "Delete Pull Request comment") + log.Error().Err(e).Msgf("failed to delete 'kubechecks running' comment, response: %+v", r) + return fmt.Errorf("failed to delete 'kubechecks running' comment: %w", e) } break } @@ -206,7 +212,7 @@ func (c *Client) hideOutdatedMessages(ctx context.Context, pr vcs.PullRequest, c for _, comment := range comments { if strings.EqualFold(comment.GetUser().GetLogin(), c.username) { - // Github API does not expose minimizeComment API. IT's only available from the GraphQL API + // GitHub API does not expose minimizeComment API. IT's only available from the GraphQL API // https://docs.github.com/en/graphql/reference/mutations#minimizecomment var m struct { MinimizeComment struct { From f5bd5211461caed5f06b5a736ffcadc9677b742c Mon Sep 17 00:00:00 2001 From: "Volodymyr Shcherbinin (vovin)" Date: Mon, 3 Feb 2025 23:35:35 +0200 Subject: [PATCH 8/9] fix typo --- pkg/vcs/github_client/message.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/vcs/github_client/message.go b/pkg/vcs/github_client/message.go index 3e68cc78..fcb48377 100644 --- a/pkg/vcs/github_client/message.go +++ b/pkg/vcs/github_client/message.go @@ -212,7 +212,7 @@ func (c *Client) hideOutdatedMessages(ctx context.Context, pr vcs.PullRequest, c for _, comment := range comments { if strings.EqualFold(comment.GetUser().GetLogin(), c.username) { - // GitHub API does not expose minimizeComment API. IT's only available from the GraphQL API + // GitHub API does not expose minimizeComment API. It's only available from the GraphQL API // https://docs.github.com/en/graphql/reference/mutations#minimizecomment var m struct { MinimizeComment struct { From 042f102d5a8397664315965c40a9c1833aff47e7 Mon Sep 17 00:00:00 2001 From: "Volodymyr Shcherbinin (vovin)" Date: Tue, 4 Feb 2025 12:01:05 +0200 Subject: [PATCH 9/9] Add missing markdown header, improve readability --- pkg/vcs/github_client/message.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/pkg/vcs/github_client/message.go b/pkg/vcs/github_client/message.go index fcb48377..f8acc393 100644 --- a/pkg/vcs/github_client/message.go +++ b/pkg/vcs/github_client/message.go @@ -16,12 +16,20 @@ import ( "github.com/zapier/kubechecks/telemetry" ) -const MaxCommentLength = 64 * 1024 - -const sepEnd = "\n```\n
" + - "\n
\n\n**Warning**: Output length greater than maximum allowed comment size. Continued in next comment." - -const sepStart = "Continued from previous comment.\n
Show Output\n\n" +const ( + MaxCommentLength = 64 * 1024 + + // Using string concatenation for better code readability. + // Cannot use raw string literal (backticks) here as the separators themselves contain backticks. + sepStart = "Continued from previous comment.\n" + + "
Show Output\n\n" + + "```diff\n" + + sepEnd = "\n```\n" + + "
\n" + + "
\n\n" + + "**Warning**: Output length greater than maximum allowed comment size. Continued in next comment." +) // splitComment splits the given comment into chunks from the beginning, // ensuring that each decorated chunk does not exceed maxSize.