-
Notifications
You must be signed in to change notification settings - Fork 38
useragent: add FromSlice function
#1389
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
Conversation
This function accepts a slice of any value and converts to a slice of `UserAgentProduct` structs. This functionality will be used by upstream clients which accept a list of raw string content to be appended to the User-Agent header, rather than a structured object with name, version, and comment sections explicitly defined.
3566963 to
c0aa19d
Compare
YakDriver
left a comment
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.
LGTM 🎉
Couple minor nits
useragent/from.go
Outdated
| // pattern (where version and comment are optional) return a zero value struct. | ||
| func FromSlice[T any](sl []T) config.UserAgentProducts { | ||
| return slices.ApplyToAll(sl, func(v T) config.UserAgentProduct { | ||
| if s, ok := any(v).(string); ok && s != "" { |
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.
doesn't really need the empty check since fromString checks that?
useragent/from.go
Outdated
| return config.UserAgentProduct{Name: parts[0]} | ||
| case 2: //nolint: mnd | ||
| subparts := strings.Split(parts[1], "(") | ||
| if len(subparts) == 2 { //nolint: mnd |
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.
| if len(subparts) == 2 { //nolint: mnd | |
| if len(subparts) == 2 && strings.HasSuffix(parts[1], ")") { //nolint: mnd |
probably not worth "fixing" but an edge case "my-product/v1.2.3 (not a comment" that would parse wrong
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.
A missing closing parenthesis would still parse correctly because strings.TrimSuffix(subparts[1], ")") just does nothing. For example, this test case passes.
{
"malformed comment closing",
"my-product/v1.2.3 (a comment",
config.UserAgentProduct{
Name: "my-product",
Version: "v1.2.3",
Comment: "a comment",
},
},--- PASS: Test_fromString (0.00s)
--- PASS: Test_fromString/malformed_comment_closing (0.00s)
There is weirdness if the opening parenthesis is missed because the version and comment will get mashed together into the Version struct field, but it winds up rendering the same in the eventual UA.
In general this conversion logic is just a shim until V2 of the AWSCC provider, where we'll deprecate the structured object user_agent in favor of a list of strings. At that time the shared components in this repo can also be simplified to just pass the strings through as-is without all this extra manipulation.
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'll still go ahead and add some test cases for malformed comment patterns, just so we document the edge cases exists and what the expected handling is.
ewbankkit
left a comment
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.
LGTM 🚀.
Description
This function accepts a slice of any value and converts to a slice of
UserAgentProductstructs. This functionality will be used by upstream clients which accept a list of raw string content to be appended to the User-Agent header, rather than a structured object with name, version, and comment sections explicitly defined.For example, this convenience method allows the existing
Contextfunction to be used as follows.Relates hashicorp/terraform-provider-aws#45464