-
Notifications
You must be signed in to change notification settings - Fork 423
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
Remove DNS-1123 validation of usernames and groups #260
Remove DNS-1123 validation of usernames and groups #260
Conversation
Welcome @richardmarshall! |
/assign @mattmoyer |
I don't have an issue with removing the regex, but the
I like this idea a lot and would happily accept this |
I expected that but wanted to seed discussion with the lowest effort code change 😄
Great, will update the code and add some documentation for the new template variable. |
202352f
to
a03ffd7
Compare
@micahhausler Updated, PTAL |
/lgtm |
@micahhausler @nckturner Anything else needed on my end to help move this forward? |
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.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: micahhausler, richardmarshall The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s doesn't require user or group names to be DNS-1123 labels so there isn't a need to enforce or transform these values to conform to that format.
To that end this PR removes the regexp check which validates DNS-1123 and removes the string replacement of
@
s in SessionName.Also of note that the current regex is missing start and end of line anchors so the validation is only ensuring that a group or user contains a sub-string that is a valid DNS-1123 label.
The second change does have impacts on existing usage of the
{{SessionName}}
template value which could break rolebindings for users who are making use of this value and have email addresses for session names.I see a few ways this could be addressed and am happy to update the PR to reflect a desired path for this.
--legacy-session-name=true
){{SessionNameRaw}}
) that doesn't do the replacement and leave the existing behavior for{{SessionName}}
in place.Related to #80