feat(vm): add replicated vm scp-endpoint command to print the scp endpoint of a vm #538

DexterYan · 2025-04-29T08:45:44Z

Summary

This pull request improves the handling of Virtual Machine (VM) endpoints in the CLI application with the following key changes:

Introduced a new SCP endpoint.
Refactored the getVMEndpoint function for better readability and efficiency.
Updated relevant test cases to align with the changes.
Removed unused code and files.

Changes

New SCP Endpoint Initialization
- Added runCmds.InitVMSCPEndpoint(vmCmd) to initialize the SCP endpoint for VM commands.
Refactored VM Endpoint Retrieval Logic
- Simplified the logic in getVMEndpoint.
- Introduced vmFromAPI for handling API responses to fetch VM attributes.
New File: vm_endpoints.go
- Encapsulates VM endpoint functionalities.
Removed Legacy File: vm_ssh_endpoint.go
- Deprecated and removed in favor of the new approach encapsulated in vm_endpoints.go.
Test Updates
- Updated test cases in vm_endpoints_test.go:
- Adjusted VM mock data to align with the new int64 type for DirectSSHPort.
- Added test validation for SCP endpoints.

Testing

replicated vm create --distribution ubuntu --version 24.04 --instance-type r1.small --disk 50
./bin/replicated vm scp-endpoint xxxx
scp://[email protected]:44875

Example Results

replicated vm scp-endpoint -h

Get the SCP endpoint and port of a VM.

The output will be in the format: scp://username@hostname:port

You can identify the VM either by its unique ID or by its name.

Note: SCP endpoints can only be retrieved from VMs in the "running" state.


Usage:
  replicated vm scp-endpoint VM_ID_OR_NAME [flags]

Example:
  # Get SCP endpoint for a specific VM by ID
  replicated vm scp-endpoint aaaaa11
  
  # Get SCP endpoint for a specific VM by name
  replicated vm scp-endpoint my-test-vm

Flags:
  -h, --help   help for scp-endpoint

Global Flags:
      --app string     The app slug or app id to use in all calls
      --debug          Enable debug output
      --token string   The API token to use to access your app in the Vendor API

replicated vm scp-endpoint inspiring_lalande

ssh://[email protected]:40589

replicated vm scp-endpoint 25888344

scp://[email protected]:40589

Shortcut

sc-122436

…point of a vm

Copilot

Pull Request Overview

This PR introduces a new SCP endpoint for VMs and refactors the VM endpoint retrieval logic by unifying the SSH and SCP commands.

Removed the legacy SSH endpoint command.
Introduced a new SCP endpoint command and updated command initialization in the root command.
Refactored getVMEndpoint and added tests to cover both endpoint types.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
cli/cmd/vm_ssh_endpoint.go	Removed the outdated SSH endpoint command.
cli/cmd/vm_endpoints_test.go	Added tests for SSH and SCP endpoints, including validation of output and error conditions.
cli/cmd/vm_endpoints.go	Introduced a unified endpoint retrieval function that supports both SSH and SCP.
cli/cmd/root.go	Updated command initialization to include the new SCP endpoint command.

Comments suppressed due to low confidence (1)

cli/cmd/vm_endpoints.go:73

[nitpick] Since the endpoint retrieval logic now supports both SSH and SCP protocols, consider renaming 'directSSHEndpoint' and 'directSSHPort' to more generic names (e.g., 'endpointHost' and 'endpointPort') to better reflect their usage.

directSSHEndpoint, _ = vmMap["DirectSSHEndpoint"].(string)

Copilot · 2025-04-29T23:53:33Z

cli/cmd/vm_endpoints.go

+	}
+
+	if directSSHEndpoint == "" || directSSHPort == 0 {
+		return errors.Errorf("VM %s does not have SSH endpoint configured", id)


The error message below always references an SSH endpoint even when the command is used for SCP. Consider using the endpointType variable to customize the error message (e.g., "VM %s does not have %s endpoint configured").

Suggested change

return errors.Errorf("VM %s does not have SSH endpoint configured", id)

return errors.Errorf("VM %s does not have %s endpoint configured", id, endpointType)

Copilot

Pull Request Overview

This PR refactors the VM endpoint functionality by consolidating SSH and SCP endpoint commands into a single code path and removing the legacy SSH endpoint file. Key changes include:

Introducing a new SCP endpoint command using runCmds.InitVMSCPEndpoint.
Refactoring the endpoint retrieval logic in getVMEndpoint and moving it into the new vm_endpoints.go file.
Removing the legacy vm_ssh_endpoint.go file and updating test cases accordingly.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
cli/cmd/vm_ssh_endpoint.go	Removed legacy SSH endpoint command.
cli/cmd/vm_endpoints_test.go	Added tests to validate the new endpoint (SSH and SCP) retrieval logic.
cli/cmd/vm_endpoints.go	Introduces consolidated logic for SSH and SCP endpoints using generic command initialization.
cli/cmd/root.go	Updated to initialize the new VMSCPEndpoint command alongside the existing VM commands.

Comments suppressed due to low confidence (2)

cli/cmd/vm_endpoints.go:65

[nitpick] Consider renaming 'directSSHEndpoint' to a more generic identifier like 'directEndpoint' since it is used for both SSH and SCP endpoints.

var directSSHEndpoint string

cli/cmd/vm_endpoints.go:66

[nitpick] Consider renaming 'directSSHPort' to 'directPort' for clarity, as the variable represents the port for both SSH and SCP endpoints.

var directSSHPort int64

Copilot

Pull Request Overview

This PR refactors the VM endpoint handling in the CLI by removing the legacy SSH-specific code, introducing a new SCP endpoint command, and updating tests to cover both endpoint types.

Removed the legacy vm_ssh_endpoint.go file.
Refactored getVMEndpoint to support both SSH and SCP endpoints.
Added tests to validate the new endpoint functionality.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
cli/cmd/vm_ssh_endpoint.go	Removed legacy SSH endpoint command in favor of the unified endpoint implementation.
cli/cmd/vm_endpoints_test.go	Introduces tests to validate both SSH and SCP endpoints.
cli/cmd/vm_endpoints.go	Refactored and consolidated the endpoint retrieval logic for SSH and SCP endpoints.
cli/cmd/root.go	Registers the new SCP endpoint command along with existing VM commands.

Copilot · 2025-04-30T00:07:52Z

cli/cmd/vm_endpoints.go

+	outputFormat := "hostname:port"
+
+	cmdLong := fmt.Sprintf(`Get the %s endpoint and port of a VM.
+
+The output will be in the %s`, protocol, outputFormat)


[nitpick] The declared output format 'hostname:port' in the command description does not match the actual printed URI format (which includes the protocol and username). Consider updating the description to accurately reflect the complete format.

Suggested change

outputFormat := "hostname:port"

cmdLong := fmt.Sprintf(`Get the %s endpoint and port of a VM.

The output will be in the %s`, protocol, outputFormat)

outputFormat := "protocol://username@hostname:port"

cmdLong := fmt.Sprintf(`Get the %s endpoint and port of a VM.

The output will be in the format: %s`, protocol, outputFormat)

@xavpaice, could you help me to confirm this is a better change. Or we can keep the description brought by #531

cli/cmd/vm_endpoints.go

Co-authored-by: Copilot <[email protected]>

Copilot

Pull Request Overview

This PR introduces a new SCP endpoint command for VMs while refactoring the endpoint retrieval logic to support both SSH and SCP endpoints. Key changes include:

Removal of the legacy vm_ssh_endpoint.go file.
Creation of a unified vm_endpoints.go file with separate initialization functions for SSH and SCP endpoints.
Addition of new test cases in vm_endpoints_test.go and registration of the new command in root.go.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
cli/cmd/vm_ssh_endpoint.go	Removed legacy SSH endpoint command.
cli/cmd/vm_endpoints.go	Added unified endpoint command supporting SSH and SCP.
cli/cmd/vm_endpoints_test.go	Introduced tests for both SSH and SCP endpoint scenarios.
cli/cmd/root.go	Updated command registration to include the new SCP endpoint.

Copilot · 2025-04-30T00:36:05Z

cli/cmd/vm_endpoints.go

+	endpointType := "ssh"
+	if strings.Contains(cmd.Use, "scp-endpoint") {
+		endpointType = "scp"


Determining the endpoint type by string matching on the command usage can be brittle. Consider using an explicit command annotation or flag to differentiate between SSH and SCP endpoints.

Suggested change

endpointType := "ssh"

if strings.Contains(cmd.Use, "scp-endpoint") {

endpointType = "scp"

endpointType, ok := cmd.Annotations["endpointType"]

if !ok {

return errors.New("endpoint type annotation is missing")

Copilot · 2025-04-30T00:36:06Z

cli/cmd/vm_endpoints.go

+			DirectSSHEndpoint: vmFromAPI.DirectSSHEndpoint,
+			DirectSSHPort:     vmFromAPI.DirectSSHPort,
+			ID:                vmFromAPI.ID,
+		}
+	}
+
+	if vm.DirectSSHEndpoint == "" || vm.DirectSSHPort == 0 {


The VM struct uses SSH-specific field names even for the SCP endpoint configuration. Consider abstracting these field names to support possible future differentiation between SSH and SCP endpoints.

Suggested change

DirectSSHEndpoint: vmFromAPI.DirectSSHEndpoint,

DirectSSHPort: vmFromAPI.DirectSSHPort,

ID: vmFromAPI.ID,

}

}

if vm.DirectSSHEndpoint == "" || vm.DirectSSHPort == 0 {

DirectEndpoint: vmFromAPI.DirectSSHEndpoint,

DirectPort: vmFromAPI.DirectSSHPort,

ID: vmFromAPI.ID,

}

}

if vm.DirectEndpoint == "" || vm.DirectPort == 0 {

Copilot

Pull Request Overview

This PR refactors the VM endpoint handling in the CLI application by removing the legacy SSH endpoint file, consolidating endpoint retrieval logic for both SSH and SCP commands, and updating tests accordingly.

Removed the unused vm_ssh_endpoint.go file.
Introduced vm_endpoints.go to handle both SSH and SCP endpoint commands.
Updated test cases and registered the new SCP endpoint command in the root command.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
cli/cmd/vm_ssh_endpoint.go	Legacy SSH endpoint command removed as part of refactoring.
cli/cmd/vm_endpoints_test.go	New tests added for verifying correct behavior for SSH and SCP endpoints.
cli/cmd/vm_endpoints.go	New file consolidating VM endpoint retrieval and formatting logic.
cli/cmd/root.go	Updated to register the new SCP endpoint command.

Comments suppressed due to low confidence (1)

cli/cmd/vm_endpoints.go:100

When fetching VM data from the API, the code always uses 'DirectSSHEndpoint' and 'DirectSSHPort', even for SCP endpoints. Consider abstracting these fields or ensuring that the API returns the appropriate endpoint values for both SSH and SCP protocols.

DirectEndpoint: vmFromAPI.DirectSSHEndpoint,

Copilot · 2025-04-30T04:07:46Z

cli/cmd/vm_endpoints.go

+	if endpointType != EndpointTypeSSH && endpointType != EndpointTypeSCP {
+		return errors.Errorf("invalid endpoint type: %s", endpointType)


[nitpick] Consider extracting the endpoint type validation into a separate function to avoid duplicating this logic across different command handlers.

Suggested change

if endpointType != EndpointTypeSSH && endpointType != EndpointTypeSCP {

return errors.Errorf("invalid endpoint type: %s", endpointType)

if err := validateEndpointType(endpointType); err != nil {

return err

cli/cmd/root.go

jdewinne · 2025-05-01T22:50:55Z

cli/cmd/vm_endpoints.go

+	"github.com/spf13/cobra"
+)
+
+const githubAccountSettingsURL = "https://vendor.replicated.com/account-settings"


This should be configurable so the endpoint depends on dev/staging/production

I'd like to explain why this URL may not need to be configurable. The URL is used in an error message that appears when the GitHub username is empty:

if githubUsername == "" { return errors.Errorf(`no github account associated with vendor portal user Visit https://vendor.replicated.com/account-settings to link your account`) }

This URL is primarily used in the production environment. It's unlikely that we would need to use this specific error message in staging environments (e.g., staging.vendor.replicated.com/account-settings).
Let me know if you think how it should be

I see a similar meesage in cluster.go - also static text. Let's consider if we really need this to be something we render out, or if a message that is more generic is more appropriate, e.g. "Visit the account-settings page in Vendor Portal to link your account".

cli/cmd/vm_endpoints.go

jdewinne · 2025-05-01T22:59:46Z

cli/cmd/vm_endpoints.go

+	}
+
+	// Use vm if provided, otherwise fetch from API
+	if vm == nil {


Isn't vm always gonna be nil?

This is for our test. We will pass a mockup data as a vm

mockVM: &VM{ ID: "vm-123", DirectEndpoint: "test-vm.example.com", DirectPort: 22, Status: "provisioning", },

It is not always gonna be nil.

I would like to explain the reasoning behind the VM nil check and why we need to pass the VM object into this function:

func (r *runners) getVMEndpoint(vmID, endpointType string, vm *VM, githubUsername string) error {

In the previous implementation, the VM object was always retrieved using r.kotsAPI.GetVM(). This approach is not compatible with writing unit tests since we cannot easily mock the VM object. By allowing the VM to be passed as a parameter, we can provide a mock VM object during testing scenarios.
That's why vm need to be checked if it is nil or not.

Josh is trying to say that getVMEndpoint shouldn't ever get a non-nil VM, and I agree. Test existing or not, the problem is the func shouldn't be written this way, passing in a nil pointer just to satisfy a test is a tricky position to be in. Typically you want the function you're mocking to be as close to the actual called func in real world or the mock is pointless.

A better way to implement this would be to mock r.kotsAPI.GetVM to return mockVM and not build the function to satisfy the test.

Same with githubUsername, we don't provide a way to override that we're adding it as a var to satisfy this test.

There may be a use case to having githubUsername because we may want to allow someone to override username with --username, so that could potentially be an empty string.

Thank you for the suggestion. This is a better solution. I have sets up a mock HTTP server and returns both kotsAPI.GetVM and kotsAPI.GetGitHubUsername.
For --username, I think it would be better to include that in a separate PR.

Co-authored-by: Josh De Winne <[email protected]>

xavpaice · 2025-05-05T02:14:34Z

cli/cmd/vm_endpoints.go

+	// Format the endpoint with username if available
+	if githubUsername == "" {
+		return errors.Errorf(`no github account associated with vendor portal user
+Visit https://vendor.replicated.com/account-settings to link your account`)


Suggested change

Visit https://vendor.replicated.com/account-settings to link your account`)

Visit the Account Settings page in Vendor Portal to link your account`)

this pattern is also seen at

replicated/cli/cmd/cluster.go

Lines 13 to 14 in efe7636

ErrCompatibilityMatrixTermsNotAccepted = errors.New("You must read and accept the Compatibility Matrix Terms of Service before using this command. To view, please visit https://vendor.replicated.com/compatibility-matrix")

)

but I don't suggest changing it in this PR

Thank you! I have updated the error message accordingly.

squizzi · 2025-05-05T19:33:12Z

cli/cmd/vm_endpoints.go

+	}
+
+	// Use vm if provided, otherwise fetch from API
+	if vm == nil {


Josh is trying to say that getVMEndpoint shouldn't ever get a non-nil VM, and I agree. Test existing or not, the problem is the func shouldn't be written this way, passing in a nil pointer just to satisfy a test is a tricky position to be in. Typically you want the function you're mocking to be as close to the actual called func in real world or the mock is pointless.

A better way to implement this would be to mock r.kotsAPI.GetVM to return mockVM and not build the function to satisfy the test.

Same with githubUsername, we don't provide a way to override that we're adding it as a var to satisfy this test.

There may be a use case to having githubUsername because we may want to allow someone to override username with --username, so that could potentially be an empty string.

Stale

DexterYan added 4 commits April 29, 2025 18:14

feat(vm): add replicated vm scp-endpoint command to print the scp end…

af20a21

…point of a vm

removed the duplicate implementation

74a390e

update scp uri format

095c557

refactor code

f4be841

DexterYan added the type::feature label Apr 29, 2025

DexterYan marked this pull request as draft April 29, 2025 08:59

DexterYan added 3 commits April 30, 2025 09:32

update endpoint URL for scp

0ddbc51

add endpoints test

917febe

improve code

5810435

DexterYan marked this pull request as ready for review April 29, 2025 23:49

DexterYan changed the title ~~WIP: feat(vm): add replicated vm scp-endpoint command to print the scp endpoint of a vm~~ feat(vm): add replicated vm scp-endpoint command to print the scp endpoint of a vm Apr 29, 2025

xavpaice requested a review from Copilot April 29, 2025 23:53

Copilot AI reviewed Apr 29, 2025

View reviewed changes

improve error message

8986445

DexterYan requested a review from Copilot April 30, 2025 00:04

Copilot AI reviewed Apr 30, 2025

View reviewed changes

improve naming

4d2461a

DexterYan requested a review from Copilot April 30, 2025 00:07

Copilot AI reviewed Apr 30, 2025

View reviewed changes

DexterYan and others added 3 commits April 30, 2025 12:09

Update cli/cmd/vm_endpoints.go

8853aa4

Co-authored-by: Copilot <[email protected]>

fix build fail

0494eff

improve cmd description

052b368

DexterYan requested a review from Copilot April 30, 2025 00:35

Copilot AI reviewed Apr 30, 2025

View reviewed changes

improve code

581f0fd

DexterYan requested a review from Copilot April 30, 2025 04:07

Copilot AI reviewed Apr 30, 2025

View reviewed changes

DexterYan added 3 commits April 30, 2025 16:12

improve code

277ca62

Merge branch 'main' into dx/sc-122436-add-scp

d0f7684

update vm_endpoints code

1090fca

DexterYan added 2 commits May 1, 2025 10:11

update test

17caea2

Merge branch 'main' into dx/sc-122436-add-scp

d8f8f08

jdewinne previously requested changes May 1, 2025

View reviewed changes

DexterYan and others added 4 commits May 2, 2025 11:14

Update cli/cmd/root.go

5bc0303

Co-authored-by: Josh De Winne <[email protected]>

remove duplicated

77fbdcc

fix test fail

da0fb5d

update the error message

df58a46

xavpaice reviewed May 5, 2025

View reviewed changes

DexterYan added 2 commits May 5, 2025 17:30

improve error message

1a0b543

improve error message

1db6b52

squizzi requested changes May 5, 2025

View reviewed changes

improve unit test for getVMEndpoint

859e04b

squizzi approved these changes May 6, 2025

View reviewed changes

squizzi requested review from jdewinne and xavpaice May 6, 2025 21:30

squizzi merged commit 37463a0 into main May 6, 2025
5 checks passed

squizzi deleted the dx/sc-122436-add-scp branch May 6, 2025 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vm): add replicated vm scp-endpoint command to print the scp endpoint of a vm #538

feat(vm): add replicated vm scp-endpoint command to print the scp endpoint of a vm #538

DexterYan commented Apr 29, 2025 •

edited

Loading

Copilot AI left a comment

Copilot AI Apr 29, 2025

Copilot AI left a comment

Copilot AI left a comment

Copilot AI Apr 30, 2025

DexterYan Apr 30, 2025

Copilot AI left a comment

Copilot AI Apr 30, 2025

Copilot AI Apr 30, 2025

Copilot AI left a comment

Copilot AI Apr 30, 2025

jdewinne May 1, 2025

DexterYan May 2, 2025

xavpaice May 5, 2025

jdewinne May 1, 2025

DexterYan May 2, 2025

DexterYan May 2, 2025

squizzi May 5, 2025

DexterYan May 6, 2025

xavpaice May 5, 2025

xavpaice May 5, 2025

DexterYan May 5, 2025

squizzi May 5, 2025

	return errors.Errorf("VM %s does not have SSH endpoint configured", id)
	return errors.Errorf("VM %s does not have %s endpoint configured", id, endpointType)

		if endpointType != EndpointTypeSSH && endpointType != EndpointTypeSCP {
		return errors.Errorf("invalid endpoint type: %s", endpointType)

	Visit https://vendor.replicated.com/account-settings to link your account`)
	Visit the Account Settings page in Vendor Portal to link your account`)

	ErrCompatibilityMatrixTermsNotAccepted = errors.New("You must read and accept the Compatibility Matrix Terms of Service before using this command. To view, please visit https://vendor.replicated.com/compatibility-matrix")
	)

feat(vm): add replicated vm scp-endpoint command to print the scp endpoint of a vm #538

feat(vm): add replicated vm scp-endpoint command to print the scp endpoint of a vm #538

Conversation

DexterYan commented Apr 29, 2025 • edited Loading

Summary

Changes

Testing

Example Results

Shortcut

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Apr 29, 2025

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DexterYan commented Apr 29, 2025 •

edited

Loading