Skip to content

Add support for SSH endpoint lookup by VM name #540

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

Merged
merged 9 commits into from
Apr 30, 2025

Conversation

devin-ai-integration[bot]
Copy link
Contributor

Add support for SSH endpoint lookup by VM name

This PR adds the ability to get the SSH endpoint of a VM by name in addition to the existing functionality of getting it by VM ID.

Changes

  • Modified the vm ssh-endpoint command to accept either VM ID or VM name
  • Implemented a function to look up VM ID from VM name
  • Added tab completion for both VM IDs and VM names
  • Updated command documentation to reflect the new functionality

Testing

  • Verified the build works: make build
  • Ran linter: go fmt ./...
  • Ran unit and integration tests: make test-unit && make test-integration

Link to Devin run: https://app.devin.ai/sessions/cfe3ddd734084631a3ae23a8d2a48178
Requested by: [email protected]

Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@@ -263,4 +263,8 @@ type runnerArgs struct {
demoteChannelSequence int64
unDemoteReleaseSequence int64
unDemoteChannelSequence int64

sshVMID string
Copy link
Member

Choose a reason for hiding this comment

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

remove these 3 unused vars (they are unused, right?)

marccampbell
marccampbell previously approved these changes Apr 29, 2025
Copy link
Member

@squizzi squizzi left a comment

Choose a reason for hiding this comment

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

Seems to work well as well in my testing.

My only issue with this is that it lists and therefore completes VMs that are not yet in running state which may result in an issue when the user tries to use that specific VM but perhaps the UX around not completing something a user would expect to get completed is more annoying then only completing "running" state VMs.

We should either only return running VMs to the completion OR provide a useful error back to the user that the vm is not yet in running state.

The error we get now if we do try to ssh to a VM that isn't yet "running" is:

./bin/replicated vm ssh-endpoint intelligent_varahamihira
Error: VM 4ef40195 does not have SSH endpoint configured

which might be confusing to a user.

Copy link
Member

@squizzi squizzi left a comment

Choose a reason for hiding this comment

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

Since we're already here and the change was made to only filter running VMs let's update the Long help text to specify that ssh endpoints can only be retrieved from running VMs.

squizzi
squizzi previously approved these changes Apr 29, 2025
@AmberAlston
Copy link
Member

what happens if there's multiple vms with the same name? it's possible today to have multiple vms with the same name but different ids

@AmberAlston
Copy link
Member

On that new error message it would be nice to also add something like "To view all VM IDs run replicated vm ls"

if _, err := r.kotsAPI.GetVM(arg); err == nil {
return arg, nil
}

Copy link
Member

Choose a reason for hiding this comment

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

getVMIDFromArg function will mask valid errors from the user. If GetVM returns ErrNotFound or ErrForbidden, then we can call ListVMs. Otherwise this function should fail. ErrForbidden can be returned due to RBAC rules.

@marccampbell marccampbell merged commit 2493001 into main Apr 30, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants