Conversation
Reports that exceed the VCS comment size limit (GitHub 64 KB, GitLab 1 MB) are now split into multiple comments at app-level boundaries, with byte-level fallback and markdown repair for oversized single sections. Each chunk carries a numbered header with the kubechecks identifier so multi-instance deployments and TidyOutdatedComments correctly manage comment groups. - Add KUBECHECKS_MAX_COMMENTS_PER_CHECK config (default 0/unlimited) - Add MaxCommentLength() to VCS Client interface - Refactor BuildComment to return []string via new SplitIntoChunks - Rewrite UpdateMessage to delete placeholder and post chunks - Add exponential backoff/retry for multi-chunk posting - Replace silent truncation in PostMessage with hard error - Regenerate mocks and docs Signed-off-by: Volodymyr Shcherbinin (vovin) <vovin@lurk.kyiv.ua>
5c99ac6 to
6119650
Compare
|
already tested, works perfectly! |
* main: fix(archive): use api.github.com zipball endpoint and fix App auth (zapier#532) build(deps): bump github.com/containerd/containerd from 1.7.30 to 1.7.33 (zapier#538) build(deps): bump golang.org/x/net from 0.38.0 to 0.55.0 in /tools/dump_crds (zapier#539) build(deps): bump google.golang.org/grpc from 1.81.1 to 1.82.1 (zapier#540) fix(diff): pass AppNamespace to ManagedResources query (zapier#535) build(deps): bump github.com/aws/aws-sdk-go-v2/service/s3 (zapier#508) build(deps): bump go dependencies (zapier#526) build(deps): bump github.com/argoproj/argo-cd/v3 from 3.2.1 to 3.2.11 (zapier#518) feat(archive): retry download on transient failures, surface typed er… (zapier#524) feat: expand wildcard Helm value file paths when packaging apps (zapier#520) feat: Ai review implementation (zapier#498) Clean up k8s versions (zapier#514) k8s versions are assumed to be major.minor.0 (zapier#512) chore: Local dev secrets management (zapier#501)
850e1de to
3853e00
Compare
|
@Greyeye @djeebus - a gentle nudge on this one. It continues to run without issue for us, and for @andridzi as well. There has been no feedback so far, so I would like to ask for a decision either way. To be explicit: "no, we do not want this" is a perfectly acceptable answer. We run it from a fork and can carry on doing so. I would simply prefer a clear decision to leaving the PR open indefinitely. If the approach is the concern rather than the feature itself, I am happy to rework it - please point me at what you would like changed. Many thanks. |
Re-posting everything notified on every run and left a gap with no report. A report that fits in one comment follows the same behaviour as before. Also fix fences and `<details>` left open at a cut, a size budget that forgot the truncation note, and the commit status being skipped when posting fails.
There was a problem hiding this comment.
🟡 Changes recommended
A compile-blocking missing import and unresolved report-splitting correctness issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds provider-aware splitting of oversized GitHub/GitLab reports into multiple comments while preserving single-comment output.
Changes:
- Adds markdown-safe report chunking and comment limits.
- Updates VCS clients with chunk posting and retries.
- Adds configuration, tests, mocks, documentation, and CLI wiring.
File summaries
| File | Description |
|---|---|
pkg/vcs/types.go |
Extends the VCS client contract. |
pkg/vcs/gitlab_client/message.go |
Posts GitLab report chunks. |
pkg/vcs/gitlab_client/message_test.go |
Tests GitLab chunk posting. |
pkg/vcs/github_client/retry.go |
Adds GitHub retry handling. |
pkg/vcs/github_client/retry_test.go |
Tests retry behavior. |
pkg/vcs/github_client/message.go |
Posts GitHub report chunks. |
pkg/vcs/github_client/message_test.go |
Tests GitHub posting behavior. |
pkg/vcs/github_client/client.go |
Adds comment retry configuration. |
pkg/vcs/github_client/archive.go |
Clarifies retry documentation. |
pkg/utils.go |
Defines the comment-count limit. |
pkg/msg/split.go |
Implements report chunking and markdown repair. |
pkg/msg/split_test.go |
Tests splitting behavior. |
pkg/msg/message.go |
Builds bounded report chunks. |
pkg/msg/message_test.go |
Tests report-building behavior. |
pkg/events/check.go |
Wires chunking and status handling. |
pkg/config/config.go |
Adds configuration and validation. |
pkg/config/config_test.go |
Tests configuration handling. |
mocks/vcs/mocks/mock_MockClient.go |
Updates generated VCS mocks. |
docs/usage.md |
Documents the new environment variable. |
cmd/root.go |
Adds the CLI configuration flag. |
Review details
Files not reviewed (1)
- mocks/vcs/mocks/mock_MockClient.go: Generated file
Suppressed comments (2)
pkg/msg/message.go:323
- The parser documentation says tags inside inline code and indented code are plain text, but this scan recognizes
<details>anywhere in the line. A literal`<details>`in check output will therefore mutates.details; after a cut, the generated close/reopen tags can hide or restructure the following content. Track code spans/indented blocks or restrict tag detection before updating the details stack.
case strings.HasPrefix(rest, "<details>"), strings.HasPrefix(rest, "<details "):
s.details = append(s.details, openDetails{})
case strings.HasPrefix(rest, "</details>") && len(s.details) > 0:
s.details = s.details[:len(s.details)-1]
}
pkg/msg/message.go:421
- When a details line exceeds the remaining budget, this branch replaces it with a truncated prefix and then advances to the next input line, discarding the unconsumed suffix. Minified JSON or a long diff line therefore still loses report content—the failure mode this change is intended to eliminate. Carry the remainder into the next part (while preserving the markdown state) instead of dropping it.
for room := maxLen - buf.Len() - after.closerLen(); len(line) > room; room = maxLen - buf.Len() - after.closerLen() {
if room < len(lineTruncated) {
line = lineTruncated
after = state
break
- Files reviewed: 19/20 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| import ( | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/rs/zerolog/log" | ||
|
|
||
| "github.com/zapier/kubechecks/pkg" | ||
| ) |
There was a problem hiding this comment.
slices.Clone lives in message.go, which imports it. split.go does not use it.
| var allSections []string | ||
| for _, appName := range names { | ||
| if m.isDeleted(appName) { | ||
| continue | ||
| } |
| func (cfg chunkConfig) sectionBudget() int { | ||
| head := len(chunkHeader(cfg.Identifier, pkg.MaxCommentsPerCheck, pkg.MaxCommentsPerCheck)) + len(continuedFrom) | ||
| tail := max(len(continuedIn), len(footerSeparator)+len(cfg.Footer)+len(truncatedNote)) | ||
|
|
||
| return cfg.MaxLength - head - tail |
A report bigger than the comment limit (GitHub 64 KiB, GitLab 1 MB) was cut at the limit, which lost the end of the report and left us with no hint that anything is missing (and usually broke the markdown too).
At times that heavily confused us and we had Atlantis as a good example of how it works nicely. So, now it is posted as several comments.
Behaviour for a report that fits in one comment does not change: the output is byte-identical to main (there is a test pinning that), and the placeholder comment is edited in place. Only the overflow is posted as new comments, each headed
Kubechecks <identifier> Report (Part N of M). Every run posts its own comments; comments from earlier runs, including all parts of a split report, are hidden or deleted by TidyOutdatedComments as before.How it splits: whole app sections first, then groups of checks, and only then the details of a single check, between lines. Fences and
<details>open at a cut are closed and reopened in the next part, so every comment renders on its own. The comment at the top ofpkg/msg/split.gowalks through it.Also in here:
KUBECHECKS_MAX_COMMENTS_PER_CHECK, 1 to 999, default 999 (part numbers are budgeted for three digits). We see 20-30 comments on large changes in production without rate-limit trouble, a low default would truncate those.vcs.Client.UpdateMessagetakes the PR, the note ID and the chunks;MaxCommentLength()is new;BuildCommenttakes an options struct.Notes:
checkReturnForBackofftreats every error except 429 as permanent. That was one call per report and is now one per comment, so a transient 5xx is more likely to cut a long report short. Happy to follow up separately.Easiest order to review:
pkg/vcs/types.goand the two clients, thenpkg/msg(split.gofirst), thengithub_client/retry.go, which stands on its own.Resolves #504.