fix: implement real functionality for kdm logs and kdm health commands (#1)#40
Merged
Merged
Conversation
- Wire real Docker/K8s clients in health.ts and logs.ts - Independent try/catch blocks so Docker shows even when K8s is down - Add .npmrc ignore-scripts=true to prevent ssh2 segfault on Node v20 - Expand health.test.ts: vi.mocked(), empty/error/per-target coverage - Trim .coderabbit.yaml tone_instructions to ≤ 250 chars Fixes KDM-cli#1
- Add real expected output with color-coded table examples - Document Docker-first + Kubernetes fallback behavior in logs - Add error table with causes and fixes for both commands - Remove placeholder dummy output from previous docs
github-actions
Bot
added
documentation
Contributor
|
Caution Review failedPull request was closed or merged during review Note
|
| Layer / File(s) | Summary |
|---|---|
Health command implementation src/commands/health.ts |
healthColor() maps status strings to chalk colors; showHealth(target) validates target against ['all','containers','pods'], fetches containers and/or pods with separate try/catch (logs Docker/Kubernetes unavailable warnings), formats rows (TYPE, NAME, HEALTH, DETAILS including pod namespace/restarts), uses a spinner, warns if no rows, and calls renderTable when rows exist. |
Health command registration src/commands/health.ts |
registerHealthCommand(program) now returns void and wires health <target> to .action(showHealth) with updated help text. |
Health command test suite src/__tests__/health.test.ts |
Vitest-based tests add mocks for getRunningContainers, getRunningPods, logger, renderTable, and spinner; assert command registration, rendering for all/containers/pods, no-result warning, unknown-target error, container/pod fetch rejection warnings, and partial-success behavior (Docker fail + pods succeed). |
Logs command implementation src/commands/logs.ts |
Adds printStream(value) to write coerced output to stdout; showLogs(name) validates input, starts spinner, attempts Docker container log retrieval (match by ID prefix or container name), prints logs, debug-logs Docker failures and falls back to Kubernetes readNamespacedPodLog({ tailLines: 100 }), prints response.body, and handles spinner/error lifecycle without rethrowing Kubernetes errors. |
Logs command registration src/commands/logs.ts |
registerLogsCommand(program) now returns void, updates the logs <name> description to mention container ID prefix/name and pod name matching, and wires .action(showLogs). |
Sequence Diagram(s)
sequenceDiagram
participant showHealth as showHealth
participant Spinner
participant Containers as getRunningContainers()
participant Pods as getRunningPods()
participant Table as renderTable
showHealth->>Spinner: start()
showHealth->>Containers: fetch (try/catch)
showHealth->>Pods: fetch (try/catch)
showHealth->>Table: renderTable(rows: TYPE, NAME, HEALTH, DETAILS)
Table-->>showHealth: rendered
showHealth->>Spinner: stop()
sequenceDiagram
participant showLogs as showLogs
participant Spinner
participant Docker as Docker API
participant Print as printStream
participant K8s as Kubernetes API
showLogs->>Spinner: start()
showLogs->>Docker: listContainers() / get container logs
alt Docker Success
Docker-->>Print: log payload
Print->>showLogs: write stdout
else Docker Fails
showLogs->>showLogs: logger.debug Docker failure
showLogs->>K8s: readNamespacedPodLog(opts: { tailLines:100 })
K8s-->>Print: pod log payload
Print->>showLogs: write stdout
end
showLogs->>Spinner: stop()
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
🩺 Health tables hum in colored rows,
Docker may grumble, K8s still flows.
Spinners spin, the logs cascade,
Tests assert the paths we made.
Small CLI magic, neatly played.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately describes the main change: implementing real functionality for the kdm logs and kdm health commands, replacing placeholder implementations with Docker and Kubernetes integrations. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/health.test.ts`:
- Around line 78-174: Add a new test in src/__tests__/health.test.ts that
simulates a mixed-failure for the "health all" command by mocking one backend to
reject (e.g., vi.mocked(getRunningContainers).mockRejectedValue(new Error(...))
or getRunningPods likewise) and the other to resolve with at least one workload,
then call program.parseAsync(['node','test','health','all']) and assert that
logger.warn was called with a message indicating the unavailable backend (e.g.,
contains "Docker unavailable" or "Kubernetes unavailable") and that
tableUtils.renderTable was still called with rows containing the available
workload (use expect.arrayContaining with expect.arrayContaining(['container',
...]) or ['pod', ...] to locate the rendered row); reference
getRunningContainers, getRunningPods, program.parseAsync, logger.warn, and
tableUtils.renderTable to locate and implement the test.
In `@src/commands/health.ts`:
- Around line 49-54: The branch that handles invalid targets (checking
validTargets.includes(target)) currently logs via logger.error but returns
normally; change it to terminate with a non-zero exit status so callers see
failure — e.g., after logging in the same block where validTargets and target
are checked, call process.exit(1) (or throw an Error) instead of returning;
ensure the error log still uses logger.error and include target/validTargets
context as currently done.
In `@src/commands/logs.ts`:
- Around line 1-23: Remove the entire commented-out scaffold block in this file
that defines registerLogsCommand (the commented imports and the commented export
const registerLogsCommand = (program: Command) => { ... }), since it is dead
code and duplicates the real implementation; search for symbols like
registerLogsCommand, program, createSpinner, and logger to locate the commented
section and delete it entirely so only the active implementation remains.
- Around line 38-41: In showLogs, validate that the incoming name string is
non-empty before doing any container lookups; if name is empty or only
whitespace, abort early (e.g., log an error and return) so you don't call
container.Id.startsWith('') which matches every container. Update the input
guard at the top of showLogs to trim the name, check length, and handle the
failure path (stop spinner, logger.error, and return) so downstream code that
iterates containers or calls container.Id.startsWith(name) only runs for a valid
non-empty name.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 14a1b961-be0a-41f1-af94-8d6bbfe1b642
⛔ Files ignored due to path filters (5)
.coderabbit.yamlis excluded by none and included by nonedocs/v1.2.1/health/README.mdis excluded by none and included by nonedocs/v1.2.1/logs/README.mdis excluded by none and included by nonepackage-lock.jsonis excluded by!**/package-lock.json,!package-lock.jsonand included by nonepackage.jsonis excluded by none and included by none
📒 Files selected for processing (3)
src/__tests__/health.test.tssrc/commands/health.tssrc/commands/logs.ts
Comment on lines
+1
to
+23
| // import { Command } from 'commander'; | ||
| // import { logger } from '../utils/logger'; | ||
| // import { createSpinner } from '../ui/spinner'; | ||
|
|
||
| // export const registerLogsCommand = (program: Command) => { | ||
| // program | ||
| // .command('logs <name>') | ||
| // .description('Show logs for a container or pod') | ||
| // .action(async (name) => { | ||
| // const spinner = createSpinner(`Fetching logs for ${name}...`).start(); | ||
| // try { | ||
| // // TODO: Implement actual log fetching logic | ||
| // spinner.stop(`Logs for ${name} fetched`); | ||
| // logger.info(`Showing logs for ${name}...`); | ||
| // } catch (error) { | ||
| // const errorMessage = (error as Error).message; | ||
| // spinner.fail(`Failed to fetch logs for ${name}: ${errorMessage}`); | ||
| // logger.error(`Failed to fetch logs for ${name}: ${errorMessage}`, error); | ||
| // throw error; | ||
| // } | ||
| // }); | ||
| // }; | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Remove commented-out placeholder code.
This dead code clutters the file now that the real implementation is in place. If you ever need the old scaffold, git history has your back.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/logs.ts` around lines 1 - 23, Remove the entire commented-out
scaffold block in this file that defines registerLogsCommand (the commented
imports and the commented export const registerLogsCommand = (program: Command)
=> { ... }), since it is dead code and duplicates the real implementation;
search for symbols like registerLogsCommand, program, createSpinner, and logger
to locate the commented section and delete it entirely so only the active
implementation remains.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR resolves Issue #1 by replacing the placeholder implementations of
kdm logsandkdm healthwith fully functional Docker and Kubernetes integrations.Both commands now fetch and display real runtime data instead of static placeholder messages.
Changes Made
kdm logskdm healthalltarget supportAdditional Improvements
kdm logskdm healthScreenshots
Health testskdm logskdm healthTesting
Tested successfully with:
Summary by CodeRabbit
New Features
Tests