- Notifications
You must be signed in to change notification settings - Fork928
config-ssh: always support agent name in host alias#3036
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
| // handle both WORKSPACE and WORKSPACE.AGENT syntax | ||
| if len(agents) == 1 { | ||
| wc.Hosts = append(wc.Hosts, workspace.Name) |
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.
We should probably invent "default agent" as a flag in the coder_agent resource or something otherwise this may become annoying, could be a good follow up
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.
This sounds like a great idea! I'm guessing we'd have to update the TF provider, update all TF templates (to set default) and ensure that only one resource has the default agent set?
(Might be a bad idea, but we could consider the first one the default, as a fallback.)
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.
Default could also be optional to avoid backwards incompatibility. If there's no default agent then we never setcoder.workspace_name, and only setcoder.workspace_name.agent_name
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.
Nice solution and tests!
| // handle both WORKSPACE and WORKSPACE.AGENT syntax | ||
| if len(agents) == 1 { | ||
| wc.Hosts = append(wc.Hosts, workspace.Name) |
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.
This sounds like a great idea! I'm guessing we'd have to update the TF provider, update all TF templates (to set default) and ensure that only one resource has the default agent set?
(Might be a bad idea, but we could consider the first one the default, as a fallback.)
| result = append(result, tokenScanner.Text()) | ||
| } | ||
| } | ||
| } |
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.
Minor (non-blocking): We could consider checkinglineScanner.Err() after the loop (similarly withtokenScanner). I realize it's quite inconsequential for how this function is used, it's more of a best practice.
@dwahler is this good to merge? We should probably get this out in a patch release. |
This PR adjusts the behavior of
coder config-sshso that italways creates aliases of the formcoder.<WORKSPACE_NAME>.<AGENT_NAME>, and additionally creates an alias of the formcoder.<WORKSPACE_NAME>if the workspace has exactly one agent. This is more consistent with the behavior ofcoder ssh, and prevents us from incorrectly rejecting commands that are suggested by UI tooltips.I believe the original logic was subtly incorrect, because it only considered how many agents existed in each workspace resource, not the total number of agents in the workspace. So if a workspace had two resources with one agent each, it would have generated multiple invalid
coder.<WORKSPACE_NAME>aliases, instead of the correct aliases with the agent name.Fixes#2969