Skip to content

Commit 8899114

Browse files
federicobozzinifedericobozzini
authored
feat!: add explicit command to fix target health issues (#261)
Signed-off-by: Federico Bozzini <federico.bozzini@arm.com>
1 parent 28f58e7 commit 8899114

8 files changed

Lines changed: 144 additions & 39 deletions

File tree

‎e2e/setup_keys_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,14 @@ func TestSetupKeysJourney(t *testing.T) {
2020
Step(t, "health reports unknown host key and suggests accept-new-host-keys")
2121
out := runTopo(t, topo, "health", "--target", container.SSHDestination)
2222
assert.Contains(t, out, "Connectivity: ❌ (ssh host key is unknown)")
23-
wantFix := fmt.Sprintf("run `topo health --target %s --accept-new-host-keys", container.SSHDestination)
24-
assert.Contains(t, out, wantFix)
23+
assert.Contains(t, out, "Fix: Trust the target's SSH host key")
24+
assert.Contains(t, out, fmt.Sprintf("Cmd: topo health --target %s --accept-new-host-keys", container.SSHDestination))
2525

2626
Step(t, "health with accept-new-host-keys trusts host and suggests setup-keys")
2727
out = runTopo(t, topo, "health", "--target", container.SSHDestination, "--accept-new-host-keys")
2828
assert.Contains(t, out, "Connectivity: ❌ (ssh authentication failed)")
29-
wantFix = fmt.Sprintf("run `topo setup-keys --target %s`", container.SSHDestination)
30-
assert.Contains(t, out, wantFix)
29+
assert.Contains(t, out, "Fix: Configure SSH keys on remote target")
30+
assert.Contains(t, out, fmt.Sprintf("Cmd: topo setup-keys --target %s", container.SSHDestination))
3131

3232
Step(t, "setup-keys generates keys and installs them on the target")
3333
askpass := writeAskPassScript(t, sshRootPassword)

‎internal/health/check.go‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,12 @@ import (
1111
)
1212

1313
type Check interface {
14-
Run(ctx context.Context, r runner.Runner, dep Dependency) (string, error)
14+
Run(ctx context.Context, r runner.Runner, dep Dependency) (*Fix, error)
15+
}
16+
17+
type Fix struct {
18+
Description string `json:"description"`
19+
Command string `json:"command,omitempty"`
1520
}
1621

1722
type CheckSeverity int
@@ -23,46 +28,46 @@ const (
2328

2429
type CommandSuccessful struct {
2530
Cmd string
26-
Fix string
31+
Fix *Fix
2732
}
2833

29-
func (c CommandSuccessful) Run(ctx context.Context, r runner.Runner, dep Dependency) (string, error) {
34+
func (c CommandSuccessful) Run(ctx context.Context, r runner.Runner, dep Dependency) (*Fix, error) {
3035
_, err := r.Run(ctx, c.Cmd)
3136
return c.Fix, err
3237
}
3338

3439
type BinaryExists struct {
3540
Severity CheckSeverity
36-
Fix string
41+
Fix *Fix
3742
}
3843

39-
func (b BinaryExists) Run(ctx context.Context, r runner.Runner, dep Dependency) (string, error) {
44+
func (b BinaryExists) Run(ctx context.Context, r runner.Runner, dep Dependency) (*Fix, error) {
4045
if err := r.BinaryExists(ctx, dep.Binary); err != nil {
4146
if errors.Is(err, runner.ErrTimeout) {
42-
return "", err
47+
return nil, err
4348
}
4449
if b.Severity == SeverityWarning {
4550
err = WarningError{Err: err}
4651
}
4752
return b.Fix, err
4853
}
49-
return "", nil
54+
return nil, nil
5055
}
5156

5257
type VersionMatches struct {
5358
CurrentVersion string
5459
FetchLatest func(ctx context.Context) (string, error)
55-
Fix string
60+
Fix *Fix
5661
}
5762

58-
func (v VersionMatches) Run(ctx context.Context, _ runner.Runner, _ Dependency) (string, error) {
63+
func (v VersionMatches) Run(ctx context.Context, _ runner.Runner, _ Dependency) (*Fix, error) {
5964
latest, err := v.FetchLatest(ctx)
6065
if err != nil {
6166
logger.Warn(fmt.Sprintf("failed to fetch latest version: %v", err))
62-
return "", nil
67+
return nil, nil
6368
}
6469
if latest == v.CurrentVersion {
65-
return "", nil
70+
return nil, nil
6671
}
6772

6873
return v.Fix, InfoError{Err: fmt.Errorf("out of date - current: %s, latest version: %s", v.CurrentVersion, latest)}

‎internal/health/dependencies.go‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,10 @@ var HostRequiredDependencies = []Dependency{
5151
return version.FetchLatest(ctx, version.ArtifactoryBaseURL)
5252
},
5353
CurrentVersion: version.Version,
54-
Fix: "run `topo upgrade`",
54+
Fix: &Fix{
55+
Description: "Upgrade Topo",
56+
Command: "topo upgrade",
57+
},
5558
}},
5659
},
5760
{
@@ -74,7 +77,7 @@ var HostRequiredDependencies = []Dependency{
7477
BinaryExists{},
7578
CommandSuccessful{
7679
Cmd: "docker info",
77-
Fix: "Ensure current user can run docker commands",
80+
Fix: &Fix{Description: "Ensure current user can run docker commands"},
7881
},
7982
},
8083
},
@@ -89,7 +92,7 @@ var TargetRequiredDependencies = []Dependency{
8992
BinaryExists{},
9093
CommandSuccessful{
9194
Cmd: "docker info",
92-
Fix: "Ensure current user can run docker commands",
95+
Fix: &Fix{Description: "Ensure current user can run docker commands"},
9396
},
9497
},
9598
},
@@ -101,7 +104,10 @@ var TargetRequiredDependencies = []Dependency{
101104
Checks: []Check{
102105
BinaryExists{
103106
Severity: SeverityWarning,
104-
Fix: "run `topo install remoteproc-runtime`",
107+
Fix: &Fix{
108+
Description: "Install the remoteproc runtime",
109+
Command: "topo install remoteproc-runtime",
110+
},
105111
},
106112
},
107113
},
@@ -113,7 +119,10 @@ var TargetRequiredDependencies = []Dependency{
113119
Checks: []Check{
114120
BinaryExists{
115121
Severity: SeverityWarning,
116-
Fix: "run `topo install remoteproc-runtime`",
122+
Fix: &Fix{
123+
Description: "Install the remoteproc runtime",
124+
Command: "topo install remoteproc-runtime",
125+
},
117126
},
118127
},
119128
},
@@ -128,7 +137,7 @@ var TargetRequiredDependencies = []Dependency{
128137
type DependencyStatus struct {
129138
Dependency Dependency
130139
Error error
131-
Fix string
140+
Fix *Fix
132141
}
133142

134143
func FilterByHardware(deps []Dependency, hardware map[HardwareCapability]struct{}) []Dependency {
@@ -159,7 +168,7 @@ func PerformChecks(ctx context.Context, dependencies []Dependency, runner runner
159168
continue
160169
}
161170

162-
var fix string
171+
var fix *Fix
163172
var err error
164173
for _, check := range dep.Checks {
165174
fix, err = check.Run(ctx, runner, dep)

‎internal/health/dependencies_test.go‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ func TestPerformChecks(t *testing.T) {
126126
Checks: []health.Check{
127127
health.BinaryExists{
128128
Severity: health.SeverityWarning,
129-
Fix: "turn Anakin into a bad man",
129+
Fix: &health.Fix{Description: "turn Anakin into a bad man"},
130130
},
131131
},
132132
}
@@ -135,7 +135,33 @@ func TestPerformChecks(t *testing.T) {
135135
got := health.PerformChecks(context.Background(), []health.Dependency{dep}, runner)
136136

137137
assert.Len(t, got, 1)
138-
assert.Equal(t, "turn Anakin into a bad man", got[0].Fix)
138+
assert.Equal(t, &health.Fix{Description: "turn Anakin into a bad man"}, got[0].Fix)
139+
})
140+
141+
t.Run("captures fix command from failing check", func(t *testing.T) {
142+
dep := health.Dependency{
143+
Binary: "remoteproc-runtime",
144+
Label: "Remoteproc Runtime",
145+
Checks: []health.Check{
146+
health.BinaryExists{
147+
Severity: health.SeverityWarning,
148+
Fix: &health.Fix{
149+
Description: "Install the remoteproc runtime",
150+
Command: "topo install remoteproc-runtime",
151+
},
152+
},
153+
},
154+
}
155+
runner := &runner.Fake{}
156+
157+
got := health.PerformChecks(context.Background(), []health.Dependency{dep}, runner)
158+
159+
assert.Len(t, got, 1)
160+
want := &health.Fix{
161+
Description: "Install the remoteproc runtime",
162+
Command: "topo install remoteproc-runtime",
163+
}
164+
assert.Equal(t, want, got[0].Fix)
139165
})
140166

141167
t.Run("checks dependency with no SoftwarePrerequisites unconditionally", func(t *testing.T) {
@@ -160,7 +186,7 @@ func TestPerformChecks(t *testing.T) {
160186
Label: "Air Fryer Engine",
161187
Checks: []health.Check{health.BinaryExists{}, health.CommandSuccessful{
162188
Cmd: "potatoes --cook-well",
163-
Fix: "Ensure current user can run the potatoe cooker",
189+
Fix: &health.Fix{Description: "Ensure current user can run the potatoe cooker"},
164190
}},
165191
}
166192
runner := &runner.Fake{
@@ -178,7 +204,7 @@ func TestPerformChecks(t *testing.T) {
178204
{
179205
Dependency: dep,
180206
Error: errors.New("permission denied"),
181-
Fix: "Ensure current user can run the potatoe cooker",
207+
Fix: &health.Fix{Description: "Ensure current user can run the potatoe cooker"},
182208
},
183209
}
184210
assert.Equal(t, want, got)

‎internal/health/health.go‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ type HealthCheck struct {
3232
Name string `json:"name"`
3333
Status CheckStatus `json:"status"`
3434
Value string `json:"value"`
35-
Fix string `json:"fix,omitempty"`
35+
Fix *Fix `json:"fix,omitempty"`
3636
}
3737

3838
type HostReport struct {
@@ -161,11 +161,20 @@ func connectivityCheck(status ConnectionStatus) HealthCheck {
161161
check.Value = status.Error.Error()
162162
switch {
163163
case errors.Is(status.Error, probe.ErrAuthFailed):
164-
check.Fix = fmt.Sprintf("run `topo setup-keys --target %s` to configure ssh keys", status.Destination)
164+
check.Fix = &Fix{
165+
Description: "Configure SSH keys on remote target",
166+
Command: fmt.Sprintf("topo setup-keys --target %s", status.Destination),
167+
}
165168
case errors.Is(status.Error, probe.ErrHostKeyUnknown):
166-
check.Fix = fmt.Sprintf("run `topo health --target %s --accept-new-host-keys` to trust the target's identity", status.Destination)
169+
check.Fix = &Fix{
170+
Description: "Trust the target's SSH host key",
171+
Command: fmt.Sprintf("topo health --target %s --accept-new-host-keys", status.Destination),
172+
}
167173
case errors.Is(status.Error, probe.ErrHostKeyChanged):
168-
check.Fix = fmt.Sprintf("run `ssh-keygen -R %s` to remove the old host key, then retry", status.Destination.Host)
174+
check.Fix = &Fix{
175+
Description: "Remove the old SSH host key from known_hosts, then retry",
176+
Command: fmt.Sprintf("ssh-keygen -R %s", status.Destination.Host),
177+
}
169178
}
170179
return check
171180
}

‎internal/health/health_test.go‎

Lines changed: 49 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,8 @@ func TestGenerateTargetReport(t *testing.T) {
8787
got := health.GenerateTargetReport(ts)
8888

8989
assert.Equal(t, health.CheckStatusError, got.Connectivity.Status)
90-
assert.Contains(t, got.Connectivity.Fix, "topo setup-keys --target ssh://user@my-target")
90+
assert.Equal(t, "Configure SSH keys on remote target", got.Connectivity.Fix.Description)
91+
assert.Equal(t, "topo setup-keys --target ssh://user@my-target", got.Connectivity.Fix.Command)
9192
})
9293

9394
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) {
101102
got := health.GenerateTargetReport(ts)
102103

103104
assert.Equal(t, health.CheckStatusError, got.Connectivity.Status)
104-
assert.Equal(t, "run `topo health --target ssh://user@my-target --accept-new-host-keys` to trust the target's identity", got.Connectivity.Fix)
105+
assert.Equal(t, "Trust the target's SSH host key", got.Connectivity.Fix.Description)
106+
assert.Equal(t, "topo health --target ssh://user@my-target --accept-new-host-keys", got.Connectivity.Fix.Command)
105107
})
106108

107109
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) {
115117
got := health.GenerateTargetReport(ts)
116118

117119
assert.Equal(t, health.CheckStatusError, got.Connectivity.Status)
118-
assert.Equal(t, "run `ssh-keygen -R my-target` to remove the old host key, then retry", got.Connectivity.Fix)
120+
assert.Equal(t, "Remove the old SSH host key from known_hosts, then retry", got.Connectivity.Fix.Description)
121+
assert.Equal(t, "ssh-keygen -R my-target", got.Connectivity.Fix.Command)
119122
})
120123
}
121124

@@ -130,6 +133,36 @@ func TestHostReport(t *testing.T) {
130133
want := `{ "dependencies": [] }`
131134
assert.JSONEq(t, want, string(b))
132135
})
136+
137+
t.Run("omits command when fix has no command", func(t *testing.T) {
138+
tr := health.HostReport{Dependencies: []health.HealthCheck{
139+
{
140+
Name: "Container Engine",
141+
Status: health.CheckStatusError,
142+
Value: "permission denied",
143+
Fix: &health.Fix{
144+
Description: "Ensure current user can run docker commands",
145+
},
146+
},
147+
}}
148+
149+
b, err := json.Marshal(tr)
150+
151+
require.NoError(t, err)
152+
want := `{
153+
"dependencies": [
154+
{
155+
"name": "Container Engine",
156+
"status": "error",
157+
"value": "permission denied",
158+
"fix": {
159+
"description": "Ensure current user can run docker commands"
160+
}
161+
}
162+
]
163+
}`
164+
assert.JSONEq(t, want, string(b))
165+
})
133166
})
134167
}
135168

@@ -180,14 +213,25 @@ func testDependencyReporting(t *testing.T, extract func([]health.DependencyStatu
180213
{
181214
Dependency: health.Dependency{Binary: "pizza", Label: "Food"},
182215
Error: health.WarningError{Err: errors.New("not enough pineapple")},
183-
Fix: "add more pineapple",
216+
Fix: &health.Fix{
217+
Description: "add more pineapple",
218+
Command: "pizza --pineapple",
219+
},
184220
},
185221
}
186222

187223
got := extract(statuses)
188224

189225
want := []health.HealthCheck{
190-
{Name: "Food", Status: health.CheckStatusWarning, Value: "not enough pineapple", Fix: "add more pineapple"},
226+
{
227+
Name: "Food",
228+
Status: health.CheckStatusWarning,
229+
Value: "not enough pineapple",
230+
Fix: &health.Fix{
231+
Description: "add more pineapple",
232+
Command: "pizza --pineapple",
233+
},
234+
},
191235
}
192236
assert.Equal(t, want, got)
193237
})

‎internal/output/templates/health.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,12 @@ type PrintableHealthReport struct {
1717

1818
const healthCheckTemplate = `
1919
{{- define "checkRow" -}}
20-
{{ .Name }}:{{ statusIcon .Status }}{{- if .Value }} ({{ .Value }}){{- end }}
20+
{{ .Name }}:{{ statusIcon .Status }}{{- if .Value }} ({{ .Value }}){{- end }}
2121
{{- if .Fix }}
22-
→ {{ .Fix }}
22+
Fix: {{ .Fix.Description }}
23+
{{- if .Fix.Command }}
24+
Cmd: {{ .Fix.Command }}
25+
{{- end }}
2326
{{- end -}}
2427
{{- end -}}
2528
Host

‎internal/output/templates/health_test.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,14 @@ func TestPrintHealthReport(t *testing.T) {
154154
toPrint := templates.PrintableHealthReport{
155155
Host: health.HostReport{
156156
Dependencies: []health.HealthCheck{
157-
{Fix: "Apply Working Hands Cream"},
157+
{
158+
Name: "Skin Care",
159+
Status: health.CheckStatusWarning,
160+
Fix: &health.Fix{
161+
Description: "Apply Working Hands Cream",
162+
Command: "topo moisturise",
163+
},
164+
},
158165
},
159166
},
160167
}
@@ -163,7 +170,9 @@ func TestPrintHealthReport(t *testing.T) {
163170
err := printable.Print(toPrint, &out, term.Plain)
164171

165172
require.NoError(t, err)
166-
assert.Contains(t, out.String(), "Apply Working Hands Cream")
173+
assert.Contains(t, out.String(), "Skin Care: ⚠️")
174+
assert.Contains(t, out.String(), " Fix: Apply Working Hands Cream")
175+
assert.Contains(t, out.String(), " Cmd: topo moisturise")
167176
})
168177

169178
t.Run("when no target is specified, prints the hint", func(t *testing.T) {

0 commit comments

Comments
 (0)