-
Notifications
You must be signed in to change notification settings - Fork 35
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
Fix: Truncate Buildkite Annotation #1645
Fix: Truncate Buildkite Annotation #1645
Conversation
🦋 Changeset detectedLatest commit: aa3c271 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Thanks for tackling this one! It's not a perfect solution (in the sense that cutting text off right at the end might result in strange output formats), but I don't think any solution is going to be perfect, sadly!
I've left a few suggestions and questions 😁
src/api/buildkite/annotate.ts
Outdated
@@ -44,6 +49,18 @@ export const annotate = async ( | |||
return; | |||
} | |||
|
|||
// Check if the annotation exceeds the maximum size | |||
const buffer = Buffer.from(markdown, 'utf-8'); |
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 might be missing something obvious! What's the reason to use a buffer here?
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.
As opposed to string.length
directly?
My understanding is that the buildkite annotation max is based on a byte size rather than a character length, so this is a more accurate comparison and would account for any characters that may be longer than a byte.
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.
Interesting, I'm not seeing the distinction drawn in https://buildkite.com/docs/agent/v3/cli-annotate
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.
Also, I think string length should be ok on that front:
$ node
> '🙂↔️' .length
5
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.
Oh wow! Did not know .length behaved like that.
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.
String.length
counts UTF-16 code units though. Can we confirm how Buildkite measures its limit?
Co-authored-by: Aaron Moat <2937187+AaronMoat@users.noreply.github.com>
Co-authored-by: Aaron Moat <2937187+AaronMoat@users.noreply.github.com>
…dieEK/skuba into longer-buildkite-annotation
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.
Looks wonderful! I'll fix that failing test in main
Description
When a Buildkite annotation is over
1 MiB
an error occurs.This PR truncates the buildkite annotation to avoid that error, while still logging the full message to the build log.
Motivation & Context
Resolves #1331
Discussion
MAX_SIZE
could potentially be passed in as optional attribute ofopts
if desired, would improve testability and possibly add some flexibility if max size ever needs to be different for different situations.