From 1f1c167da2c8fd9d4708bb73af1bdc51dc5f994b Mon Sep 17 00:00:00 2001 From: Paul Annesley Date: Wed, 1 Apr 2026 23:40:14 +1030 Subject: [PATCH 1/2] Add meta-data set-batch command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a `buildkite-agent meta-data set-batch` command for setting multiple meta-data key/value pairs in a single transactional API request. All items succeed or none do. Usage: buildkite-agent meta-data set-batch "foo=bar" "baz=qux" Unlike `meta-data set`, values cannot be read from stdin — all key/value pairs are provided as key=value positional arguments. API: POST /jobs/{id}/data/set-batch with {"items": [{"key": ..., "value": ...}, ...]}. Response is 204 No Content on success. Changes: - api/meta_data.go: MetaDataBatch type and SetMetaDataBatch method - clicommand/meta_data_set_batch.go: CLI command with argument parsing, secret redaction, and retry logic matching `set` - clicommand/meta_data_set_batch_test.go: table-driven parsing tests and HTTP-level tests (success, server error, 401/404 no-retry, validation error) - AGENT.md: correct import organization docs to match gofumpt behavior (stdlib, then everything else in one group) Amp-Thread-ID: https://ampcode.com/threads/T-019d48e9-8854-7158-8dd7-16a883e56b84 Co-authored-by: Amp --- AGENT.md | 2 +- api/meta_data.go | 18 ++ clicommand/commands.go | 1 + clicommand/config_completeness_test.go | 1 + clicommand/meta_data_set_batch.go | 129 ++++++++++++++ clicommand/meta_data_set_batch_test.go | 232 +++++++++++++++++++++++++ 6 files changed, 382 insertions(+), 1 deletion(-) create mode 100644 clicommand/meta_data_set_batch.go create mode 100644 clicommand/meta_data_set_batch_test.go diff --git a/AGENT.md b/AGENT.md index b24bc6c5a4..0774fdfec7 100644 --- a/AGENT.md +++ b/AGENT.md @@ -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 diff --git a/api/meta_data.go b/api/meta_data.go index a9a591320b..ceff7d3751 100644 --- a/api/meta_data.go +++ b/api/meta_data.go @@ -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)) @@ -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" { diff --git a/clicommand/commands.go b/clicommand/commands.go index 80cf7f3a1d..cb81a46c91 100644 --- a/clicommand/commands.go +++ b/clicommand/commands.go @@ -99,6 +99,7 @@ var BuildkiteAgentCommands = []cli.Command{ Usage: "Get/set metadata from Buildkite jobs", Subcommands: []cli.Command{ MetaDataSetCommand, + MetaDataSetBatchCommand, MetaDataGetCommand, MetaDataExistsCommand, MetaDataKeysCommand, diff --git a/clicommand/config_completeness_test.go b/clicommand/config_completeness_test.go index 238cbb82c9..ef4a7a20ba 100644 --- a/clicommand/config_completeness_test.go +++ b/clicommand/config_completeness_test.go @@ -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}, diff --git a/clicommand/meta_data_set_batch.go b/clicommand/meta_data_set_batch.go new file mode 100644 index 0000000000..d22e3c3354 --- /dev/null +++ b/clicommand/meta_data_set_batch.go @@ -0,0 +1,129 @@ +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 ... [options...] + +Description: + +Set multiple meta-data key/value pairs on a build in a single request. + +Each argument must be in key=value format. + +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) + }, +} + +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, fmt.Errorf("invalid argument %q: must be in key=value format", arg) + } + if strings.TrimSpace(key) == "" { + return nil, fmt.Errorf("invalid argument %q: key cannot be empty, or composed of only whitespace characters", arg) + } + if strings.TrimSpace(value) == "" { + return nil, fmt.Errorf("invalid argument %q: value cannot be empty, or composed of only whitespace characters", arg) + } + 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() + } + 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 +} diff --git a/clicommand/meta_data_set_batch_test.go b/clicommand/meta_data_set_batch_test.go new file mode 100644 index 0000000000..827dc0c616 --- /dev/null +++ b/clicommand/meta_data_set_batch_test.go @@ -0,0 +1,232 @@ +package clicommand + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/buildkite/agent/v3/api" + "github.com/buildkite/agent/v3/logger" + "github.com/google/go-cmp/cmp" +) + +func TestParseMetaDataBatchArgs(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args []string + want []api.MetaData + wantErr string + }{ + { + name: "single pair", + args: []string{"foo=bar"}, + want: []api.MetaData{{Key: "foo", Value: "bar"}}, + }, + { + name: "multiple pairs", + args: []string{"a=1", "b=2", "c=3"}, + want: []api.MetaData{ + {Key: "a", Value: "1"}, + {Key: "b", Value: "2"}, + {Key: "c", Value: "3"}, + }, + }, + { + name: "value containing equals sign", + args: []string{"key=val=ue"}, + want: []api.MetaData{{Key: "key", Value: "val=ue"}}, + }, + { + name: "missing equals sign", + args: []string{"foobar"}, + wantErr: `invalid argument "foobar": must be in key=value format`, + }, + { + name: "empty key", + args: []string{"=bar"}, + wantErr: `invalid argument "=bar": key cannot be empty`, + }, + { + name: "whitespace-only key", + args: []string{" =bar"}, + wantErr: `invalid argument " =bar": key cannot be empty`, + }, + { + name: "empty value", + args: []string{"foo="}, + wantErr: `invalid argument "foo=": value cannot be empty`, + }, + { + name: "whitespace-only value", + args: []string{"foo= "}, + wantErr: `invalid argument "foo= ": value cannot be empty`, + }, + { + name: "error on first invalid stops parsing", + args: []string{"a=1", "bad", "c=3"}, + wantErr: `invalid argument "bad": must be in key=value format`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got, err := parseMetaDataBatchArgs(tc.args) + if tc.wantErr != "" { + if err == nil { + t.Fatalf("parseMetaDataBatchArgs(%v) error = nil, want error containing %q", tc.args, tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("parseMetaDataBatchArgs(%v) error = %q, want error containing %q", tc.args, err, tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("parseMetaDataBatchArgs(%v) error = %v, want nil", tc.args, err) + } + if diff := cmp.Diff(got, tc.want); diff != "" { + t.Fatalf("parseMetaDataBatchArgs(%v) diff (-got +want):\n%s", tc.args, diff) + } + }) + } +} + +func TestSetMetaDataBatch(t *testing.T) { + t.Parallel() + + t.Run("success", func(t *testing.T) { + t.Parallel() + + var receivedBatch api.MetaDataBatch + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + if req.Method != "POST" { + t.Errorf("req.Method = %q, want %q", req.Method, "POST") + } + if want := "/jobs/jobid/data/set-batch"; req.URL.Path != want { + t.Errorf("req.URL.Path = %q, want %q", req.URL.Path, want) + } + if err := json.NewDecoder(req.Body).Decode(&receivedBatch); err != nil { + t.Errorf("decoding request body: %v", err) + } + rw.WriteHeader(http.StatusNoContent) + })) + defer server.Close() + + cfg := MetaDataSetBatchConfig{ + Job: "jobid", + APIConfig: APIConfig{ + AgentAccessToken: "agentaccesstoken", + Endpoint: server.URL, + }, + } + + items := []api.MetaData{ + {Key: "foo", Value: "bar"}, + {Key: "baz", Value: "qux"}, + } + + l := logger.NewBuffer() + if err := setMetaDataBatch(t.Context(), cfg, l, items); err != nil { + t.Fatalf("setMetaDataBatch error = %v, want nil", err) + } + if diff := cmp.Diff(receivedBatch.Items, items); diff != "" { + t.Errorf("receivedBatch.Items diff (-got +want):\n%s", diff) + } + }) + + t.Run("server error gives up when context is cancelled", func(t *testing.T) { + t.Parallel() + + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + rw.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + cfg := MetaDataSetBatchConfig{ + Job: "jobid", + APIConfig: APIConfig{ + AgentAccessToken: "agentaccesstoken", + Endpoint: server.URL, + }, + } + + items := []api.MetaData{{Key: "a", Value: "1"}} + + ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond) + defer cancel() + + l := logger.NewBuffer() + err := setMetaDataBatch(ctx, cfg, l, items) + if err == nil { + t.Fatal("setMetaDataBatch error = nil, want error") + } + if want := "failed to set meta-data batch"; !strings.Contains(err.Error(), want) { + t.Errorf("setMetaDataBatch error = %q, want error containing %q", err, want) + } + }) + + t.Run("401 does not retry", func(t *testing.T) { + t.Parallel() + + callCount := 0 + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + callCount++ + rw.WriteHeader(http.StatusUnauthorized) + })) + defer server.Close() + + cfg := MetaDataSetBatchConfig{ + Job: "jobid", + APIConfig: APIConfig{ + AgentAccessToken: "agentaccesstoken", + Endpoint: server.URL, + }, + } + + items := []api.MetaData{{Key: "a", Value: "1"}} + + l := logger.NewBuffer() + if err := setMetaDataBatch(t.Context(), cfg, l, items); err == nil { + t.Fatal("setMetaDataBatch error = nil, want error") + } + if callCount != 1 { + t.Errorf("callCount = %d, want 1", callCount) + } + }) + + t.Run("404 does not retry", func(t *testing.T) { + t.Parallel() + + callCount := 0 + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + callCount++ + rw.WriteHeader(http.StatusNotFound) + })) + defer server.Close() + + cfg := MetaDataSetBatchConfig{ + Job: "jobid", + APIConfig: APIConfig{ + AgentAccessToken: "agentaccesstoken", + Endpoint: server.URL, + }, + } + + items := []api.MetaData{{Key: "a", Value: "1"}} + + l := logger.NewBuffer() + if err := setMetaDataBatch(t.Context(), cfg, l, items); err == nil { + t.Fatal("setMetaDataBatch error = nil, want error") + } + if callCount != 1 { + t.Errorf("callCount = %d, want 1", callCount) + } + }) +} From 3a94f79703a21d3e5e352dcdcf4ccf687ac2fbe2 Mon Sep 17 00:00:00 2001 From: Paul Annesley Date: Wed, 27 May 2026 16:40:50 +0930 Subject: [PATCH 2/2] Use custom error types in parseMetaDataBatchArgs Per @DrJosh9000's review on #3791, replace ad-hoc fmt.Errorf strings with three named error types (invalidFormatError, emptyKeyError, emptyValueError) carrying the offending arg. Tests use errors.Is against typed values, removing the brittle substring matching and the wantErr != "" branch. Amp-Thread-ID: https://ampcode.com/threads/T-019e6840-8988-7410-b2d8-a7fd594cd920 Co-authored-by: Amp --- clicommand/meta_data_set_batch.go | 24 +++++++++++++++++++--- clicommand/meta_data_set_batch_test.go | 28 +++++++++----------------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/clicommand/meta_data_set_batch.go b/clicommand/meta_data_set_batch.go index d22e3c3354..12ea90f5c5 100644 --- a/clicommand/meta_data_set_batch.go +++ b/clicommand/meta_data_set_batch.go @@ -85,18 +85,36 @@ var MetaDataSetBatchCommand = cli.Command{ }, } +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, fmt.Errorf("invalid argument %q: must be in key=value format", arg) + return nil, invalidFormatError{arg: arg} } if strings.TrimSpace(key) == "" { - return nil, fmt.Errorf("invalid argument %q: key cannot be empty, or composed of only whitespace characters", arg) + return nil, emptyKeyError{arg: arg} } if strings.TrimSpace(value) == "" { - return nil, fmt.Errorf("invalid argument %q: value cannot be empty, or composed of only whitespace characters", arg) + return nil, emptyValueError{arg: arg} } items = append(items, api.MetaData{Key: key, Value: value}) } diff --git a/clicommand/meta_data_set_batch_test.go b/clicommand/meta_data_set_batch_test.go index 827dc0c616..67bee80371 100644 --- a/clicommand/meta_data_set_batch_test.go +++ b/clicommand/meta_data_set_batch_test.go @@ -3,6 +3,7 @@ package clicommand import ( "context" "encoding/json" + "errors" "net/http" "net/http/httptest" "strings" @@ -21,7 +22,7 @@ func TestParseMetaDataBatchArgs(t *testing.T) { name string args []string want []api.MetaData - wantErr string + wantErr error }{ { name: "single pair", @@ -45,32 +46,32 @@ func TestParseMetaDataBatchArgs(t *testing.T) { { name: "missing equals sign", args: []string{"foobar"}, - wantErr: `invalid argument "foobar": must be in key=value format`, + wantErr: invalidFormatError{arg: "foobar"}, }, { name: "empty key", args: []string{"=bar"}, - wantErr: `invalid argument "=bar": key cannot be empty`, + wantErr: emptyKeyError{arg: "=bar"}, }, { name: "whitespace-only key", args: []string{" =bar"}, - wantErr: `invalid argument " =bar": key cannot be empty`, + wantErr: emptyKeyError{arg: " =bar"}, }, { name: "empty value", args: []string{"foo="}, - wantErr: `invalid argument "foo=": value cannot be empty`, + wantErr: emptyValueError{arg: "foo="}, }, { name: "whitespace-only value", args: []string{"foo= "}, - wantErr: `invalid argument "foo= ": value cannot be empty`, + wantErr: emptyValueError{arg: "foo= "}, }, { name: "error on first invalid stops parsing", args: []string{"a=1", "bad", "c=3"}, - wantErr: `invalid argument "bad": must be in key=value format`, + wantErr: invalidFormatError{arg: "bad"}, }, } @@ -79,17 +80,8 @@ func TestParseMetaDataBatchArgs(t *testing.T) { t.Parallel() got, err := parseMetaDataBatchArgs(tc.args) - if tc.wantErr != "" { - if err == nil { - t.Fatalf("parseMetaDataBatchArgs(%v) error = nil, want error containing %q", tc.args, tc.wantErr) - } - if !strings.Contains(err.Error(), tc.wantErr) { - t.Fatalf("parseMetaDataBatchArgs(%v) error = %q, want error containing %q", tc.args, err, tc.wantErr) - } - return - } - if err != nil { - t.Fatalf("parseMetaDataBatchArgs(%v) error = %v, want nil", tc.args, err) + if !errors.Is(err, tc.wantErr) { + t.Fatalf("parseMetaDataBatchArgs(%v) error = %v, want %v", tc.args, err, tc.wantErr) } if diff := cmp.Diff(got, tc.want); diff != "" { t.Fatalf("parseMetaDataBatchArgs(%v) diff (-got +want):\n%s", tc.args, diff)