Conversation
cmd/topo/vscode.go
Outdated
| } | ||
|
|
||
| var listSsh = &cobra.Command{ | ||
| Use: "list-ssh <config-filepath>", |
There was a problem hiding this comment.
Should this handle any config path or just topo config path? If former then I'm not sure it belongs in topo. 🤔
There was a problem hiding this comment.
The intention would be to invoke it on ~/.ssh/config. Feels like this is something ssh should be able to do on its own but alas. Totally agree it's not a specific problem - outlined why I did this in the CLI here; #193 (comment)
The only reason I exposed it as a parameter is in case that requirement changes on the vscode side we can avoid having to make CLI changes.
I'll try to add some more context to plead my case - in the vscode extension we want to support a similar workflow to the "Remote - SSH" extension where when you go to add a target we display a list of all your configured ssh hosts to pick as a convenience feature: 
Now, the reason I did this in the CLI as opposed to the vscode extension is to avoid taking on another dependency on the vscode side for sshconfig parsing when we've already got one on the CLI side. However, since its only consumer (at least for now) is the vscode extension it's just a hidden command like Happy to walk this back and re-implement in TS with https://www.npmjs.com/package/ssh-config or basic string manipulation if there's agreement this doesn't belong in the CLI. |
Signed-off-by: awphi <[email protected]>
Signed-off-by: awphi <[email protected]>
|
I wonder if this shouldn't be 'hidden' in cases third parties want to drive their own UI from Topo? |
| for _, host := range cfg.Hosts { | ||
| for _, node := range host.Nodes { | ||
| if include, ok := node.(*sshconfig.Include); ok { | ||
| includePath := strings.TrimSpace(strings.TrimPrefix(include.String(), "Include")) |
There was a problem hiding this comment.
Unfortunately the spec is more complicated than this:
Multiple path names may be specified and each pathname may contain glob(7) wildcards, tokens as described in the “TOKENS” section, environment variables as described in the “ENVIRONMENT VARIABLES” section and, for user configurations, shell-like ‘~’ references to user home directories. Wildcards will be expanded and processed in lexical order.
https://man7.org/linux/man-pages/man5/ssh_config.5.html
It would appear that the ssh_config library can't help us here either.
This is all making me a bit nervous about whether we're taking on responsibilities that aren't core to Topo. At the end of the day, we have a dependency on people having at least a cursory understanding of SSH. One could argue that anyone who has such a fancy config may not need our help (particularly WRT ENV vars). Either way the current implementation cannot guarantee that it will return all targets across all includes paths.
Are we sure the juice is worth the squeeze in terms of the use case here? Do we intend to list all SSH hosts in the VS Code extension? if there's no use case in the CLI I wonder if this might be better leveraging an SSH extension in VS Code?
There was a problem hiding this comment.
It would appear that the ssh_config library can't help us here either.
So I've been digging into ssh_config a bit more since raising this PR and it seems it already recursively resolves Include statements before in a spec-compliant manner so my incomplete version here could be removed. HOWEVER, it does not expose a way to list all the hosts from those Included files (e.g. see open PR adding this here kevinburke/ssh_config#78). Depending on your thoughts on my answer to your second question I'd suggest vendoring this dep and modifying as needed - there's a lot we can strip from it that we're not using and we can cherry-pick in the GetHosts method.
Are we sure the juice is worth the squeeze in terms of the use case here? Do we intend to list all SSH hosts in the VS Code extension?
Imo yes, I'm confident editing my own ssh config but I still like the convenience of the quick pick menu in vscode to quickly ssh into a machine without having to type anything and/or remember my Host alias.
However, if you're really not sure about the value here I'm happy to abandon this PR and we can drop sshconfig target discovery altogether.
if there's no use case in the CLI
I agree there's absolutely no use case in the CLI for this today but I don't know if this will hold forever e.g. we might want similar behaviour in a TUI or if someone vibes up a lazytopo or similar (linking back to what Rob said here; #193 (comment)). The reason it lives in the CLI today is because we already have an sshconfig-parsing dependency here so may as well utilise it rather than adding another on the vscode side.
I wonder if this might be better leveraging an SSH extension in VS Code?
This was indeed my first preference and @federicobozzini's initial suggestion but unfortunately the de-facto standard (or potentially just standard it seems) SSH extension in vscode is "Remote SSH" by Microsoft which is closed-source and exposes no public API we could hook into.
4 participants
Changes
list-candidate-targetswhich takes a path to an ssh config file and spits out a de-duplicated list ofHostentries found in said config, recursively evaluating the contents ofIncluded files.Hostas the most reliable way to resolve ssh directives of a given host is withssh -Grather than reading the config.mapset-like data structure to support cycle detection. There's a few other places in the codebase that could make use of this DS too so will follow up.