refactor(status): move checks to operations - #561
Conversation
* CardID: CCT-2407 Move status collection behind a clean GetStatus API while keeping dependency injection private for tests. Keep formatting, spinner, and exit-code handling in the CLI, rendering output after collection completes.
e69bfb5 to
25013e9
Compare
zpetrace
left a comment
zpetrace
left a comment
There was a problem hiding this comment.
Mainly looking good, I am not that oriented in the code itself so my points are directed mainly towards testing but apart from few minor suggestions it looks good, thanks!:)
| if err != nil { | ||
| systemStatus.returnCode += 1 | ||
| systemStatus.ContentError = err.Error() | ||
| return fmt.Errorf("unable to check content management: %w", err) | ||
| } |
There was a problem hiding this comment.
Formatters are well tested, but statusAction itself isn’t (spinner, human path, JSON branch). Even a small test asserting exit code + stdout for --format json would be good IMO
| func contentIsEnabled() (bool, error) { | ||
| service, err := subman.NewRHSMClient() | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| return service.IsContentManagementEnabled() | ||
| } |
There was a problem hiding this comment.
Doesn't contentIsEnabled open a second subman.NewRHSMClient() while registration uses IsRegistered()? Wouldn't it be better share one client per getStatus() call?
| { | ||
| name: "rhsm connected", | ||
| render: func() { formatRHSMStatus(operations.StatusReport{RHSMConnected: true}) }, | ||
| want: " [✓] Connected to Red Hat Subscription Management\n", |
There was a problem hiding this comment.
just a suggestion: want strings pin literal ✓ / 𐄂 but IMO it would be better to use ui.Icons.Ok / ui.Icons.Error in expected output, or assert on message text with strings.Contains
There was a problem hiding this comment.
The same goes for lines 57,72
|
|
||
| // TestFormatStatusLines pins the exact human-readable line each formatter prints | ||
| // for its success, negative, and error states. | ||
| func TestFormatStatusLines(t *testing.T) { |
There was a problem hiding this comment.
Only format* helpers are tested here — see status_cmd.go:57 for statusAction / JSON coverage.
| if contentEnabled && report.RHSMConnected { | ||
| report.ContentEnabled = true | ||
| slog.Info("System has access to content") | ||
| } else { | ||
| report.ContentEnabled = false | ||
| slog.Info("System has no access to content") | ||
| } |
There was a problem hiding this comment.
ContentEnabled requires contentEnabled && RHSMConnected. Content disabled alone doesn’t increment failedChecks (parity with old returnCode). Maybe worth a test like RHSM not registered + content enabled → ContentEnabled == false, failedChecks == 1?
2 participants
Move status collection behind a clean GetStatus API while keeping
dependency injection private for tests. Keep formatting, spinner, and
exit-code handling in the CLI, rendering output after collection completes.