Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 7, 2025

Conversation

f0ssel
Copy link
Contributor

@f0ssel f0ssel commented Mar 28, 2025

Fixes an issue where old ssh configs that use the owner--workspace--agent format will fail to properly use the coder ssh command since we migrated off the coder vscodessh command.

@f0ssel f0ssel force-pushed the f0ssel/ssh-workspace-name branch from 9680d52 to 0154e6c Compare March 28, 2025 16:03
Copy link
Member

@Parkreiner Parkreiner left a 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(`[/.]+|--`)
Copy link
Member

@Parkreiner Parkreiner Mar 28, 2025

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

Copy link
Contributor

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 [].

Copy link
Member

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

@f0ssel f0ssel merged commit fc471eb into main Apr 7, 2025
29 checks passed
@f0ssel f0ssel deleted the f0ssel/ssh-workspace-name branch April 7, 2025 14:07
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants