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

Ensure all string slice args have whitespace cleaned off of each element #3021

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

moskyb
Copy link
Contributor

@moskyb moskyb commented Oct 2, 2024

This allows users to use:

buildkite-agent oidc request-token --claim 'this, that, the-other'

where previously, even if that and the-other were allowed optional claims, they'd be rejected, as they'd be sent to buildkite with their preceeding whitespace

Testing

  • Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
  • Code is formatted (with go fmt ./...)

This allows users to use:
    buildkite-agent oidc request-token --claim 'this, that, the-other'
where previously, even if `that` and `the-other` were allowed optional claims, they'd be rejected, as they'd be sent to buildkite with their preceeding whitespace
@moskyb moskyb requested review from sj26 and a team October 2, 2024 00:34
Copy link
Member

@sj26 sj26 left a comment

Choose a reason for hiding this comment

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

Nice idea!

Want to do the same to AWSSessionTags?

Can we add a test?

Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

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

@@ -225,7 +225,11 @@ func (l Loader) setFieldValueFromCLI(fieldName string, cliName string) error {
case reflect.String:
value = configFileValue
case reflect.Slice:
value = strings.Split(configFileValue, ",")
valueSlice := strings.Split(configFileValue, ",")
Copy link
Contributor

Choose a reason for hiding this comment

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

Good move, it seems very unlikely that any string list will be dependent on whitespace.

@moskyb moskyb force-pushed the strip-whitespace-on-optional-claims branch from 3338f3d to fe814e2 Compare October 3, 2024 00:34
@moskyb
Copy link
Contributor Author

moskyb commented Oct 3, 2024

@DrJosh9000 i've updated this so that it's the normalize:"list" struct tag that does this, rather than the CLI parsing.

i would love to have a couple of days free to unwind loader.go and give it some proper tests....

@moskyb moskyb changed the title Strip whitespace on optional claim names Ensure all string slice args have whitespace cleaned off of each element Oct 3, 2024
Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

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

Even better!

@moskyb moskyb merged commit cba314d into main Oct 3, 2024
1 check passed
@moskyb moskyb deleted the strip-whitespace-on-optional-claims branch October 3, 2024 02:01
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.

3 participants