Skip to content
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

Add flag for setting annotation priority #2644

Merged
merged 2 commits into from Feb 22, 2024
Merged

Conversation

matthewborden
Copy link
Contributor

@matthewborden matthewborden commented Feb 20, 2024

Description

Introduce a new --priority flag for buildkite-agent annotate that allows setting a priority on annotations. This allows customers to influence the ordering of annotations. Annotations have a default priority of 3 and a manual priority can be set with buildkite-agent annotate --body "Test Annotation" --priority 5 to influence the ordering of annotations. GraphQL and Agent API changes have been merged to support this new flag.

There's a feature flag that enables the new ordering in the UI. We'll need to time enabling that feature flag with this agent version being released.

Fixes COMP-175.

Testing

I've added priority to the existing tests. I've also tested the feature against a locally running copy of Buildkite.

Here's a test running against the production Buildkite Agent API, with the changes compiled locally:

steps:
  - label: ":console: Annotation Test"
    command: |
      /usr/local/bin/buildkite-agent annotate 'Example 1`info` style' --style 'info' --context 'ctx-info'
      /usr/local/bin/buildkite-agent annotate 'Example 2`error` style' --style 'error' --context 'ctx-error2'
      /usr/local/bin/buildkite-agent annotate 'Example 3`warning` style' --style 'warning' --context 'ctx-warn'
      /usr/local/bin/buildkite-agent annotate 'Example 4`success` style' --style 'success' --context 'ctx-success'
      /usr/local/bin/buildkite-agent annotate 'Example 5 Priority 6 `error` style' --style 'error' --context 'ctx-error' --priority 6
      /usr/local/bin/buildkite-agent annotate 'Example 6 `default` style' --context 'ctx-default1'
      /usr/local/bin/buildkite-agent annotate 'Example 7 `default` style' --context 'ctx-default2'
      /usr/local/bin/buildkite-agent annotate 'Example 8 `default` style' --context 'ctx-default3'
      /usr/local/bin/buildkite-agent annotate 'Example 9 Priority 10 `default` style' --context 'ctx-default4' --priority 10
      /usr/local/bin/buildkite-agent annotate 'Example 10 Priority 10 `default` style' --context 'ctx-default5' --priority 10
      /usr/local/bin/buildkite-agent annotate 'Example 11 Priority 2 `default` style' --context 'ctx-default6' --priority 2
      /usr/local/bin/buildkite-agent annotate 'Example 12 `default` style' --context 'ctx-default7'
      /usr/local/bin/buildkite-agent annotate 'Example 12 `default` style' --context 'ctx-default8'
      /usr/local/bin/buildkite-agent annotate 'Example 13 `default` style' --context 'ctx-default9'
Screenshot 2024-02-19 at 8 24 56 pm

Allow setting a priority on annotations to allow customers to prioritise
the ordering of annotations. Annotations have a default priority of 3
and a manual priority can be set with `buildkite-agent annotate
--priority 3`.
@matthewborden matthewborden marked this pull request as ready for review February 20, 2024 04:36
@matthewborden matthewborden requested a review from a team February 20, 2024 04:52
Copy link
Contributor

@123sarahj123 123sarahj123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM , but might be best for Go engineer to check it over

clicommand/annotate.go Outdated Show resolved Hide resolved
@@ -94,6 +95,11 @@ var AnnotateCommand = cli.Command{
Usage: "Append to the body of an existing annotation",
EnvVar: "BUILDKITE_ANNOTATION_APPEND",
},
cli.IntFlag{
Name: "priority",
Usage: "Priority of the annotation (1 to 10), default is 3",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should mention that higher priority annotations will appear above lower ones in the UI. It might be obvious to some, but IMO --priority 1 is a bit ambiguous.

Copy link
Contributor

@triarius triarius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. You might want to add an explicit test, but I don't think tests would add much value here. I'm more concerned with bubbling up the error from the API to the job logs if a user invokes buildkite-agent annotate --body "Test Annotation" --priority 100.

Style: cfg.Style,
Context: cfg.Context,
Append: cfg.Append,
Priority: cfg.Priority,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to do any client side validation of priority? On one hand, it will be annoying to change if the API side bounds change. On the other hand, it is nice to avoid a request that's destined to error.

If you don't do client side validation, I think it's a good idea to check that the error message from the API is bubbled up to the job logs in an actionable manner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided against client side validation, it allows us to change the priority bounds and have older agents be able to pass the updated priorities.

I'm more concerned with bubbling up the error from the API to the job logs if a user invokes buildkite-agent annotate --body "Test Annotation" --priority 100.

Screenshot 2024-02-19 at 9 29 11 pm

Copy link
Contributor

@triarius triarius Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not very user-friendly, but I think it gets the point across.

Copy link
Contributor

@triarius triarius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔟

@matthewborden matthewborden merged commit fca9ac3 into main Feb 22, 2024
1 check passed
@matthewborden matthewborden deleted the annotation_priority branch February 22, 2024 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants