Skip to content

feat!: show actionable connectivity health fixes #277

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.

Sign up for GitHub

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

Closed
federicobozzini wants to merge 1 commit into from
Closed

feat!: show actionable connectivity health fixes #277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
68 changes: 30 additions & 38 deletions internal/health/health.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,6 @@ import (

type CheckStatus string

func NewCheckStatusFromError(err error) CheckStatus {
if err != nil {
return CheckStatusError
}
return CheckStatusOK
}

const (
CheckStatusOK CheckStatus = "ok"
CheckStatusWarning CheckStatus = "warning"
Expand Down Expand Up @@ -123,7 +116,11 @@ func GenerateHostReport(statuses []DependencyStatus) HostReport {
func GenerateTargetReport(targetStatus Status) TargetReport {
report := TargetReport{}
report.IsLocalhost = targetStatus.Connection.IsPlainLocalhost()
report.Connectivity = connectivityCheck(targetStatus.Connection)
report.Connectivity = healthCheckWithStatusFromError(
HealthCheck{Name: "Connectivity"},
targetStatus.Connection.Error,
connectivityFix(targetStatus.Connection),
)

report.SubsystemDriver.Name = "Subsystem Driver (remoteproc)"
remoteProcessors := targetStatus.Hardware.RemoteProcessors
Expand All @@ -149,55 +146,50 @@ func GenerateTargetReport(targetStatus Status) TargetReport {
return report
}

func connectivityCheck(status ConnectionStatus) HealthCheck {
check := HealthCheck{
Name: "Connectivity",
Status: NewCheckStatusFromError(status.Error),
}
if status.Error == nil {
return check
}

check.Value = status.Error.Error()
func connectivityFix(status ConnectionStatus) *Fix {
switch {
case errors.Is(status.Error, probe.ErrAuthFailed):
check.Fix = &Fix{
return &Fix{
Description: "Configure SSH keys on remote target",
Command: fmt.Sprintf("topo setup-keys --target %s", status.Destination),
}
case errors.Is(status.Error, probe.ErrHostKeyUnknown):
check.Fix = &Fix{
return &Fix{
Description: "Trust the target's SSH host key",
Command: fmt.Sprintf("topo health --target %s --accept-new-host-keys", status.Destination),
}
case errors.Is(status.Error, probe.ErrHostKeyChanged):
check.Fix = &Fix{
return &Fix{
Description: "Remove the old SSH host key from known_hosts, then retry",
Command: fmt.Sprintf("ssh-keygen -R %s", status.Destination.Host),
}
}
return check
return nil
}

func generateDependencyReport(statuses []DependencyStatus) []HealthCheck {
res := []HealthCheck{}
for _, ds := range statuses {
hc := HealthCheck{Name: ds.Dependency.Label}
if ds.Error == nil {
hc.Status = CheckStatusOK
hc.Value = ds.Dependency.Binary
} else {
if _, ok := errors.AsType[WarningError](ds.Error); ok {
hc.Status = CheckStatusWarning
} else if _, ok := errors.AsType[InfoError](ds.Error); ok {
hc.Status = CheckStatusInfo
} else {
hc.Status = CheckStatusError
}
hc.Value = ds.Error.Error()
hc.Fix = ds.Fix
}
res = append(res, hc)
check := HealthCheck{Name: ds.Dependency.Label, Value: ds.Dependency.Binary}
res = append(res, healthCheckWithStatusFromError(check, ds.Error, ds.Fix))
}
return res
}

func healthCheckWithStatusFromError(check HealthCheck, err error, fix *Fix) HealthCheck {
Copy link
Copy Markdown
Contributor Author

@federicobozzini federicobozzini May 29, 2026

Choose a reason for hiding this comment

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

I'm not crazy about this name but it's the best I could come up with 🤷

if err == nil {
check.Status = CheckStatusOK
return check
}

if _, ok := errors.AsType[WarningError](err); ok {
check.Status = CheckStatusWarning
} else if _, ok := errors.AsType[InfoError](err); ok {
check.Status = CheckStatusInfo
} else {
check.Status = CheckStatusError
}
check.Value = err.Error()
check.Fix = fix
return check
}
28 changes: 28 additions & 0 deletions internal/health/health_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,34 @@ func TestTargetReport(t *testing.T) {
require.NoError(t, json.Unmarshal(b, &result))
assert.JSONEq(t, `[]`, string(result["dependencies"]))
})

t.Run("includes connectivity fix", func(t *testing.T) {
Copy link
Copy Markdown
Contributor

@muchzill4 muchzill4 Jun 1, 2026

Choose a reason for hiding this comment

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

  1. What additional coverage does this test add over what we have already?
  2. If there's something we want to test, testing it via json representation of the structs is an odd. Are we trying to cover how the data is converted to json here? Am I missing something?

tr := health.GenerateTargetReport(health.Status{
Connection: health.ConnectionStatus{
Destination: ssh.NewDestination("user@my-target"),
Error: probe.ErrAuthFailed,
},
})

b, err := json.Marshal(tr)

require.NoError(t, err)
want := `{
"connectivity": {
"name": "Connectivity",
"status": "error",
"value": "ssh authentication failed",
"fix": {
"description": "Configure SSH keys on remote target",
"command": "topo setup-keys --target ssh://user@my-target"
}
},
"dependencies": []
}`
var result map[string]json.RawMessage
require.NoError(t, json.Unmarshal(b, &result))
assert.JSONEq(t, want, fmt.Sprintf(`{"connectivity": %s, "dependencies": %s}`, result["connectivity"], result["dependencies"]))
})
})
}

Expand Down
Loading