Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion pkg/cmd/register/sshkeys.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,20 @@ func SelectNodeFromList(ctx context.Context, t *terminal.Terminal, prompter term
thisNodeID = reg.ExternalNodeID
}
t.Vprint("")
thisIdx := -1
labels := make([]string, len(nodes))
for i, n := range nodes {
labels[i] = fmt.Sprintf("%s (%s)", n.GetName(), n.GetExternalNodeId())
if thisNodeID != "" && n.GetExternalNodeId() == thisNodeID {
labels[i] = fmt.Sprintf("%s (%s) — this node", n.GetName(), n.GetExternalNodeId())
thisIdx = i
} else {
labels[i] = fmt.Sprintf("%s (%s)", n.GetName(), n.GetExternalNodeId())
}
}
if thisIdx > 0 {
nodes[0], nodes[thisIdx] = nodes[thisIdx], nodes[0]
labels[0], labels[thisIdx] = labels[thisIdx], labels[0]
Comment on lines +57 to +58
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're altering the order of the input slice of nodes now... I suppose that's not a huge problem, but a bit of a hidden side effect.

Copy link
Copy Markdown
Contributor Author

@patelspratik patelspratik Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya, we could make it a copy, but the list is already an arbitrary order, so I don't think it should matter.

interestingly this list is actually only passed in and then thrown away, so we could move the entire listnodes fetch inside of this, then there isn't any side effects. I'll do that

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have some stable order of the rest of these? E.g.:

my-awesome-node (this node)
abc
bcd
def
...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya, we can sort. this is currently an unordered list from the backend

}
chosen := prompter.Select("Select node", labels)
var selected *nodev1.ExternalNode
for i, label := range labels {
Expand Down
100 changes: 100 additions & 0 deletions pkg/cmd/register/sshkeys_test.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
package register

import (
"context"
"os"
"os/user"
"path/filepath"
"strings"
"testing"

nodev1 "buf.build/gen/go/brevdev/devplane/protocolbuffers/go/devplaneapi/v1"

"github.com/brevdev/brev-cli/pkg/terminal"
)

func tempUser(t *testing.T) *user.User {
Expand Down Expand Up @@ -230,3 +235,98 @@ func TestInstallAuthorizedKey_EmptyUserID_UsesPrefix(t *testing.T) {
t.Errorf("should not contain user_id when empty, got:\n%s", content)
}
}

// --- SelectNodeFromList ---

// captureSelector records the items passed to Select and returns the item at returnIdx.
type captureSelector struct {
items []string
returnIdx int
}

func (s *captureSelector) Select(_ string, items []string) string {
s.items = items
return items[s.returnIdx]
}

func makeNodes(ids ...string) []*nodev1.ExternalNode {
nodes := make([]*nodev1.ExternalNode, len(ids))
for i, id := range ids {
nodes[i] = &nodev1.ExternalNode{
ExternalNodeId: id,
Name: "node-" + id,
}
}
return nodes
}

func TestSelectNodeFromList_ThisNodeMovedToFront(t *testing.T) {
sel := &captureSelector{returnIdx: 0}
regStore := &mockRegistrationStore{
reg: &DeviceRegistration{ExternalNodeID: "c"},
}
nodes := makeNodes("a", "b", "c", "d")

got, err := SelectNodeFromList(context.Background(), terminal.New(), sel, regStore, nodes)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}

// "this node" label should be first in the list.
if !strings.Contains(sel.items[0], "— this node") {
t.Errorf("expected first item to be 'this node', got %q", sel.items[0])
}
// No other item should have the label.
for _, item := range sel.items[1:] {
if strings.Contains(item, "— this node") {
t.Errorf("unexpected 'this node' label on non-first item: %q", item)
}
}
// Selecting index 0 should return node "c".
if got.GetExternalNodeId() != "c" {
t.Errorf("expected selected node 'c', got %q", got.GetExternalNodeId())
}
}

func TestSelectNodeFromList_ThisNodeAlreadyFirst(t *testing.T) {
sel := &captureSelector{returnIdx: 0}
regStore := &mockRegistrationStore{
reg: &DeviceRegistration{ExternalNodeID: "a"},
}
nodes := makeNodes("a", "b", "c")

got, err := SelectNodeFromList(context.Background(), terminal.New(), sel, regStore, nodes)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}

if !strings.Contains(sel.items[0], "— this node") {
t.Errorf("expected first item to be 'this node', got %q", sel.items[0])
}
if got.GetExternalNodeId() != "a" {
t.Errorf("expected selected node 'a', got %q", got.GetExternalNodeId())
}
}

func TestSelectNodeFromList_NoRegistration(t *testing.T) {
sel := &captureSelector{returnIdx: 1}
regStore := &mockRegistrationStore{} // no registration

nodes := makeNodes("a", "b", "c")

got, err := SelectNodeFromList(context.Background(), terminal.New(), sel, regStore, nodes)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}

// No item should have the "this node" label.
for _, item := range sel.items {
if strings.Contains(item, "— this node") {
t.Errorf("unexpected 'this node' label: %q", item)
}
}
// Order should be unchanged; selecting index 1 returns "b".
if got.GetExternalNodeId() != "b" {
t.Errorf("expected selected node 'b', got %q", got.GetExternalNodeId())
}
}
Loading