- Notifications
You must be signed in to change notification settings - Fork905
fix: handle vscodessh style workspace names in coder ssh#17154
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
9680d52 to0154e6cCompareThere 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.
Changes look straightforward to me. Mainly had questions about the regex pattern
| @@ -57,6 +58,7 @@ var ( | |||
| autostopNotifyCountdown = []time.Duration{30 * time.Minute} | |||
| // gracefulShutdownTimeout is the timeout, per item in the stack of things to close | |||
| gracefulShutdownTimeout = 2 * time.Second | |||
| workspaceNameRe = regexp.MustCompile(`[/.]+|--`) | |||
ParkreinerMar 28, 2025 • 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.
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 feel like we can make this Regex more specific to avoid false positives. I don't know if Go's version requires you to escape the. and/, but I'm worried the. is getting interpreted as a wildcard and not a literal period
I think we could go with something like\/|\.|--, which treats the. case,/ case, and-- case as distinct. At least from the test inputs, I didn't see any cases where the dots and slashes could be joined together as a single pattern, which is what the current regex allows
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.
. is not interpreted as a wildcard inside[].
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.
Ah, that's right – forgot that regex classes stop that behavior
fc471eb intomainUh oh!
There was an error while loading.Please reload this page.
Fixes an issue where old ssh configs that use the
owner--workspace--agentformat will fail to properly use thecoder sshcommand since we migrated off thecoder vscodesshcommand.