feat: switch default output format from text to json (12.14) #398#399
feat: switch default output format from text to json (12.14) #398#399josecelano merged 4 commits intomainfrom
text to json (12.14) #398#399Conversation
- Move #[default] from OutputFormat::Text to OutputFormat::Json - Update default_value in CLI args from 'text' to 'json' - Update doctest and doc comments to reflect new default - Update 5 E2E test assertions to check for JSON fields instead of text strings - All linters pass, 431 tests pass
There was a problem hiding this comment.
Pull request overview
This PR switches the Torrust Tracker Deployer CLI’s default output format from human-readable text to machine-readable json, completing the JSON-output epic and enabling automation/AI-agent friendly defaults across commands.
Changes:
- Made
OutputFormat::Jsonthe enum default (and updated the doctest/docs accordingly). - Updated the global CLI arg
--output-formatdefault value from"text"to"json"and refreshed help text. - Updated E2E tests for
renderandpurgeto assert JSON-shaped output rather than text success messages.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/presentation/cli/input/cli/output_format.rs |
Moves #[default] to Json and updates the doctest/doc comments to reflect JSON default. |
src/presentation/cli/input/cli/args.rs |
Changes clap default to "json" and updates the CLI help text to match the new default behavior. |
tests/e2e/purge_command.rs |
Updates assertions to look for JSON success fields instead of text success strings. |
tests/e2e/render_command.rs |
Updates assertions to look for JSON fields (e.g., output_dir) instead of text success strings. |
tests/e2e/render_command.rs
Outdated
| let output = format!("{}{}", render_result.stdout(), render_result.stderr()); | ||
| assert!( | ||
| output.contains("generated successfully"), | ||
| "Output should contain success message. Combined output: {output}" | ||
| output.contains("\"output_dir\""), | ||
| "Output should contain JSON output_dir field. Combined output: {output}" | ||
| ); |
There was a problem hiding this comment.
These E2E assertions look at stdout + stderr, which means the test would still pass even if the JSON result accidentally gets printed to stderr. The CLI output contract routes progress/success to stderr and result data (including JSON) to stdout (see UserOutput::result()/data() and ProgressReporter::result()). Consider asserting on render_result.stdout() for the JSON field (and optionally that stderr does not contain JSON) so the test enforces the stream separation.
tests/e2e/render_command.rs
Outdated
| let output = format!("{}{}", render_result.stdout(), render_result.stderr()); | ||
| assert!( | ||
| output.contains("generated successfully"), | ||
| "Output should contain success message. Combined output: {output}" | ||
| output.contains("\"output_dir\""), | ||
| "Output should contain JSON output_dir field. Combined output: {output}" | ||
| ); |
There was a problem hiding this comment.
These E2E assertions look at stdout + stderr, which means the test would still pass even if the JSON result accidentally gets printed to stderr. The CLI output contract routes progress/success to stderr and result data (including JSON) to stdout (see UserOutput::result()/data() and ProgressReporter::result()). Consider asserting on render_result.stdout() for the JSON field (and optionally that stderr does not contain JSON) so the test enforces the stream separation.
tests/e2e/purge_command.rs
Outdated
| let output = format!("{}{}", purge_result.stdout(), purge_result.stderr()); | ||
| assert!( | ||
| output.contains("purged successfully"), | ||
| "Output should contain success message. Combined output: {output}" | ||
| output.contains("\"purged\": true"), | ||
| "Output should contain JSON success field. Combined output: {output}" | ||
| ); |
There was a problem hiding this comment.
These E2E assertions look at stdout + stderr, which means the test would still pass even if the JSON result accidentally gets printed to stderr. The CLI output contract routes progress/success to stderr and result data (including JSON) to stdout (see UserOutput::result()/data() and ProgressReporter::result()). Consider asserting on purge_result.stdout() for the JSON field (and optionally that stderr does not contain JSON) so the test enforces the stream separation.
tests/e2e/purge_command.rs
Outdated
| let output = format!("{}{}", purge_result.stdout(), purge_result.stderr()); | ||
| assert!( | ||
| output.contains("purged successfully"), | ||
| "Output should contain success message. Combined output: {output}" | ||
| output.contains("\"purged\": true"), | ||
| "Output should contain JSON success field. Combined output: {output}" | ||
| ); |
There was a problem hiding this comment.
These E2E assertions look at stdout + stderr, which means the test would still pass even if the JSON result accidentally gets printed to stderr. The CLI output contract routes progress/success to stderr and result data (including JSON) to stdout (see UserOutput::result()/data() and ProgressReporter::result()). Consider asserting on purge_result.stdout() for the JSON field (and optionally that stderr does not contain JSON) so the test enforces the stream separation.
| /// Output format for command results (default: json) | ||
| /// | ||
| /// Controls the format of user-facing output (stdout channel). | ||
| /// - text: Human-readable formatted output with tables and sections (default) | ||
| /// - json: Machine-readable JSON for automation, scripts, and AI agents | ||
| /// - json: Machine-readable JSON for automation, scripts, and AI agents (default) | ||
| /// - text: Human-readable formatted output with tables and sections |
There was a problem hiding this comment.
The doc comment says output format “controls the format of user-facing output (stdout channel)”, but the CLI design sends progress/success/warnings to stderr and only the final command result/JSON to stdout. To avoid confusion for users and contributors, consider rewording this to explicitly say it controls the result data written to stdout, while progress/status messages remain on stderr regardless of output format.
tests/e2e/render_command.rs
Outdated
| let output = format!("{}{}", render_result.stdout(), render_result.stderr()); | ||
| assert!( | ||
| output.contains("generated successfully"), | ||
| "Output should contain success message. Combined output: {output}" | ||
| output.contains("\"output_dir\""), | ||
| "Output should contain JSON output_dir field. Combined output: {output}" | ||
| ); |
There was a problem hiding this comment.
These E2E assertions look at stdout + stderr, which means the test would still pass even if the JSON result accidentally gets printed to stderr. The CLI output contract routes progress/success to stderr and result data (including JSON) to stdout (see UserOutput::result()/data() and ProgressReporter::result()). Consider asserting on render_result.stdout() for the JSON field (and optionally that stderr does not contain JSON) so the test enforces the stream separation.
- render/purge E2E tests: assert on stdout only instead of stdout+stderr combined, enforcing the stdout/stderr separation contract - args.rs: clarify doc comment — output-format controls result data on stdout; progress/warnings always go to stderr regardless of this flag
…alid JSON
Replace unsafe format!() string interpolation of error messages with
serde_json::json! construction in all JsonView::render() error fallbacks.
The previous format!(r#"{{ "message": "{e}" }}"#) approach would
produce invalid JSON if the error message contained quotes, backslashes,
or newlines. Using serde_json::json! ensures the message is always
properly escaped.
Applies to 11 json_view.rs files:
- configure, release, render, destroy, purge, test, register, validate,
list, run, show
Reported by Copilot review on PR #397.
|
ACK 2319b28 |
- Mark 12.14 (switch default output format to JSON) as completed PR #399 merged to main - Mark section 12 (Add JSON output format support) as fully completed - Remove 9 closed issue documentation files: - #371 add-json-output-to-configure-command - #377 add-json-output-to-release-command - #380 add-json-output-to-test-command - #386 add-json-output-to-destroy-command - #390 add-json-output-to-validate-command - #392 add-json-output-to-render-command - #394 add-json-output-to-purge-command - #396 add-json-output-to-register-command - #398 switch-default-output-format-to-json
Description
Switches the default CLI output format from
texttojson, closing #398 and completing epic #348.All commands now support JSON output (12.1–12.13 complete), so this prerequisite is satisfied.
Changes
#[default]fromTexttoJson; updated doctest and doc commentsdefault_value = "text"→"json", updated doc commenttests/e2e/purge_command.rs— updated 2 test assertions to check"purged": true(JSON) instead of"purged successfully"(text)tests/e2e/render_command.rs— updated 3 test assertions to check"output_dir"(JSON) instead of"generated successfully"(text)Behavior After This Change
Running any command without
--output-formatnow produces JSON:$ torrust-tracker-deployer list { "environments": [] }Human-readable text output requires an explicit flag:
Breaking Change
Users relying on unformatted text output without
--output-formatwill now receive JSON. Add--output-format textto restore the previous behavior.Testing
cargo test)cargo run --bin linter all)Related
texttojson(12.14) #398