-
Notifications
You must be signed in to change notification settings - Fork 881
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 set coder.workspace_name
, and only set coder.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 checking lineScanner.Err()
after the loop (similarly with tokenScanner
). 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-ssh
so that it always 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