Implement deployment command and enhance diagnostics with JSON support#11
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 2 minutes and 17 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThis PR introduces a complete deployment pipeline, Docker/Compose execution infrastructure, enhanced diagnostics, and related CLI/UI integration. It adds ~2,400 lines implementing Docker build/run/logs, Compose orchestration, persistent deployment history with environment-aware Kubernetes namespace mapping, comprehensive doctor checks with JSON export, and state management for execution output display in the TUI. ChangesDocker & Compose Execution Infrastructure
Deployment Pipeline & History
Diagnostics & Configuration
CLI Integration & Service Execution
Test Coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main.rs (1)
68-178: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the
DoctorandDeployarms into helpers to recovermain's code health.CodeScene is failing the build because
mainregressed (10.00 → 9.18, "Complex Method" + "Bumpy Road Ahead"). The deploy arm alone packs plan validation, pipeline execution, history construction, and persistence into one block. Pulling each arm into its own function flattens the match and clears the gate.♻️ Sketch: lift handlers out of the match
- Some(Command::Doctor { full, json }) => { - let report = if full { - let settings = - config::settings::Settings::load_or_default(&config::paths::config_file())?; - doctor::environment_check::run_full(settings.registry.as_deref()) - } else { - doctor::environment_check::run() - }; - - if json { - ... - } else { - ... - } - } + Some(Command::Doctor { full, json }) => run_doctor(full, json)?, ... - Some(Command::Deploy { environment }) => { - let state = startup::initialize(cli.project)?; - ... - history.save(&history_path)?; - println!("\nDeployment recorded in {}", history_path.display()); - } + Some(Command::Deploy { environment }) => run_deploy(cli.project, &environment)?,Then define
fn run_doctor(full: bool, json: bool) -> anyhow::Result<()>andfn run_deploy(project: PathBuf, environment: &str) -> anyhow::Result<()>carrying the moved bodies.🤖 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/main.rs` around lines 68 - 178, Extract the large Doctor and Deploy match-arm bodies into two helper functions to simplify main: implement fn run_doctor(full: bool, json: bool) -> anyhow::Result<()> which moves the Doctor arm logic (loading Settings, calling doctor::environment_check::run/_full, printing/exporting JSON or render/summary and saving to config::paths::doctor_report_file()), and implement fn run_deploy(project: PathBuf, environment: &str) -> anyhow::Result<()> which moves the Deploy arm logic (calling startup::initialize, building plan via deploy::pipeline::plan, validating plan.ready(), executing deploy::pipeline::execute_pipeline, constructing and saving deploy::history::DeploymentHistory using config::paths::deploy_history_file() and chrono_timestamp(), and printing results). Replace the original match arms (Some(Command::Doctor { .. }) and Some(Command::Deploy { .. })) with calls to these helpers, propagating errors with ? and preserving all behavior (plan blocker reporting, printing, history.record fields, and history.save).src/ui/dashboard.rs (1)
484-698: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftBreak up
render_first_launch— CodeScene gate is failing on its complexity.The hotspot health dropped (8.68 → 7.79) because this single function now does rect math, outer block, ASCII art, subtitle, a six-line capability card, and the action list inline. Extract the self-contained pieces (e.g.
welcome_rect(),render_ascii_banner(), and acapability_line(label, present, palette)helper that collapses the six near-identicaldetails.push(...)blocks) to drop the cognitive load and clear the gate.🤖 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/ui/dashboard.rs` around lines 484 - 698, The function render_first_launch is too large and doing multiple concerns (rect math, banner, subtitle, capability lines, and action list); extract small helpers to reduce complexity: create welcome_rect(area: Rect) -> Rect to compute the centered welcome_area and move the Clear/outer block render into render_outer_block(frame, welcome_area, palette), extract the ASCII art into render_ascii_banner(frame, chunk, palette) and the subtitle block into render_subtitle(frame, chunk, palette), and replace the six near-identical details.push(...) blocks with a capability_line(label: &str, present: bool, palette: theme::Palette) -> Line helper used by render_capabilities_card(frame, chunk, state, palette) to build the details vector; keep render_first_launch calling these new functions in sequence so behavior is unchanged but complexity is reduced.
🤖 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/compose/logs.rs`:
- Around line 36-53: The command result currently ignores non-zero exit codes
and returns stderr as if it were log lines; in the function that runs
Command::new("docker") and collects output (the block producing `output`,
`stdout`, `stderr`, `combined`, and `lines`), check `output.status.success()`
and if false return an anyhow/miette error (include stderr and a contextual
message like "docker compose logs failed") instead of folding stderr into log
lines; also address the unused `follow` field on `ComposeLogRequest` — either
remove `follow` from that code path or wire it into the log fetching logic so
`fetch`/the log function respects follow mode and the docstring remains correct.
In `@src/compose/services.rs`:
- Around line 30-45: The running() function currently ignores the child process
exit status and returns an empty Vec on command failures; update running() to
check output.status like list() does, returning an Err when status is
non-success. Specifically, after Command::output() in running(), inspect
output.status and if !status.success() return an anyhow/miette error (including
output.stderr and a descriptive message) so calling code sees the docker compose
ps failure rather than an empty services list.
In `@src/deploy/environments.rs`:
- Around line 15-21: from_string currently returns Environment::Development
silently on unknown inputs; update it to keep the same fallback behavior but
emit a warning when the input doesn't match any known variant. Modify the
match's default arm in from_string to log a warning (including the original s
value) before returning Environment::Development so typos like "produciton" are
visible; use the project's logging facility (e.g., tracing::warn! or log::warn!)
consistent with other code.
- Around line 6-12: The resolve_namespace function
(resolve_namespace(Environment)) is currently unused except in tests
(tests/deployment_pipeline.rs::environment_resolves_to_namespace), so either
wire it into the deploy flow or delete it and update tests; to fix, update the
deploy handler where the Kubernetes namespace is chosen (the deploy handler that
creates/patches k8s resources) to call resolve_namespace(&env) and use its
return value for namespace selection, or remove resolve_namespace and change
tests to assert the deploy handler's existing namespace logic (or remove the
test); reference resolve_namespace, Environment, and the deploy handler /
tests/deployment_pipeline.rs when making the change.
In `@src/deploy/pipeline.rs`:
- Around line 247-275: The current execute_deployment_update_step allows
applying the whole repo when k8s/ is missing and doesn't use the resolved
namespace; update execute_deployment_update_step (and similarly
execute_rollout_verification_step) to 1) restrict manifest selection to only
k8s/ (fail fast with an Err if k8s/ is absent) or implement a safe scan that
only picks files with Kubernetes resource markers, 2) accept and thread a
resolved namespace (obtain via resolve_namespace in src/deploy/environments.rs)
into the kubectl invocations by adding the "-n" / "--namespace" flag to both
kubectl apply and kubectl rollout status calls, and 3) add unit tests that mock
the command execution path to assert manifest selection behavior and that
kubectl is invoked with the expected "-n <namespace>" argument (abstract the
Command invocation behind a trait or helper to enable mocking). Ensure
references: function names execute_deployment_update_step,
execute_rollout_verification_step, types ProjectContext/ProjectCapabilities, and
resolve_namespace to locate code to change.
- Around line 277-289: The rollout verification currently calls kubectl without
a target, so update execute_rollout_verification_step to accept (or receive) the
Deployment name and namespace and pass them into the Command args (e.g.,
"rollout", "status", format!("deployment/{}", name), "-n", namespace,
"--timeout=120s"); ensure the function signature and all call sites are adjusted
to provide the name and namespace and preserve the same error handling and
output parsing (use String::from_utf8_lossy(&output.stderr) and
output.status.success() as before).
In `@src/docker/build.rs`:
- Around line 28-71: Refactor the duplicated logic in execute and rebuild by
extracting a helper (e.g., run_docker_build or docker_build_with_args) that
accepts BuildRequest, project_root, and an iterator/Vec of extra docker args (so
rebuild can pass "--no-cache"); move the common timing, Command::new("docker"),
current_dir, output capture, stdout/stderr merging, and BuildResult construction
into that helper; then implement execute to call the helper with ["build", "-t",
&full_tag, "."] (or no extra flag) and rebuild to call it with ["build",
"--no-cache", "-t", &full_tag, "."] so plumbing changes (timeout, env, buildkit)
are made once in the helper.
In `@src/docker/containers.rs`:
- Around line 112-123: The test container_summary_fields creates a
ContainerSummary but only asserts id and name; update the test
(container_summary_fields) to also assert the fields added in this PR by
checking container.image, container.status, and container.ports match
"nginx:latest", "Up 5 minutes", and "0.0.0.0:80->80/tcp" respectively so the new
parsing/format string stays covered.
- Around line 15-91: Both list() and list_all() duplicate the same docker ps
invocation/format, success-check, and parsing logic; extract a private helper
(e.g., fn list_with_args(args: &[&str]) -> Result<Vec<ContainerSummary>>) that
runs Command::new("docker").args(args).output(), checks status, converts stdout
to string, and parses lines into ContainerSummary (fields
id,name,image,status,ports) using the same splitn(5, '\t') logic; then have
list() call list_with_args(&["ps","--format", "..."]) and list_all() call
list_with_args(&["ps","-a","--format","..."]) so the format string and parsing
live in one place.
In `@src/docker/logs.rs`:
- Around line 11-32: The fetch function currently folds stderr into returned log
lines and ignores exit status, so errors like "No such container" are returned
as OK log entries; modify fetch to check output.status.success() after
Command::output(), and if not successful return an anyhow error (using
Context/anyhow::anyhow or bail!) that includes the stderr (and optionally
stdout) text to aid debugging; also ensure stdout and stderr are concatenated
with a separator (e.g., "\n") so a non-newline-terminated stdout doesn't merge
into stderr. Apply the same exact changes to fetch_all (validate
output.status.success(), return an Err with stderr context, and use a separator
when combining stdout/stderr).
In `@src/docker/run.rs`:
- Around line 63-74: The RunResult.container_id is being set unconditionally
from stdout which is only valid for detached runs; update the code that builds
RunResult (the block referencing output, stdout, stderr and constructing
RunResult) to only populate container_id when the run request was detached (use
request.detached) — otherwise leave container_id empty/None (or an explicit
empty string) so downstream stop/restart logic won't receive a bogus id;
alternatively you can add a secondary check that stdout plausibly matches a
container id (hex length) before assigning, but the primary fix is to guard
assignment with request.detached.
In `@src/doctor/environment_check.rs`:
- Around line 225-255: The registry_connectivity_check function currently
returns ok true whenever docker is present because the condition uses ||
!registry_url.is_empty(); fix it by performing a real probe (or by changing
messaging). Replace the docker info probe in registry_connectivity_check with a
concrete manifest inspect call: run Command::new("docker").args(["manifest",
"inspect", "<registry_url>/<known_image>"]).output(), treat a successful exit as
ok=true and include the manifest output in detail, and on non-success set
ok=false with a suggestion to run docker login (or include the error output);
ensure DoctorCheck fields (name, ok, detail, suggestion) are populated
accordingly and handle Command errors (e.g., Docker not installed) separately so
the previous dead else branch is reachable and meaningful.
- Around line 34-52: DoctorReport::export_json currently hand-rolls JSON and
interpolates unescaped fields (check.name, check.detail, check.suggestion) which
is unsafe; fix by adding serde/serde_json to Cargo.toml, derive serde::Serialize
on DoctorCheck and DoctorReport, and replace export_json implementation to
serialize the report via serde_json::to_string_pretty (or to_string) so all
fields are correctly escaped. Also revisit the registry_connectivity_check:
either change the code to perform the intended probe (use `docker manifest
inspect <registry_url>`/equivalent) to determine connectivity, or update the
comment and logic to reflect that it calls `docker info` and that ok should not
be driven solely by !registry_url.is_empty() (adjust the ok condition to reflect
actual connectivity result from the command). Ensure you reference
DoctorReport::export_json, DoctorCheck/DoctorReport derive, and
registry_connectivity_check when editing.
In `@src/doctor/registry_check.rs`:
- Around line 29-44: The check_registry and registry_connectivity_check logic
currently treats a successful `docker info` as proof the target registry is
reachable; change both to perform a registry-scoped probe: in
check_registry(&str) call an unauthenticated HTTP HEAD (or GET if HEAD
unsupported) to https://{registry}/v2/ and treat 200 or 401 as Connected,
404/5xx or network errors as Disconnected, and empty registry string as Unknown;
keep docker info as a fallback/diagnostic but add a timeout to the
Command::new("docker").arg("info") invocation (use the Command timeout helper or
spawn+wait with a deadline) and surface its result separately in
registry_connectivity_check so the UI can display Docker daemon status and the
actual registry probe status independently (refer to functions check_registry
and registry_connectivity_check).
In `@src/main.rs`:
- Around line 137-143: Replace the abrupt std::process::exit(1) with returning
an error so destructors and tracing flush run; when plan.ready() is false,
construct an anyhow error (e.g., via anyhow::bail! or Err(anyhow::anyhow!(...)))
that includes the "Deployment plan has blockers:" message plus the contents of
plan.blockers, and propagate that out of main (which already returns
anyhow::Result<()>), rather than calling std::process::exit.
- Line 155: The function chrono_timestamp currently returns a string like
"epoch:<secs>" but is documented and used as an ISO-8601 timestamp (see
DeploymentRecord.timestamp and deploy_history.yaml); update chrono_timestamp to
produce a proper ISO-8601 timestamp (e.g., use the time crate with time = {
version = "0.3", features = ["formatting"] } and format with an ISO-8601
formatter) and ensure the produced string replaces "epoch:<secs>" format; after
changing chrono_timestamp, update its docs/comments and the tests/sample data in
src/deploy/history.rs (and any saved deploy_history.yaml samples) to expect
ISO-8601 values so everything is consistent.
In `@src/ui/dashboard.rs`:
- Around line 485-495: The current computation in render_first_launch uses
Ord::clamp on u16 and a u16 multiplication that can panic or overflow; fix by
widening arithmetic to a larger integer (e.g., cast area.width/area.height to
u32), compute scaled_width = (area.width as u32 * 65 / 100) using the widened
type, then clamp safely with min/max (e.g., let width_u32 =
scaled_width.max(60).min(area.width as u32)), convert back to u16 with safe
saturation (as u16 since you already bounded it), and for height compute
similarly without clamp panic (e.g., let height_u32 = 25u32.min(area.height as
u32).max(20) or use area.height as u32 .max(20).min(25)); update the Rect
construction (x, y, width, height) to use the new u16 values so no clamp panics
or u16 overflow can occur.
In `@tests/deployment_pipeline.rs`:
- Around line 159-227: Collapse the repeated DeploymentRecord literals by adding
a small helper function (e.g., fn record(ts: &str, env: &str, success: bool) ->
DeploymentRecord) and use it inside the tests
deployment_history_records_and_truncates, deployment_history_last_success, and
deployment_history_yaml_round_trip to construct records instead of repeating the
eight-field literal; update calls to history.record(...) to pass record(...)
results so DeploymentHistory and DeploymentRecord usage stays unchanged while
removing duplication.
In `@tests/doctor_enhanced.rs`:
- Around line 27-32: Replace fragile substring assertions by deserializing the
String returned from report.export_json() into serde_json::Value and asserting
on concrete fields (e.g., that value["checks"][i]["name"] == "Docker CLI",
value["checks"][i]["ok"] == true/false, and value["checks"][i]["suggestion"] is
null or equals "Start Docker Desktop"); add serde_json as a dev-dependency in
Cargo.toml if missing, and update the test in tests/doctor_enhanced.rs to use
the parsed Value (referencing the export_json() call and the json variable) for
all checks instead of contains(...) substring matches.
---
Outside diff comments:
In `@src/main.rs`:
- Around line 68-178: Extract the large Doctor and Deploy match-arm bodies into
two helper functions to simplify main: implement fn run_doctor(full: bool, json:
bool) -> anyhow::Result<()> which moves the Doctor arm logic (loading Settings,
calling doctor::environment_check::run/_full, printing/exporting JSON or
render/summary and saving to config::paths::doctor_report_file()), and implement
fn run_deploy(project: PathBuf, environment: &str) -> anyhow::Result<()> which
moves the Deploy arm logic (calling startup::initialize, building plan via
deploy::pipeline::plan, validating plan.ready(), executing
deploy::pipeline::execute_pipeline, constructing and saving
deploy::history::DeploymentHistory using config::paths::deploy_history_file()
and chrono_timestamp(), and printing results). Replace the original match arms
(Some(Command::Doctor { .. }) and Some(Command::Deploy { .. })) with calls to
these helpers, propagating errors with ? and preserving all behavior (plan
blocker reporting, printing, history.record fields, and history.save).
In `@src/ui/dashboard.rs`:
- Around line 484-698: The function render_first_launch is too large and doing
multiple concerns (rect math, banner, subtitle, capability lines, and action
list); extract small helpers to reduce complexity: create welcome_rect(area:
Rect) -> Rect to compute the centered welcome_area and move the Clear/outer
block render into render_outer_block(frame, welcome_area, palette), extract the
ASCII art into render_ascii_banner(frame, chunk, palette) and the subtitle block
into render_subtitle(frame, chunk, palette), and replace the six near-identical
details.push(...) blocks with a capability_line(label: &str, present: bool,
palette: theme::Palette) -> Line helper used by render_capabilities_card(frame,
chunk, state, palette) to build the details vector; keep render_first_launch
calling these new functions in sequence so behavior is unchanged but complexity
is reduced.
🪄 Autofix (Beta)
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: cd9be347-2b8e-465b-b95d-2654c60c974f
📒 Files selected for processing (33)
src/compose/down.rssrc/compose/logs.rssrc/compose/mod.rssrc/compose/restart.rssrc/compose/services.rssrc/compose/up.rssrc/config/paths.rssrc/config/settings.rssrc/deploy/environments.rssrc/deploy/history.rssrc/deploy/mod.rssrc/deploy/pipeline.rssrc/deploy/rollback.rssrc/docker/build.rssrc/docker/containers.rssrc/docker/images.rssrc/docker/logs.rssrc/docker/networks.rssrc/docker/run.rssrc/docker/volumes.rssrc/doctor/docker_check.rssrc/doctor/environment_check.rssrc/doctor/kubernetes_check.rssrc/doctor/registry_check.rssrc/main.rssrc/services/executor.rssrc/storage/preferences.rssrc/ui/dashboard.rssrc/ui/state.rstests/compose_execution.rstests/deployment_pipeline.rstests/docker_execution.rstests/doctor_enhanced.rs
…vironment namespaces into deployment pipeline steps
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…er, compose, deploy, and ui modules
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…nary script generation in tests
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…hecks, and remove unused deployment code
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ndant test module
There was a problem hiding this comment.
Our agent can fix these. Install it.
Gates Passed
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
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
1 participant
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.
add deployment and enhancement for diagnostics
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Documentation