DBPW - Make username generation in SQLCredentialsProducer available without an instance#10050
Conversation
| userUUID, | ||
| now, | ||
| } | ||
| username := joinNonEmpty(ub.separator, parts...) |
There was a problem hiding this comment.
Any reason not to have the signature be the same as strings.Join, joinNonEmpty(parts, ub.separator)?
There was a problem hiding this comment.
Because you can't put a vararg at the beginning of the signature.
There was a problem hiding this comment.
You can change the signature to accept a slice
There was a problem hiding this comment.
Accepting a slice is a lot less flexible than accepting a vararg.
There was a problem hiding this comment.
Since the separator is a string value, having the input values be passed in as a slice is a bit easier to read IMO
There was a problem hiding this comment.
Admittedly it's a small issue, but this approach makes it so you don't have to allocate a slice if all of the values are already in variables (or in-line strings). I've put up a commit that shows this (since I neglected to remove the slice from a previous iteration of this function): b6a431d
There was a problem hiding this comment.
Yeah, this is purely internal so I'll leave it up to you.
|
|
||
| func DisplayName(dispName string, maxLength int) UsernameOpt { | ||
| return func(b *usernameBuilder) { | ||
| if maxLength > 0 { |
There was a problem hiding this comment.
I think it's missing the case where the length is NoneLength (-1), which would entirely skip including the display name regardless of what display name was provided if we are sticking with current behavior. Same for RoleName.
There was a problem hiding this comment.
Ah good catch. I missed that. Fixed.
| if len(str) < l { | ||
| return str | ||
| } | ||
| return str[:l] |
There was a problem hiding this comment.
This could be out of bound if l is negative.
There was a problem hiding this comment.
True, but the checks for this are happening above in makeUsername, DisplayName and RoleName. I wanted to keep this function cleaner by not returning an error.
There was a problem hiding this comment.
Can we add a comment on trunc to explicitly state this? I'm more concerned about future usage of this func without proper bound check leading to panics. Alternatively, we could handle the negative value by returning an empty string if that's provided (effectively doing a max(l, 0) on the length).
There was a problem hiding this comment.
Looking at this again I think we can do:
If l == 0 - return the unmodified string
if l == NoneLength - return ""
if l < 0 - return "" (in principle we could error in this case, but I think just returning an empty string is fine too)
calvn
left a comment
calvn
left a comment
There was a problem hiding this comment.
Left a few comment, but looks good otherwise!
pcman312
changed the title
…hicorp#10050) (hashicorp#10071) * move request to parent * add changelog * add test * use refresh instead() Co-authored-by: claire bontempo <68122737+hellobontempo@users.noreply.github.com>
3 participants
Overview
Moves the logic of username generation in the
SQLCredentialsProducerinto its own standalone function. Also gives it the ability to uppercase usernames.Note: I did test this against the existing
SQLCredentialsProducerto ensure the usernames that are being generated match the same patterns as they have been. Since the producer has been updated to use the new function, doing any tests to ensure they match seemed pointless to include, but I did have them prior to moving the producer over to the new function.