- Notifications
You must be signed in to change notification settings - Fork1.1k
chore: move app URL parsing to its own package#11651
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Emyrk commentedJan 16, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed by Graphite.Learn more about stacking. |
| //nameRegex isthesame as our UsernameRegex without the ^ and $. | ||
| nameRegex="[a-zA-Z0-9]+(?:-[a-zA-Z0-9]+)*" |
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 think it should still trim the old regex instead TBH
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.
Because of import loops, we would need to put that regex in some other package then 😢.
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.
codersdk maybe?
Uh oh!
There was an error while loading.Please reload this page.
What this does
Parsing
CODER_WILDCARD_ACCESS_URLas aurl.URLwas always incorrect (seehttps://goplay.tools/snippet/YHUpcSkVrli). Since wildcard urls do not have a scheme, the url parsing was never accurate to how it parsed things. And it would not accept ports.We have a wildcard parse function, so why not make that the validation instead of using the url and
url.String().Lastly I moved
appurlto it's own package to avoid import loops. This is a reasonable thing to package.In support of#8189. We need to move the field from a url to a string to support ports.