Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

dev/ci: introduce and migrate to bk.AnnotatedCmd #30951

Merged
merged 20 commits into from
Feb 11, 2022

Conversation

bobheadxi
Copy link
Member

@bobheadxi bobheadxi commented Feb 10, 2022

bk.AnnotatedCmd is the generator-based pull-oriented annotations API to replace sporadic calls to annotate.sh and buildkite-angent annotate, splitting out annotation creation concerns out of check scripts and into the pipeline generator in a style similar to how ArtifactPaths 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:

  • It should be simple enough that you can add annotations from any shell script → see usage below
  • Do not break the scripts if not run on the CI → biggest win here - we completely detach annotation commands from check scripts, and make annotations meaningful locally as well since you can find them in ./annotations. Potential sg check integration maybe? 👀
  • Define what is a good (and bad) annotation and document it. → with 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 of bk.AnnotatedCmd

Closes https://github.com/sourcegraph/sourcegraph/issues/26071

How to use it

In your script, leave a file in ./annotations:

if [ $EXIT_CODE -ne 0 ]; then
  echo -e "$OUT" >./annotations/docsite
fi

In your pipeline operation, replace bk.Cmd with bk.AnnotatedCmd:

	pipeline.AddStep(":memo: Check and build docsite",
		bk.AnnotatedCmd("./dev/check/docsite.sh", bk.AnnotatedCmdOpts{
			Type: bk.AnnotationTypeError,
		}))

That's it!

See https://buildkite.com/sourcegraph/sourcegraph/builds/130786:

Big screenshot image

Rules of the game:

  • You either create a MultiJobContext, or all annotations from a job go into a single annotation block.
  • For both MultiJobContext and default per-job annotations, annotations of different levels go into separate annotation blocks
  • Limited formatting options: content is either directly appended into annotation, or we include the name of the annotation file and include that as a heading.
    • .md files in annotations are treated as markdown content, otherwise we format them as terminal content

How it works

  1. annotated-command.sh, aliased as ./an, turns pipeline options into arguments and runs the given command
  2. Once the command has run, we then scan the ./annotations directory for files
  3. Pass each file to annotate.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.
  4. Done!

Benefits:

  • no more enterprise scripts being called in your check scripts
  • no more checking of $BUILDKITE or buildkite-agent
  • view annotations locally
  • easily see which jobs generate annotations

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.

@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Feb 10, 2022

Codenotify: Notifying subscribers in CODENOTIFY files for diff 6fd4ecc...549d345.

No notifications.

@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Feb 10, 2022

Codenotify: Notifying subscribers in OWNERS files for diff 6fd4ecc...549d345.

Notify File(s)
@sourcegraph/dev-experience enterprise/dev/ci/internal/buildkite/buildkite.go
enterprise/dev/ci/internal/ci/operations.go
enterprise/dev/ci/scripts/annotate.sh
enterprise/dev/ci/scripts/annotated-command.sh
enterprise/dev/ci/scripts/trace-command.sh
enterprise/dev/ci/scripts/upload-buildevent-report.sh

@bobheadxi bobheadxi force-pushed the main-dry-run/annotations-api branch from 545cd4d to c7f2ee1 Compare February 11, 2022 00:53
@bobheadxi bobheadxi force-pushed the main-dry-run/annotations-api branch from ca0d6a3 to 4cfdcbc Compare February 11, 2022 01:04
@bobheadxi bobheadxi changed the title dev/ci: introduce bk.AnnotatedCmd dev/ci: introduce and migrate to bk.AnnotatedCmd Feb 11, 2022
@bobheadxi bobheadxi requested a review from asdine February 11, 2022 01:59
Copy link
Contributor

@jhchabran jhchabran left a 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
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member Author

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

@bobheadxi
Copy link
Member Author

bobheadxi commented Feb 11, 2022

Don't we need to document it in the docs as well?

I've added a brief mention in https://github.com/sourcegraph/sourcegraph/pull/30951/commits/849c0b18b8a8f9e5eca14b84a41e482d13587b08 !

Comment on lines 255 to 262
type AnnotationType string

const (
AnnotationTypeSuccess AnnotationType = "success"
AnnotationTypeInfo AnnotationType = "info"
AnnotationTypeWarning AnnotationType = "warning"
AnnotationTypeError AnnotationType = "error"
)
Copy link
Member Author

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?

Copy link
Member Author

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

@bobheadxi bobheadxi merged commit 921a6fb into main Feb 11, 2022
@bobheadxi bobheadxi deleted the main-dry-run/annotations-api branch February 11, 2022 20:32
davejrt pushed a commit that referenced this pull request Feb 14, 2022
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.
davejrt pushed a commit that referenced this pull request Feb 15, 2022
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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dev/ci: create pipeline annotations API
3 participants