Add meta-data set-batch command#3791
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a11a13a911
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if resp != nil && (resp.StatusCode == 401 || resp.StatusCode == 404) { | ||
| r.Break() |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
🤖 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.
|
|
||
| Set multiple meta-data key/value pairs on a build in a single request. | ||
|
|
||
| Each argument must be in key=value format. |
There was a problem hiding this comment.
Should we support JSON input (over stdin)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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)
There was a problem hiding this comment.
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.
| if strings.TrimSpace(value) == "" { | ||
| return nil, fmt.Errorf("invalid argument %q: value cannot be empty, or composed of only whitespace characters", arg) | ||
| } |
There was a problem hiding this comment.
Should we offer a way to "clear" a particular key? key= (nothing following) seems like a reasonable syntax for that.
There was a problem hiding this comment.
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).
a11a13a to
9186eaa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9186eaa35e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| for i := range items { | ||
| if redactedValue := redact.String(items[i].Value, needles); redactedValue != items[i].Value { | ||
| l.Warn("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) |
There was a problem hiding this comment.
Use Warnf so the new command compiles
This new command calls l.Warn here and again in the retry loop, but the logger.Logger interface only defines Warnf, not Warn; any build or test that includes clicommand fails with l.Warn undefined. Please switch these warning calls to Warnf.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Fixed in 1f1c167 — both l.Warn calls in meta_data_set_batch.go now use l.Warnf.
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 <amp@ampcode.com>
9186eaa to
1f1c167
Compare
|
Rebased, eliminated test assertion usage, updated for |
DrJosh9000
left a comment
There was a problem hiding this comment.
This is pretty good, I have a style comment that will supercharge your table driven test
| 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) | ||
| } |
There was a problem hiding this comment.
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 != "".
There was a problem hiding this comment.
🤖 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).
There was a problem hiding this comment.
Don't mind the sass from Amp/Opus there 😜
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 <amp@ampcode.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a94f79703
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if resp != nil && (resp.StatusCode == 401 || resp.StatusCode == 404) { | ||
| r.Break() |
There was a problem hiding this comment.
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 👍 / 👎.
Description
Add a
buildkite-agent meta-data set-batchcommand for setting multiple meta-data key/value pairs in a single API request.The batch operation is transactional — all items succeed or none do.
Context
Companion to a new
POST /v3/jobs/:job_id/data/set-batchAPI route, which is not yet generally available.Changes
MetaDataBatchtype andSetMetaDataBatchmethod (POST /jobs/{id}/data/set-batch)meta-data set-batchsubcommand acceptingkey=valuearguments (no stdin support)AGENT.mdTesting
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)Disclosures / Credits
Amp (Claude Opus 4.6) wrote the implementation and tests with human direction and review.
Shipping