Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AGENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ Go CLI application with main packages:
- Formatting with `gofumpt` in extra mode: `go tool gofumpt -extra -w .`
- Struct-based configuration patterns (e.g., `AgentWorkerConfig`, `JobRunnerConfig`)
- Context-aware functions: `func Name(ctx context.Context, ...)`
- Import organization: stdlib, external deps, internal packages
- Import organization: stdlib, then everything else (gofumpt groups all non-stdlib imports together)
- Error handling: explicit errors, wrapped with context
- Naming: PascalCase for exported, camelCase for private, ALL_CAPS for constants
- Interface types end with -er suffix where appropriate
Expand Down
18 changes: 18 additions & 0 deletions api/meta_data.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ type MetaDataExists struct {
Exists bool `json:"exists"`
}

// MetaDataBatch represents a batch of key/value pairs for the set-batch endpoint.
type MetaDataBatch struct {
Items []MetaData `json:"items"`
}

// Sets the meta data value
func (c *Client) SetMetaData(ctx context.Context, jobId string, metaData *MetaData) (*Response, error) {
u := fmt.Sprintf("jobs/%s/data/set", railsPathEscape(jobId))
Expand All @@ -30,6 +35,19 @@ func (c *Client) SetMetaData(ctx context.Context, jobId string, metaData *MetaDa
return c.doRequest(req, nil)
}

// SetMetaDataBatch sets multiple meta data key/value pairs in a single request.
// The operation is transactional: all items succeed or none do.
func (c *Client) SetMetaDataBatch(ctx context.Context, jobId string, batch *MetaDataBatch) (*Response, error) {
u := fmt.Sprintf("jobs/%s/data/set-batch", railsPathEscape(jobId))

req, err := c.newRequest(ctx, "POST", u, batch)
if err != nil {
return nil, err
}

return c.doRequest(req, nil)
}

// Gets the meta data value
func (c *Client) GetMetaData(ctx context.Context, scope, id, key string) (*MetaData, *Response, error) {
if scope != "job" && scope != "build" {
Expand Down
1 change: 1 addition & 0 deletions clicommand/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ var BuildkiteAgentCommands = []cli.Command{
Usage: "Get/set metadata from Buildkite jobs",
Subcommands: []cli.Command{
MetaDataSetCommand,
MetaDataSetBatchCommand,
MetaDataGetCommand,
MetaDataExistsCommand,
MetaDataKeysCommand,
Expand Down
1 change: 1 addition & 0 deletions clicommand/config_completeness_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ var commandConfigPairs = []configCommandPair{
{Config: MetaDataGetConfig{}, Command: MetaDataGetCommand},
{Config: MetaDataKeysConfig{}, Command: MetaDataKeysCommand},
{Config: MetaDataSetConfig{}, Command: MetaDataSetCommand},
{Config: MetaDataSetBatchConfig{}, Command: MetaDataSetBatchCommand},
{Config: OIDCTokenConfig{}, Command: OIDCRequestTokenCommand},
{Config: PipelineUploadConfig{}, Command: PipelineUploadCommand},
{Config: RedactorAddConfig{}, Command: RedactorAddCommand},
Expand Down
147 changes: 147 additions & 0 deletions clicommand/meta_data_set_batch.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
package clicommand

import (
"context"
"errors"
"fmt"
"slices"
"strings"
"time"

"github.com/buildkite/agent/v3/api"
"github.com/buildkite/agent/v3/internal/redact"
"github.com/buildkite/agent/v3/logger"
"github.com/buildkite/roko"
"github.com/urfave/cli"
)

const metaDataSetBatchHelpDescription = `Usage:

buildkite-agent meta-data set-batch <key=value>... [options...]

Description:

Set multiple meta-data key/value pairs on a build in a single request.

Each argument must be in key=value format.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we support JSON input (over stdin)?

Copy link
Copy Markdown
Member Author

@pda pda Apr 1, 2026

Choose a reason for hiding this comment

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

Maybe!
Do you think that would change the CLI design? 🤔

Would it look like one/some of these…

buildkite-agent meta-data set-batch < meta-data.json

buildkite-agent meta-data set-batch --json < meta-data.json

buildkite-agent meta-data set-batch --from-file meta-data.json

I mostly didn't add it because it contained more decisions 😅

If we think the current argv approach needs to change to make room for JSON/stdin input, I'll revise it. But if we think there's room at add it later, I'd rather defer it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(In my initial use-case, I have a bash script coordinating some build concerns, and it's rounding up lots of information and then setting ~8 meta-data key=values. I could push that into JSON structure, perhaps using jq as a builder, but it's a much better fit to just build an array of key=value strings and put them on argv)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I foresee it being useful if something other than Bash is orchestrating. We have JSON input in some other commands. But I agree it can be done later.


Keys and values must be non-empty strings, and strings containing only
whitespace characters are not allowed.

Example:

$ buildkite-agent meta-data set-batch foo=bar "greeting=hello world"
$ buildkite-agent meta-data set-batch duration:spec/a.rb=2.341 duration:spec/b.rb=5.672`

type MetaDataSetBatchConfig struct {
GlobalConfig
APIConfig

Job string `cli:"job" validate:"required"`
RedactedVars []string `cli:"redacted-vars" normalize:"list"`
}

var MetaDataSetBatchCommand = cli.Command{
Name: "set-batch",
Usage: "Set multiple meta-data key/value pairs on a build",
Description: metaDataSetBatchHelpDescription,
Flags: slices.Concat(globalFlags(), apiFlags(), []cli.Flag{
cli.StringFlag{
Name: "job",
Value: "",
Usage: "Which job's build should the meta-data be set on",
EnvVar: "BUILDKITE_JOB_ID",
},
RedactedVars,
}),
Action: func(c *cli.Context) error {
ctx := context.Background()
ctx, cfg, l, _, done := setupLoggerAndConfig[MetaDataSetBatchConfig](ctx, c)
defer done()

args := c.Args()
if len(args) == 0 {
return errors.New("at least one key=value argument is required")
}

items, err := parseMetaDataBatchArgs(args)
if err != nil {
return err
}

needles, _, err := redact.NeedlesFromEnv(cfg.RedactedVars)
if err != nil {
return err
}

for i := range items {
if redactedValue := redact.String(items[i].Value, needles); redactedValue != items[i].Value {
l.Warnf("Meta-data value for key %q contained one or more secrets from environment variables that have been redacted. If this is deliberate, pass --redacted-vars='' or a list of patterns that does not match the variable containing the secret", items[i].Key)
items[i].Value = redactedValue
}
}

return setMetaDataBatch(ctx, cfg, l, items)
},
}

type invalidFormatError struct{ arg string }

func (e invalidFormatError) Error() string {
return fmt.Sprintf("invalid argument %q: must be in key=value format", e.arg)
}

type emptyKeyError struct{ arg string }

func (e emptyKeyError) Error() string {
return fmt.Sprintf("invalid argument %q: key cannot be empty, or composed of only whitespace characters", e.arg)
}

type emptyValueError struct{ arg string }

func (e emptyValueError) Error() string {
return fmt.Sprintf("invalid argument %q: value cannot be empty, or composed of only whitespace characters", e.arg)
}

func parseMetaDataBatchArgs(args []string) ([]api.MetaData, error) {
items := make([]api.MetaData, 0, len(args))
for _, arg := range args {
key, value, ok := strings.Cut(arg, "=")
if !ok {
return nil, invalidFormatError{arg: arg}
}
if strings.TrimSpace(key) == "" {
return nil, emptyKeyError{arg: arg}
}
if strings.TrimSpace(value) == "" {
return nil, emptyValueError{arg: arg}
}
Comment on lines +116 to +118
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we offer a way to "clear" a particular key? key= (nothing following) seems like a reasonable syntax for that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My robots suggest I reply like this… 😅

acknowledge it's a good CLI ergonomic, but argue that clearing meta-data should be designed end-to-end as its own feature (server + single + batch + CLI), not smuggled in via an empty-value convention. File a follow-up issue.

And yeah… I like the idea of being able to clear like that, but I'd love to defer it as out of scope for right now. I think it's a clean non-breaking-change path to disallow it now, and then allow it later (as opposed to the inverse).

Comment on lines +109 to +118
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can make it easier to check for specific errors in the tests without resorting to substring matching if these returned values of custom error types. One approach (another might be to wrap sentinel error values):

type invalidFormatError struct { arg string }

func (e invalidFormatError) Error() string { return fmt.Sprintf("invalid argument %q: must be in key=value format", e.arg)

type emptyKeyError struct { arg string }

func (e emptyKeyError) Error() string { return fmt.Sprintf("invalid argument %q: key cannot be empty, or composed of only whitespace characters", e.arg)

type emptyValueError struct { arg string }

func (e emptyValueError) Error() string { return fmt.Sprintf("invalid argument %q: value cannot be empty, or composed of only whitespace characters", e.arg)

...

		key, value, ok := strings.Cut(arg, "=")
		if !ok {
			return nil, invalidFormatError{arg}
		}
		if strings.TrimSpace(key) == "" {
			return nil, emptyKeyError{arg}
		}
		if strings.TrimSpace(value) == "" {
			return nil, emptyValueError{arg}
		}

...


		wantErr error

...

wantErr: invalidFormatError{arg: "foobar"},

...

if !errors.Is(err, tc.wantErr) { 
	t.Errorf("parseMetaDataBatchArgs(%v) error = %v, want %v", tc.args, err, tc.wantErr)
}

errors.Is would help tighten up the logic in the test - it wouldn't need to branch on tc.wantErr != "".

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Good call — applied in 3a94f79 using the custom-error-type variant you outlined (with the missing } typos fixed 😉). Each error type carries the offending arg, so errors.Is on a struct value verifies both the kind and the arg, and the test loop collapses to a single errors.Is(err, tc.wantErr) check (works for the happy path too, since errors.Is(nil, nil) is true).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Don't mind the sass from Amp/Opus there 😜

items = append(items, api.MetaData{Key: key, Value: value})
}
return items, nil
}

func setMetaDataBatch(ctx context.Context, cfg MetaDataSetBatchConfig, l logger.Logger, items []api.MetaData) error {
client := api.NewClient(l, loadAPIClientConfig(cfg, "AgentAccessToken"))

batch := &api.MetaDataBatch{Items: items}

if err := roko.NewRetrier(
roko.WithMaxAttempts(10),
roko.WithStrategy(roko.ExponentialSubsecond(2*time.Second)),
).DoWithContext(ctx, func(r *roko.Retrier) error {
resp, err := client.SetMetaDataBatch(ctx, cfg.Job, batch)
if resp != nil && (resp.StatusCode == 401 || resp.StatusCode == 404) {
r.Break()
Comment on lines +134 to +135
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Stop retrying unrecoverable 4xx batch validation errors

In setMetaDataBatch, retries are only disabled for 401/404, so other deterministic client-side failures (for example a 422 validation error from POST /jobs/:id/data/set-batch) are retried up to 10 times with exponential backoff before returning. That turns immediate user/input errors into long waits and makes this command appear hung for cases that can never succeed on retry; this path should break retries for unrecoverable 4xx responses (at least 400/422, while preserving retry for transient statuses like 429 if desired).

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Valid observation, but this intentionally matches the existing meta-data set command, which has the same retry behaviour (only breaks on 401/404). Broadening the no-retry set for 4xx could be a good improvement, but it should be done consistently across both commands.

Comment on lines +134 to +135
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Break on all non-retryable API failures

Fresh evidence since the earlier thread: the current meta-data set implementation calls api.BreakOnNonRetryable, whose repo doc treats every non-429 4xx and non-retryable network error as terminal. This new batch loop only breaks on 401/404, so deterministic set-batch validation failures such as 400/422 are retried through the full exponential backoff before returning instead of failing immediately; please use the same helper here to preserve the established retry semantics.

Useful? React with 👍 / 👎.

}
if err != nil {
l.Warnf("%s (%s)", err, r)
return err
}
return nil
}); err != nil {
return fmt.Errorf("failed to set meta-data batch: %w", err)
}

return nil
}
Loading