-
Notifications
You must be signed in to change notification settings - Fork 1.3k
dev/ci: introduce and migrate to bk.AnnotatedCmd #30951
Conversation
Codenotify: Notifying subscribers in CODENOTIFY files for diff 6fd4ecc...549d345. No notifications. |
Codenotify: Notifying subscribers in OWNERS files for diff 6fd4ecc...549d345.
|
…ry-run/annotations-api
545cd4d
to
c7f2ee1
Compare
ca0d6a3
to
4cfdcbc
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.
Nice! The end result is really cool and much more obvious.
Don't we need to document it in the docs as well?
@@ -16,7 +16,7 @@ set -e | |||
echo -e "$OUT" | |||
|
|||
if [ $EXIT_CODE -ne 0 ]; then | |||
echo -e "$OUT" | ./enterprise/dev/ci/scripts/annotate.sh -s "docsite" | |||
echo -e "$OUT" >./annotations/docsite |
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.
Could we use the $BUILDKITE_JOB_ID
for that purpose? Could look like this:
echo -e "$OUT" >$ANNOTATION
And the pipeline generator would set $ANNOTATION
to ./annotation/$BUILDKITE_JOB_ID
or just use the script name if run locally.
WDYT?
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 an alternative, we could provide a link in the same way we did with ./tr
. Sometimes this makes me think we should extend the PATH to wrap those in a less magical way than with the symlinks.
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.
the pipeline generator would set $ANNOTATION to ./annotation/$BUILDKITE_JOB_ID
This implementation potentially uses the filename as a "title" with IncludeNames
so this is less than ideal. We also don't know ahead of time where $ANNOTATION
will be used, so the relative pathing might be funky, though linking as you mention might work around this
or just use the script name if run locally.
Building conditioning into scripts is something I wanted to avoid entirely with this new API - I'm aiming for something analogous to creating artifacts, where you just leave your content somewhere and that's it
I've added a brief mention in https://github.com/sourcegraph/sourcegraph/pull/30951/commits/849c0b18b8a8f9e5eca14b84a41e482d13587b08 ! |
type AnnotationType string | ||
|
||
const ( | ||
AnnotationTypeSuccess AnnotationType = "success" | ||
AnnotationTypeInfo AnnotationType = "info" | ||
AnnotationTypeWarning AnnotationType = "warning" | ||
AnnotationTypeError AnnotationType = "error" | ||
) |
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.
@jhchabran I'm thinking of only exporting warnings and errors in the API here, since we want to encourage only showing actionable things - a success is usually not actionable. Pipeline-level annotations like the build trace uploads can still set info and success levels directly. WDYT?
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've opted to disable the info/success types for now, since there are no usages, with a note that we can revisit this in the future: 549d345
bk.AnnotatedCmd is the generator-based pull-oriented annotations API to replace sporadic calls to annotate.sh and buildkite-agent annotate, splitting out annotation creation concerns out of check scripts and into the pipeline generator.
bk.AnnotatedCmd is the generator-based pull-oriented annotations API to replace sporadic calls to annotate.sh and buildkite-agent annotate, splitting out annotation creation concerns out of check scripts and into the pipeline generator.
bk.AnnotatedCmd
is the generator-based pull-oriented annotations API to replace sporadic calls toannotate.sh
andbuildkite-angent annotate
, splitting out annotation creation concerns out of check scripts and into the pipeline generator in a style similar to howArtifactPaths
works, but cooler and with way more bad bash from an engineer desperately copy-pasta-ing from Stackoverflow late at night (myself).This achieves all of the goals laid out by @jhchabran in https://github.com/sourcegraph/sourcegraph/issues/26071#issuecomment-952680939:
./annotations
. Potentialsg check
integration maybe? 👀bk.AnnotatedCmd
as the main touch point for users, I've included a big docstring there including this information which should make it easy to discover for users ofbk.AnnotatedCmd
Closes https://github.com/sourcegraph/sourcegraph/issues/26071
How to use it
In your script, leave a file in
./annotations
:In your pipeline operation, replace
bk.Cmd
withbk.AnnotatedCmd
:That's it!
See https://buildkite.com/sourcegraph/sourcegraph/builds/130786:
Big screenshot
Rules of the game:
MultiJobContext
, or all annotations from a job go into a single annotation block.MultiJobContext
and default per-job annotations, annotations of different levels go into separate annotation blocks.md
files inannotations
are treated as markdown content, otherwise we format them as terminal contentHow it works
annotated-command.sh
, aliased as./an
, turns pipeline options into arguments and runs the given command./annotations
directory for filesannotate.sh
with the generated arguments and some other parameters based on the name of the file, e.g. if it has a.md
extension or if we should include the file name.Benefits:
$BUILDKITE
orbuildkite-agent
Test plan
This PR is on a
main-dry-run
that runs everything. https://github.com/sourcegraph/sourcegraph/commit/7dafb63f601e39e025b181a829969a2abaa95af1 triggers intentional linter errors that will generate annotations. Security scanning annotations are always around, so we verify if that works.