Skip to content

feat: Add destroy subcommand to CLI with UserOutput system#28

Merged
josecelano merged 14 commits into
mainfrom
copilot/add-clap-subcommand-configuration
Oct 24, 2025
Merged

feat: Add destroy subcommand to CLI with UserOutput system#28
josecelano merged 14 commits into
mainfrom
copilot/add-clap-subcommand-configuration

Conversation

Copilot AI commented Oct 22, 2025

Copy link
Copy Markdown
Contributor

Overview

This PR implements the destroy subcommand in the CLI with a new UserOutput system for user-facing messages, completing the UI layer for the destroy command workflow.

Changes

UserOutput System (src/shared/user_output.rs)

Implemented a dual-channel output system following Unix conventions and modern CLI best practices (cargo, docker, npm):

  • VerbosityLevel enum: Five levels (Quiet, Normal, Verbose, VeryVerbose, Debug) with proper ordering
  • UserOutput struct: Dual writers separating concerns:
    • stderr: Progress updates, status messages, warnings, errors
    • stdout: Final results and structured data for piping/redirection
  • Benefits:
    • Clean piping: torrust-tracker-deployer destroy env 2>/dev/null captures only results
    • Automation friendly: Scripts can redirect channels independently
    • Follows established CLI tool patterns

CLI Integration (src/app.rs)

Added destroy subcommand with complete integration:

  • Commands enum: New Destroy variant accepting environment parameter
  • Command handler: handle_destroy_command() function that:
    • Validates environment names with clear error messages
    • Loads environments from persistent storage
    • Executes DestroyCommandHandler from application layer
    • Provides user-friendly progress updates using UserOutput
    • Handles all error cases with actionable guidance

Example Usage

# Get help
torrust-tracker-deployer destroy --help

# Destroy an environment
torrust-tracker-deployer destroy e2e-provision
⏳ Destroying environment 'e2e-provision'...
⏳ Tearing down infrastructure...
⏳ Cleaning up resources...
✅ Environment 'e2e-provision' destroyed successfully

# Error handling
torrust-tracker-deployer destroy "invalid name!"
❌ Invalid environment name 'invalid name!': contains invalid characters: !

Testing

  • Unit tests: 23 new tests (14 UserOutput + 9 CLI parsing)
  • All tests passing: 852 total tests (843 library + 9 binary)
  • Channel separation: Tests verify stdout/stderr usage with buffer writers
  • Verbosity behavior: Tests verify message filtering at different levels
  • CLI parsing: Tests cover argument validation, defaults, and error cases

Code Quality

  • ✅ All linters passing (markdown, yaml, toml, cspell, clippy, rustfmt, shellcheck)
  • ✅ Code formatted with cargo fmt
  • ✅ All clippy warnings addressed
  • ✅ Comprehensive documentation with examples

Dependencies

⚠️ Manual E2E testing blocked by #21 (Fix E2E Infrastructure Preservation)

Full end-to-end testing with actual infrastructure requires the --keep flag from Issue #21 to preserve E2E test environments. The CLI is functionally complete and ready for use once that dependency is resolved.

Related Issues

Documentation

See docs/issues/23-add-clap-subcommand-configuration.md for full specification and docs/research/UX/console-app-output-patterns.md for channel strategy research.

Original prompt

This section details on the original issue you should resolve

<issue_title>Add Clap Subcommand Configuration</issue_title>
<issue_description># Add Clap Subcommand Configuration

Type: Task
Priority: High
Parent Epic: #10 - UI Layer Destroy Command
Dependencies: #21 - MUST BE COMPLETED FIRST (E2E Infrastructure Preservation)
Estimated Effort: 3-4 hours


📋 Issue Overview

Implement the destroy subcommand in the CLI with basic functionality and UserOutput scaffolding. This creates the user-facing interface that calls the DestroyCommandHandler from the application layer.

🔗 Dependencies

Blocking Dependency: Issue #21 (Fix E2E Infrastructure Preservation)

This issue CANNOT be implemented until Issue #21 is completed because:

  1. Manual Testing Required: The destroy CLI command must be manually tested to ensure it works correctly
  2. E2E Infrastructure Needed: Manual testing requires preserving E2E infrastructure using --keep flag
  3. Verification Workflow: Developers need to:
    • Run E2E tests with --keep to provision infrastructure
    • Test the destroy CLI command on that preserved infrastructure
    • Verify complete cleanup using LXD commands (lxc list | grep e2e-provision)

Without Issue #21 completed, the manual testing workflow is broken and this destroy CLI implementation cannot be properly validated.

🎯 Goals

  • Add destroy subcommand to Clap configuration
  • Wire up subcommand to call DestroyCommandHandler from application layer
  • Implement VerbosityLevel enum and UserOutput struct
  • Add essential progress messages using UserOutput
  • Ensure separation between user output and internal logging

📦 Scope

Clap Subcommand Structure

Add to src/app.rs:

#[derive(Subcommand)]
pub enum Commands {
    /// Destroy an existing deployment environment
    Destroy {
        /// Name of the environment to destroy
        environment: String,
    },
}

UserOutput Integration

  • Add VerbosityLevel enum (Quiet, Normal, Verbose, VeryVerbose, Debug)
  • Add UserOutput struct with methods for different message types
  • Note: Only enum definition for now, no Clap verbosity flags yet
  • Use UserOutput for essential destroy command messages

Example Usage

torrust-tracker-deployer destroy <ENVIRONMENT_NAME>

Example Output (Normal verbosity level)

⏳ Destroying environment 'my-env'...
⏳ Tearing down infrastructure...
⏳ Cleaning up resources...
✅ Environment 'my-env' destroyed successfully

✅ Acceptance Criteria

Prerequisites: Issue #21 (Fix E2E Infrastructure Preservation) must be completed before this can be fully tested and accepted.

  • destroy subcommand added to Clap configuration
  • Subcommand accepts required environment parameter
  • VerbosityLevel enum implemented with 5 levels (Quiet, Normal, Verbose, VeryVerbose, Debug)
  • UserOutput struct implemented with dual writers and proper channel separation
  • Channel strategy follows console output patterns research (progress to stderr, results to stdout)
  • Essential destroy messages implemented using appropriate channels
  • Subcommand successfully calls DestroyCommandHandler from application layer
  • User-friendly error messages for invalid environment names
  • Help text provides clear usage information
  • Unit tests implemented with buffer writers for output capture and assertion
  • Unit tests verify proper channel separation (stdout vs stderr)
  • Unit tests cover CLI argument parsing and error handling
  • Manual testing completed using E2E test infrastructure (--keep flag)
  • Manual testing verifies complete infrastructure, data, and build cleanup
  • User output completely separated from internal logging
  • All linting passes successfully
  • Integration works end-to-end with existing command handler

Manual Testing Procedure

⚠️ Prerequisite: Issue #21 (Fix E2E Infrastructure Preservation) must be completed first. The --keep flag functionality is required for this testing procedure.

Step 1: Set up test infrastructure using E2E tests

# Run E2E provision and destroy test but keep infrastructure (don't auto-destroy)
cargo run --bin e2e_provision_and_destroy_tests -- --keep

# This creates an environment named "e2e-provision" with infrastructure ready for testing

Step 2: Test the new destroy CLI command

# Test help output
cargo run -- help destroy
cargo run -- destroy --help

# Test basic destroy command with the E2E environment
cargo run -- destroy e2e-provision

# Test invalid environment names
cargo run -- destroy invalid-name!
cargo run -- destroy ""

S...

Fixes #23


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 3 commits October 22, 2025 17:09
Implement user-facing output functionality separate from internal logging with
dual-channel strategy following Unix conventions:
- stdout: final results and data for piping/redirection
- stderr: progress updates, status messages, warnings, errors

Add comprehensive unit tests with buffer writers for output capture and assertion.

Co-authored-by: josecelano <58816+josecelano@users.noreply.github.com>
Add Commands enum with Destroy variant accepting environment parameter.
Wire up destroy subcommand to call DestroyCommandHandler from application layer.
Implement comprehensive CLI argument parsing tests.
Integrate UserOutput for user-friendly progress messages and error handling.

Co-authored-by: josecelano <58816+josecelano@users.noreply.github.com>
Fix needless_pass_by_value clippy warning by using &str instead of String.
Add allow attribute for type_complexity in test helper.
Run cargo fmt to ensure consistent code formatting.

Co-authored-by: josecelano <58816+josecelano@users.noreply.github.com>
Copilot AI changed the title [WIP] Add Clap subcommand configuration for destroy CLI feat: Add destroy subcommand to CLI with UserOutput system Oct 22, 2025
Copilot AI requested a review from josecelano October 22, 2025 17:32
@josecelano

Copy link
Copy Markdown
Member

Code Review Update

I've reviewed the generated code and identified several architectural improvements needed. Rather than providing detailed feedback through PR comments, I'll handle the refactoring directly as it's more efficient than iterating through review cycles.

The main areas I'll be addressing:

  1. Architecture improvements - Some circular dependencies in the command handler that need restructuring
  2. Error handling patterns - Ensuring consistent error handling throughout the CLI layer
  3. Code organization - Better separation of concerns between CLI and application layers
  4. Testing coverage - Additional edge case coverage for robust CLI behavior

I'll push the improvements directly to maintain momentum on this PR. The core functionality is solid - these are primarily architectural refinements to align with the project's patterns and principles.

Replace manual impl Default with derive attribute and #[default] annotation
as suggested by clippy for better maintainability.
Improve separation of concerns by moving environment loading and state
handling logic from presentation layer (CLI) to application layer:

- DestroyCommandHandler now loads environment internally from EnvironmentName
- CLI simplified to focus only on user interface concerns
- Proper build directory path resolution using env.tofu_build_dir()
- Eliminates circular dependency between CLI and command handler
- Better testability and maintainability of business logic

Breaking changes:
- DestroyCommandHandler.execute() now takes EnvironmentName instead of Environment<S>
- Constructor no longer requires OpenTofuClient (created internally)

This follows clean architecture principles with proper layer separation.
…ntState

- Add destroy() method to AnyEnvironmentState for unified state destruction
- Add tofu_build_dir() method to AnyEnvironmentState for unified directory access
- Update DestroyCommandHandler to use new methods with Result error handling
- Add StateTypeError integration with Clone derive for error propagation
- Remove 24 lines of repetitive match patterns across two locations
- Add comprehensive test coverage for new methods and error cases
…ently

- Remove Result wrapper from tofu_build_dir method - now returns PathBuf directly
- Include Destroyed state in pattern matching like all other states
- Simplify caller code in DestroyCommandHandler (no more error handling needed)
- Update tests to reflect the simplified interface
- Improve consistency: getter methods should return same result regardless of state
- Let caller decide how to use the path based on their specific needs
…tring

- Add LXD_PROVIDER_NAME constant to domain::environment module
- Replace hardcoded 'lxd' string in tofu_build_dir() with constant reference
- Improves maintainability and follows existing pattern with other constants
- All 847 tests pass with comprehensive E2E validation
…architecture

- Create new src/presentation/ layer for presentation concerns
- Move user_output.rs from shared/ to presentation/ layer
- Update all import paths from shared::user_output to presentation::user_output
- Add proper presentation layer module documentation
- Update lib.rs to include presentation module in DDD architecture
- Update documentation references to reflect new file location
- Maintains proper DDD separation: presentation handles user-facing output

The user_output module represents presentation layer logic in DDD and belongs
with other presentation concerns rather than shared utilities.
- Replace Box<dyn Error> with specific DestroyCommandError enum using thiserror
- Add comprehensive error variants for different failure scenarios
- Implement tiered help system with brief tips + detailed .help() method
- Include actionable troubleshooting guidance following error handling conventions
- Preserve error chains with #[source] attributes for full traceability
- Provide clear, user-friendly error messages with specific instructions

Features:
- InvalidEnvironmentName: Validates environment name format with examples
- DestroyOperationFailed: Handles destroy command execution failures
- Environment context: Includes environment name and data directory paths
- Detailed help: Platform-specific commands and recovery procedures

Error handling follows docs/contributing/error-handling.md conventions:
- Clarity: Unambiguous error messages with specific context
- Traceability: Full error chains preserved for debugging
- Actionability: Clear instructions for resolution
… error handling

- Separate CLI parsing from business logic in presentation/cli/
- Create modular command execution system in presentation/commands/
- Implement unified CommandError with tiered help system
- Add LoggingConfig abstraction decoupled from CLI parsing
- Reduce app.rs to thin bootstrap (452 → 38 lines)
- Create centralized help system in src/help.rs
- Follow module organization guidelines with proper separation of concerns

This completes the DDD architectural refactoring for clean separation
between presentation, application, and domain layers.
…ates

- Enhanced codebase-architecture.md with presentation layer documentation
- Added architecture requirements to issue templates
- Renamed TEMPLATE.md to SPECIFICATION-TEMPLATE.md for clarity
- Renamed TASK-TEMPLATE.md to GITHUB-ISSUE-TEMPLATE.md for clarity
- Updated template references in roadmap-issues.md

These improvements address architectural gaps that caused AI assistants
to implement CLI functionality in wrong DDD layers.
@josecelano josecelano marked this pull request as ready for review October 24, 2025 15:41
Fixes CI linting failure by adding 'reprovisioning' to project-words.txt.
This word appears in docs/features/hybrid-command-architecture/README.md
from main branch when PR is merged for CI testing.
@josecelano

Copy link
Copy Markdown
Member

ACK 87ac5f6

@josecelano josecelano merged commit aa27f83 into main Oct 24, 2025
27 checks passed
@josecelano josecelano deleted the copilot/add-clap-subcommand-configuration branch April 15, 2026 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Clap Subcommand Configuration

2 participants