feat(cli): add --output json/yaml to sandbox get, status, and sandbox create#1989
feat(cli): add --output json/yaml to sandbox get, status, and sandbox create#1989rhuss wants to merge 1 commit into
Conversation
|
All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO document and I hereby sign the DCO. |
|
recheck |
| println!(" {} {}", "HTTP:".dimmed(), hs); | ||
| } | ||
| if let Some(ref e) = error { | ||
| println!(" {} {}", "gRPC error:".dimmed(), e); |
There was a problem hiding this comment.
This isn't byte-for-byte identical I don't think. If http_health_check() returns None then we used to show Error: ... but now it should hit this arm and will show gRPC error:
There was a problem hiding this comment.
Good catch. When http_health_check() returns None (no HTTP reachability at all), the original code used "Error:" as the label, not "gRPC error:". My refactoring incorrectly used "gRPC error:" for all cases in the "error" arm.
Fixed: the "error" arm now checks whether HTTP status is present. If yes, it shows both "HTTP:" and "gRPC error:" (matching the original dual-reachability case). If no HTTP status, it shows "Error:" (matching the original no-reachability case).
Applied in 9aa0ec4.
There was a problem hiding this comment.
(I'm still on it, not sure if this is the best solution)
| let policy_json = config.policy.as_ref().map_or_else( | ||
| || serde_json::Value::Null, | ||
| |p| openshell_policy::sandbox_policy_to_json_value(p).unwrap_or(serde_json::Value::Null), | ||
| ); |
There was a problem hiding this comment.
Is there a reason this does not propagate the conversion error? Other parts do, do we really want to return a null policy if conversion fails?
There was a problem hiding this comment.
You're right. Changed sandbox_detail_to_json to return Result<serde_json::Value> and propagate the conversion error via wrap_err. This matches how sandbox_settings_get (line 7213) handles sandbox_policy_to_json_value errors.
Applied in 9aa0ec4.
1816278 to
9aa0ec4
Compare
|
@johntmyers thanks for the review! As I'm on my way to dive into the codebase I will try to understand the codebase a bit better (not a Rust coder yet). But I think your comment ironically also hint to a problmen that this PR tries to partially solve: We need to be careful currently around the wording of log/error messages as they are parsed and used for result evaluation (i know at least one tool that wraps that parses the output of Therefor having the option for a structured output make much sense for these use cases. I wonder though, whether one should keep stderr free for informal messages or whether not (if |
|
I'll create a follow-up issue for emitting structured JSON errors when |
9aa0ec4 to
b88a214
Compare
… create Add structured output support to three commands that previously only supported human-readable table output: - `sandbox get --output json/yaml`: emits sandbox detail including policy source, revision, and active policy content via a new `sandbox_detail_to_json` converter. - `status --output json/yaml`: emits gateway connection state via a new `status_to_json` converter with conditional fields (auth, version, error, http_status) matching the human output. - `sandbox create --output json/yaml`: emits sandbox metadata after the sandbox reaches Ready phase, suppressing spinners and ANSI chrome from stdout. Uploads and port forwarding still execute before output. Clap rejects `--output` combined with `--editor` or trailing commands. All three commands reuse the existing `OutputFormat` enum and `print_output_single` helper. Default output (no --output flag) is byte-identical to current behavior. Unit tests cover both converters. Closes NVIDIA#1964 Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
b88a214 to
f067c05
Compare
Summary
Add
--output json/yamltosandbox get,status, andsandbox createso automation and MCP tool wrappers can consume structured data instead of parsing ANSI-colored human output.Related Issue
Closes #1964
Changes
sandbox get --output json/yaml: Newsandbox_detail_to_jsonconverter enriches the existingsandbox_to_jsonwith policy source, revision, and active policy content fromGetSandboxConfigResponse. Usessandbox_policy_to_json_valuefor direct proto-to-JSON conversion.status --output json/yaml: Newstatus_to_jsonconverter emits gateway, server, status, and conditional fields (auth, version, error, http_status). Refactoredgateway_statusto build data before output routing.sandbox create --output json/yaml: Emits sandbox metadata after Ready phase. Human chrome (header, spinner) suppressed from stdout; progress message on stderr. Uploads and port forwarding still execute before output. Clap rejects--outputcombined with--editoror trailing commands.status --output jsonwith no configured gateway emits{"status": "not_configured"}instead of human text.OutputFormatenum andprint_output_singlehelper.--outputflag) is byte-identical to current behavior (verified by diffing raw ANSI output).sandbox get,sandbox create, andstatusinmanage-sandboxes.mdxandmanage-gateways.mdx.Testing
cargo clippyclean,cargo fmtcleancargo test --lib)sandbox_detail_to_jsonandstatus_to_jsonconverterssandbox get --output json,status --output json/yaml, default output byte-identical regression checkChecklist