feat!: add explicit command to fix target health issues

e2e/setup_keys_test.go

@@ -20,14 +20,14 @@ func TestSetupKeysJourney(t *testing.T) {
 	Step(t, "health reports unknown host key and suggests accept-new-host-keys")
 	out := runTopo(t, topo, "health", "--target", container.SSHDestination)
 	assert.Contains(t, out, "Connectivity: ❌ (ssh host key is unknown)")
-	wantFix := fmt.Sprintf("run `topo health --target %s --accept-new-host-keys", container.SSHDestination)
-	assert.Contains(t, out, wantFix)
+	assert.Contains(t, out, "Fix: Trust the target's SSH host key")
+	assert.Contains(t, out, fmt.Sprintf("Cmd: topo health --target %s --accept-new-host-keys", container.SSHDestination))
 
 	Step(t, "health with accept-new-host-keys trusts host and suggests setup-keys")
 	out = runTopo(t, topo, "health", "--target", container.SSHDestination, "--accept-new-host-keys")
 	assert.Contains(t, out, "Connectivity: ❌ (ssh authentication failed)")
-	wantFix = fmt.Sprintf("run `topo setup-keys --target %s`", container.SSHDestination)
-	assert.Contains(t, out, wantFix)
+	assert.Contains(t, out, "Fix: Configure SSH keys on remote target")
+	assert.Contains(t, out, fmt.Sprintf("Cmd: topo setup-keys --target %s", container.SSHDestination))
 
 	Step(t, "setup-keys generates keys and installs them on the target")
 	askpass := writeAskPassScript(t, sshRootPassword)

internal/health/check.go

@@ -11,7 +11,12 @@ import (
 )
 
 type Check interface {
-	Run(ctx context.Context, r runner.Runner, dep Dependency) (string, error)
+	Run(ctx context.Context, r runner.Runner, dep Dependency) (*Fix, error)
+}
+
+type Fix struct {
+	Description string `json:"description"`
+	Command     string `json:"command,omitempty"`
 }
 
 type CheckSeverity int
@@ -23,46 +28,46 @@ const (
 
 type CommandSuccessful struct {
 	Cmd string
-	Fix string
+	Fix *Fix
 }
 
-func (c CommandSuccessful) Run(ctx context.Context, r runner.Runner, dep Dependency) (string, error) {
+func (c CommandSuccessful) Run(ctx context.Context, r runner.Runner, dep Dependency) (*Fix, error) {
 	_, err := r.Run(ctx, c.Cmd)
 	return c.Fix, err
 }
 
 type BinaryExists struct {
 	Severity CheckSeverity
-	Fix      string
+	Fix      *Fix
 }
 
-func (b BinaryExists) Run(ctx context.Context, r runner.Runner, dep Dependency) (string, error) {
+func (b BinaryExists) Run(ctx context.Context, r runner.Runner, dep Dependency) (*Fix, error) {
 	if err := r.BinaryExists(ctx, dep.Binary); err != nil {
 		if errors.Is(err, runner.ErrTimeout) {
-			return "", err
+			return nil, err
 		}
 		if b.Severity == SeverityWarning {
 			err = WarningError{Err: err}
 		}
 		return b.Fix, err
 	}
-	return "", nil
+	return nil, nil
 }
 
 type VersionMatches struct {
 	CurrentVersion string
 	FetchLatest    func(ctx context.Context) (string, error)
-	Fix            string
+	Fix            *Fix
 }
 
-func (v VersionMatches) Run(ctx context.Context, _ runner.Runner, _ Dependency) (string, error) {
+func (v VersionMatches) Run(ctx context.Context, _ runner.Runner, _ Dependency) (*Fix, error) {
 	latest, err := v.FetchLatest(ctx)
 	if err != nil {
 		logger.Warn(fmt.Sprintf("failed to fetch latest version: %v", err))
-		return "", nil
+		return nil, nil
 	}
 	if latest == v.CurrentVersion {
-		return "", nil
+		return nil, nil
 	}
 
 	return v.Fix, InfoError{Err: fmt.Errorf("out of date - current: %s, latest version: %s", v.CurrentVersion, latest)}

internal/health/dependencies.go

@@ -51,7 +51,10 @@ var HostRequiredDependencies = []Dependency{
 				return version.FetchLatest(ctx, version.ArtifactoryBaseURL)
 			},
 			CurrentVersion: version.Version,
-			Fix:            "run `topo upgrade`",
+			Fix: &Fix{
+				Description: "Upgrade Topo",
+				Command:     "topo upgrade",
+			},
 		}},
 	},
 	{
@@ -74,7 +77,7 @@ var HostRequiredDependencies = []Dependency{
 			BinaryExists{},
 			CommandSuccessful{
 				Cmd: "docker info",
-				Fix: "Ensure current user can run docker commands",
+				Fix: &Fix{Description: "Ensure current user can run docker commands"},
 			},
 		},
 	},
@@ -89,7 +92,7 @@ var TargetRequiredDependencies = []Dependency{
 			BinaryExists{},
 			CommandSuccessful{
 				Cmd: "docker info",
-				Fix: "Ensure current user can run docker commands",
+				Fix: &Fix{Description: "Ensure current user can run docker commands"},
 			},
 		},
 	},
@@ -101,7 +104,10 @@ var TargetRequiredDependencies = []Dependency{
 		Checks: []Check{
 			BinaryExists{
 				Severity: SeverityWarning,
-				Fix:      "run `topo install remoteproc-runtime`",
+				Fix: &Fix{
+					Description: "Install the remoteproc runtime",
+					Command:     "topo install remoteproc-runtime",
+				},
 			},
 		},
 	},
@@ -113,7 +119,10 @@ var TargetRequiredDependencies = []Dependency{
 		Checks: []Check{
 			BinaryExists{
 				Severity: SeverityWarning,
-				Fix:      "run `topo install remoteproc-runtime`",
+				Fix: &Fix{
+					Description: "Install the remoteproc runtime",
+					Command:     "topo install remoteproc-runtime",
+				},
 			},
 		},
 	},
@@ -128,7 +137,7 @@ var TargetRequiredDependencies = []Dependency{
 type DependencyStatus struct {
 	Dependency Dependency
 	Error      error
-	Fix        string
+	Fix        *Fix
 }
 
 func FilterByHardware(deps []Dependency, hardware map[HardwareCapability]struct{}) []Dependency {
@@ -159,7 +168,7 @@ func PerformChecks(ctx context.Context, dependencies []Dependency, runner runner
 			continue
 		}
 
-		var fix string
+		var fix *Fix
 		var err error
 		for _, check := range dep.Checks {
 			fix, err = check.Run(ctx, runner, dep)

internal/health/dependencies_test.go

@@ -126,7 +126,7 @@ func TestPerformChecks(t *testing.T) {
 			Checks: []health.Check{
 				health.BinaryExists{
 					Severity: health.SeverityWarning,
-					Fix:      "turn Anakin into a bad man",
+					Fix:      &health.Fix{Description: "turn Anakin into a bad man"},
 				},
 			},
 		}
@@ -135,7 +135,33 @@ func TestPerformChecks(t *testing.T) {
 		got := health.PerformChecks(context.Background(), []health.Dependency{dep}, runner)
 
 		assert.Len(t, got, 1)
-		assert.Equal(t, "turn Anakin into a bad man", got[0].Fix)
+		assert.Equal(t, &health.Fix{Description: "turn Anakin into a bad man"}, got[0].Fix)
+	})
+
+	t.Run("captures fix command from failing check", func(t *testing.T) {
+		dep := health.Dependency{
+			Binary: "remoteproc-runtime",
+			Label:  "Remoteproc Runtime",
+			Checks: []health.Check{
+				health.BinaryExists{
+					Severity: health.SeverityWarning,
+					Fix: &health.Fix{
+						Description: "Install the remoteproc runtime",
+						Command:     "topo install remoteproc-runtime",
+					},
+				},
+			},
+		}
+		runner := &runner.Fake{}
+
+		got := health.PerformChecks(context.Background(), []health.Dependency{dep}, runner)
+
+		assert.Len(t, got, 1)
+		want := &health.Fix{
+			Description: "Install the remoteproc runtime",
+			Command:     "topo install remoteproc-runtime",
+		}
+		assert.Equal(t, want, got[0].Fix)
 	})
 
 	t.Run("checks dependency with no SoftwarePrerequisites unconditionally", func(t *testing.T) {
@@ -160,7 +186,7 @@ func TestPerformChecks(t *testing.T) {
 			Label:  "Air Fryer Engine",
 			Checks: []health.Check{health.BinaryExists{}, health.CommandSuccessful{
 				Cmd: "potatoes --cook-well",
-				Fix: "Ensure current user can run the potatoe cooker",
+				Fix: &health.Fix{Description: "Ensure current user can run the potatoe cooker"},
 			}},
 		}
 		runner := &runner.Fake{
@@ -178,7 +204,7 @@ func TestPerformChecks(t *testing.T) {
 			{
 				Dependency: dep,
 				Error:      errors.New("permission denied"),
-				Fix:        "Ensure current user can run the potatoe cooker",
+				Fix:        &health.Fix{Description: "Ensure current user can run the potatoe cooker"},
 			},
 		}
 		assert.Equal(t, want, got)

internal/health/health.go

@@ -32,7 +32,7 @@ type HealthCheck struct {
 	Name   string      `json:"name"`
 	Status CheckStatus `json:"status"`
 	Value  string      `json:"value"`
-	Fix    string      `json:"fix,omitempty"`
+	Fix    *Fix        `json:"fix,omitempty"`
 }
 
 type HostReport struct {
@@ -159,11 +159,20 @@ func connectivityCheck(status ConnectionStatus) HealthCheck {
 	check.Value = status.Error.Error()
 	switch {
 	case errors.Is(status.Error, probe.ErrAuthFailed):
-		check.Fix = fmt.Sprintf("run `topo setup-keys --target %s` to configure ssh keys", status.Destination)
+		check.Fix = &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 = fmt.Sprintf("run `topo health --target %s --accept-new-host-keys` to trust the target's identity", status.Destination)
+		check.Fix = &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 = fmt.Sprintf("run `ssh-keygen -R %s` to remove the old host key, then retry", status.Destination.Host)
+		check.Fix = &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
 }

internal/health/health_test.go

@@ -87,7 +87,8 @@ func TestGenerateTargetReport(t *testing.T) {
 		got := health.GenerateTargetReport(ts)
 
 		assert.Equal(t, health.CheckStatusError, got.Connectivity.Status)
-		assert.Contains(t, got.Connectivity.Fix, "topo setup-keys --target ssh://user@my-target")
+		assert.Equal(t, "Configure SSH keys on remote target", got.Connectivity.Fix.Description)
+		assert.Equal(t, "topo setup-keys --target ssh://user@my-target", got.Connectivity.Fix.Command)
 	})
 
 	t.Run("when host key is new, Connectivity includes an accept-new-host-keys fix", func(t *testing.T) {
@@ -101,7 +102,8 @@ func TestGenerateTargetReport(t *testing.T) {
 		got := health.GenerateTargetReport(ts)
 
 		assert.Equal(t, health.CheckStatusError, got.Connectivity.Status)
-		assert.Equal(t, "run `topo health --target ssh://user@my-target --accept-new-host-keys` to trust the target's identity", got.Connectivity.Fix)
+		assert.Equal(t, "Trust the target's SSH host key", got.Connectivity.Fix.Description)
+		assert.Equal(t, "topo health --target ssh://user@my-target --accept-new-host-keys", got.Connectivity.Fix.Command)
 	})
 
 	t.Run("when host key has changed, Connectivity includes a known_hosts fix", func(t *testing.T) {
@@ -115,7 +117,8 @@ func TestGenerateTargetReport(t *testing.T) {
 		got := health.GenerateTargetReport(ts)
 
 		assert.Equal(t, health.CheckStatusError, got.Connectivity.Status)
-		assert.Equal(t, "run `ssh-keygen -R my-target` to remove the old host key, then retry", got.Connectivity.Fix)
+		assert.Equal(t, "Remove the old SSH host key from known_hosts, then retry", got.Connectivity.Fix.Description)
+		assert.Equal(t, "ssh-keygen -R my-target", got.Connectivity.Fix.Command)
 	})
 }
 
@@ -130,6 +133,36 @@ func TestHostReport(t *testing.T) {
 			want := `{ "dependencies": [] }`
 			assert.JSONEq(t, want, string(b))
 		})
+
+		t.Run("omits command when fix has no command", func(t *testing.T) {
+			tr := health.HostReport{Dependencies: []health.HealthCheck{
+				{
+					Name:   "Container Engine",
+					Status: health.CheckStatusError,
+					Value:  "permission denied",
+					Fix: &health.Fix{
+						Description: "Ensure current user can run docker commands",
+					},
+				},
+			}}
+
+			b, err := json.Marshal(tr)
+
+			require.NoError(t, err)
+			want := `{
+				"dependencies": [
+					{
+						"name": "Container Engine",
+						"status": "error",
+						"value": "permission denied",
+						"fix": {
+							"description": "Ensure current user can run docker commands"
+						}
+					}
+				]
+			}`
+			assert.JSONEq(t, want, string(b))
+		})
 	})
 }
 
@@ -180,14 +213,25 @@ func testDependencyReporting(t *testing.T, extract func([]health.DependencyStatu
 			{
 				Dependency: health.Dependency{Binary: "pizza", Label: "Food"},
 				Error:      health.WarningError{Err: errors.New("not enough pineapple")},
-				Fix:        "add more pineapple",
+				Fix: &health.Fix{
+					Description: "add more pineapple",
+					Command:     "pizza --pineapple",
+				},
 			},
 		}
 
 		got := extract(statuses)
 
 		want := []health.HealthCheck{
-			{Name: "Food", Status: health.CheckStatusWarning, Value: "not enough pineapple", Fix: "add more pineapple"},
+			{
+				Name:   "Food",
+				Status: health.CheckStatusWarning,
+				Value:  "not enough pineapple",
+				Fix: &health.Fix{
+					Description: "add more pineapple",
+					Command:     "pizza --pineapple",
+				},
+			},
 		}
 		assert.Equal(t, want, got)
 	})

internal/output/templates/health.go

@@ -17,9 +17,12 @@ type PrintableHealthReport struct {
 
 const healthCheckTemplate = `
 {{- define "checkRow" -}}
-  {{ .Name }}:{{ statusIcon .Status }}{{- if .Value }} ({{ .Value }}){{- end }}
+{{ .Name }}:{{ statusIcon .Status }}{{- if .Value }} ({{ .Value }}){{- end }}
 {{- if .Fix }}
-  → {{ .Fix }}
+  Fix: {{ .Fix.Description }}
+  {{- if .Fix.Command }}
+  Cmd: {{ .Fix.Command }}
+  {{- end }}
 {{- end -}}
 {{- end -}}
 Host

internal/output/templates/health_test.go

@@ -142,7 +142,14 @@ func TestPrintHealthReport(t *testing.T) {
 			toPrint := templates.PrintableHealthReport{
 				Host: health.HostReport{
 					Dependencies: []health.HealthCheck{
-						{Fix: "Apply Working Hands Cream"},
+						{
+							Name:   "Skin Care",
+							Status: health.CheckStatusWarning,
+							Fix: &health.Fix{
+								Description: "Apply Working Hands Cream",
+								Command:     "topo moisturise",
+							},
+						},
 					},
 				},
 			}
@@ -151,7 +158,9 @@ func TestPrintHealthReport(t *testing.T) {
 			err := printable.Print(toPrint, &out, term.Plain)
 
 			require.NoError(t, err)
-			assert.Contains(t, out.String(), "Apply Working Hands Cream")
+			assert.Contains(t, out.String(), "Skin Care: ⚠️")
+			assert.Contains(t, out.String(), "  Fix: Apply Working Hands Cream")
+			assert.Contains(t, out.String(), "  Cmd: topo moisturise")
 		})
 
 		t.Run("when no target is specified, prints the hint", func(t *testing.T) {