-
Notifications
You must be signed in to change notification settings - Fork 356
Add meta-data set-batch command
#3791
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 <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. | ||
|
|
||
| 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) | ||
| } | ||
|
Comment on lines
+116
to
+118
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we offer a way to "clear" a particular key?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My robots suggest I reply like this… 😅
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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
}
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Valid observation, but this intentionally matches the existing
Comment on lines
+134
to
+135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence since the earlier thread: the current 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 | ||
| } | ||
There was a problem hiding this comment.
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)?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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…
I mostly didn't add it because it contained more decisions 😅
If we think the current
argvapproach 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.
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
jqas a builder, but it's a much better fit to just build an array ofkey=valuestrings and put them on argv)There was a problem hiding this comment.
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.