-
Notifications
You must be signed in to change notification settings - Fork 7
feat: list-candidate-targets command #193
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
base: main
Are you sure you want to change the base?
Changes from all commits
7fa3b34
d3783b9
7826b35
76f6e8a
309c1e5
14b42e3
db6e56f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| package collections | ||
|
|
||
| import ( | ||
| "maps" | ||
| "slices" | ||
| ) | ||
|
|
||
| type Set[T comparable] struct { | ||
| elements map[T]struct{} | ||
| } | ||
|
|
||
| func NewSet[T comparable](items ...T) Set[T] { | ||
| s := Set[T]{elements: make(map[T]struct{})} | ||
| for _, item := range items { | ||
| s.Add(item) | ||
| } | ||
| return s | ||
| } | ||
|
|
||
| func (s *Set[T]) Add(item T) { | ||
| s.elements[item] = struct{}{} | ||
| } | ||
|
|
||
| func (s *Set[T]) ToSlice() []T { | ||
| return slices.Collect(maps.Keys(s.elements)) | ||
| } | ||
|
|
||
| func (s *Set[T]) Contains(item T) bool { | ||
| _, exists := s.elements[item] | ||
| return exists | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| package collections_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/arm/topo/internal/collections" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestSet(t *testing.T) { | ||
| t.Run("NewSet", func(t *testing.T) { | ||
| t.Run("creates a set containing the provided items", func(t *testing.T) { | ||
| set := collections.NewSet("a", "b", "c") | ||
|
|
||
| assert.True(t, set.Contains("a")) | ||
| assert.True(t, set.Contains("b")) | ||
| assert.True(t, set.Contains("c")) | ||
| assert.Len(t, set.ToSlice(), 3) | ||
| }) | ||
|
|
||
| t.Run("deduplicates items", func(t *testing.T) { | ||
| set := collections.NewSet("a", "a", "b") | ||
|
|
||
| got := set.ToSlice() | ||
|
|
||
| assert.Len(t, got, 2) | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("Add", func(t *testing.T) { | ||
| t.Run("adds an item to the set", func(t *testing.T) { | ||
| set := collections.NewSet[string]() | ||
|
|
||
| set.Add("x") | ||
|
|
||
| assert.True(t, set.Contains("x")) | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("Contains", func(t *testing.T) { | ||
| t.Run("returns true for an item in the set", func(t *testing.T) { | ||
| set := collections.NewSet("present") | ||
|
|
||
| got := set.Contains("present") | ||
|
|
||
| assert.True(t, got) | ||
| }) | ||
|
|
||
| t.Run("returns false for an item not in the set", func(t *testing.T) { | ||
| set := collections.NewSet("present") | ||
|
|
||
| got := set.Contains("absent") | ||
|
|
||
| assert.False(t, got) | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("ToSlice", func(t *testing.T) { | ||
| t.Run("returns all elements as a slice", func(t *testing.T) { | ||
| set := collections.NewSet(1, 2, 3) | ||
|
|
||
| got := set.ToSlice() | ||
|
|
||
| assert.ElementsMatch(t, []int{1, 2, 3}, got) | ||
| }) | ||
|
|
||
| t.Run("returns an empty slice for an empty set", func(t *testing.T) { | ||
| set := collections.NewSet[int]() | ||
|
|
||
| got := set.ToSlice() | ||
|
|
||
| assert.Empty(t, got) | ||
| }) | ||
| }) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| package ssh | ||
|
|
||
| import ( | ||
| "strings" | ||
|
|
||
| "github.com/arm/topo/internal/collections" | ||
| "github.com/arm/topo/internal/output/logger" | ||
| sshconfig "github.com/kevinburke/ssh_config" | ||
| ) | ||
|
|
||
| func gatherIncludedConfigPaths(cfg *sshconfig.Config) []string { | ||
| includedPaths := []string{} | ||
|
|
||
| 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")) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately the spec is more complicated than this:
https://man7.org/linux/man-pages/man5/ssh_config.5.html It would appear that the 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So I've been digging into
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 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.
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
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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is it YAGNI for CLI then?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If we don't mind taking on an npm dependency on the vscode side and duplicating some sshconfig parsing code then yes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Just to clarify, this PR doesn't list all SSH hosts, only the one in the config path used as an argument. This is also the topo-specific bit, since we plan to have all the topo configs in a separate file.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If you are after something that lists all topo hosts, which config is owned by topo, then the ssh config path shouldn't be configurable, as topo's internals know the path already.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll need to know all hosts, not just ones in the topo config file so we can support adding externally managed hosts as targets in the extension. That is to say we're probably going to invoke It's configurable in case the user is using an unorthodox ssh config file path they'd like to consume in the vscode extension but maybe we shouldn't care about that. Important to remember this command is just for the discovery drop-down when adding a new target to the extension and won't be the source of truth for the actual target list. The diagram on the jira ticket is worth referring to here. |
||
| if includePath != "" { | ||
| includedPaths = append(includedPaths, includePath) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return includedPaths | ||
| } | ||
|
|
||
| func ListHosts(configPath string) []string { | ||
| queue := []string{configPath} | ||
| seen := collections.NewSet[string]() | ||
| hosts := collections.NewSet[string]() | ||
|
|
||
| for len(queue) > 0 { | ||
| currentPath := queue[0] | ||
| queue = queue[1:] | ||
| if seen.Contains(currentPath) { | ||
| continue | ||
| } | ||
| seen.Add(currentPath) | ||
|
|
||
| cfg, err := readConfigFile(currentPath) | ||
| if err != nil { | ||
| logger.Error("failed to read ssh config file while listing hosts", "path", currentPath, "error", err) | ||
| continue | ||
| } | ||
|
|
||
| for _, host := range cfg.Hosts { | ||
| queue = append(queue, gatherIncludedConfigPaths(cfg)...) | ||
|
|
||
| for _, pattern := range host.Patterns { | ||
| patternStr := pattern.String() | ||
| if patternStr == "*" { | ||
| continue | ||
| } | ||
| hosts.Add(patternStr) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return hosts.ToSlice() | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| package ssh_test | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/arm/topo/internal/ssh" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func writeSSHConfig(t *testing.T, dir, name, content string) string { | ||
| t.Helper() | ||
| path := filepath.Join(dir, name) | ||
| require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) | ||
| return filepath.ToSlash(path) | ||
| } | ||
|
|
||
| func TestListHosts(t *testing.T) { | ||
| t.Run("returns hosts from a single config file", func(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| configPath := writeSSHConfig(t, tmp, "config", ` | ||
| Host board1 | ||
| HostName 192.168.0.1 | ||
|
|
||
| Host board2 | ||
| HostName 192.168.0.2 | ||
| `) | ||
|
|
||
| got := ssh.ListHosts(configPath) | ||
|
|
||
| assert.ElementsMatch(t, []string{"board1", "board2"}, got) | ||
| }) | ||
|
|
||
| t.Run("excludes wildcard host pattern", func(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| configPath := writeSSHConfig(t, tmp, "config", ` | ||
| Host * | ||
| ServerAliveInterval 60 | ||
|
|
||
| Host myhost | ||
| HostName 10.0.0.1 | ||
| `) | ||
|
|
||
| got := ssh.ListHosts(configPath) | ||
|
|
||
| assert.ElementsMatch(t, []string{"myhost"}, got) | ||
| }) | ||
|
|
||
| t.Run("follows include directives", func(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| includedPath := writeSSHConfig(t, tmp, "extra_config", ` | ||
| Host included-host | ||
| HostName 10.0.0.2 | ||
| `) | ||
| configPath := writeSSHConfig(t, tmp, "config", fmt.Sprintf(` | ||
| Include %s | ||
|
|
||
| Host main-host | ||
| HostName 10.0.0.1 | ||
| `, includedPath)) | ||
|
|
||
| got := ssh.ListHosts(configPath) | ||
|
|
||
| assert.ElementsMatch(t, []string{"main-host", "included-host"}, got) | ||
| }) | ||
|
|
||
| t.Run("deduplicates hosts across files", func(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| includedPath := writeSSHConfig(t, tmp, "extra_config", ` | ||
| Host shared-host | ||
| HostName 10.0.0.1 | ||
| `) | ||
| configPath := writeSSHConfig(t, tmp, "config", ` | ||
| Include `+includedPath+` | ||
|
|
||
| Host shared-host | ||
| HostName 10.0.0.1 | ||
| `) | ||
|
|
||
| got := ssh.ListHosts(configPath) | ||
|
|
||
| assert.ElementsMatch(t, []string{"shared-host"}, got) | ||
| }) | ||
|
|
||
| t.Run("handles cyclic includes without infinite loop", func(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| configAPath := filepath.Join(tmp, "config_a") | ||
| configBPath := filepath.Join(tmp, "config_b") | ||
|
|
||
| writeSSHConfig(t, tmp, "config_a", ` | ||
| Include `+configBPath+` | ||
|
|
||
| Host host-a | ||
| HostName 10.0.0.1 | ||
| `) | ||
| writeSSHConfig(t, tmp, "config_b", ` | ||
| Include `+configAPath+` | ||
|
|
||
| Host host-b | ||
| HostName 10.0.0.2 | ||
| `) | ||
|
|
||
| got := ssh.ListHosts(configAPath) | ||
|
|
||
| assert.Nil(t, got, "should return without hanging on cyclic includes") | ||
| }) | ||
|
|
||
| t.Run("returns empty slice for nonexistent config file", func(t *testing.T) { | ||
| got := ssh.ListHosts("/nonexistent/path/config") | ||
|
|
||
| assert.Empty(t, got) | ||
| }) | ||
|
|
||
| t.Run("returns empty slice for config with only wildcard host", func(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| configPath := writeSSHConfig(t, tmp, "config", ` | ||
| Host * | ||
| ServerAliveInterval 60 | ||
| `) | ||
|
|
||
| got := ssh.ListHosts(configPath) | ||
|
|
||
| assert.Empty(t, got) | ||
| }) | ||
| } |
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.
nit:
targetsmight be better. Trying to be cautious on mixing the usage ofhostandtargetwithin the code. I think we usehostfor docker context buttargetfor all others