Skip to content

Commit 704be06

Browse files
committed
Merge #57: refactor: migrate command handlers to modular directory structure
74cffc0 refactor: [#56] migrate DestroyCommandHandler to modular structure (copilot-swe-agent[bot]) ece9119 refactor: [#56] migrate ConfigureCommandHandler to modular structure (copilot-swe-agent[bot]) 845df78 refactor: [#56] migrate ProvisionCommandHandler to modular structure (copilot-swe-agent[bot]) 309a5d4 Initial plan (copilot-swe-agent[bot]) Pull request description: Refactors three single-file command handlers (`provision`, `configure`, `destroy`) into modular directory structures matching the existing `create` handler pattern. Reduces file sizes and improves maintainability through separation of concerns. ## Changes **File splits:** - `provision.rs` (752 lines) → `provision/{mod.rs, handler.rs, errors.rs, tests/}` - `configure.rs` (370 lines) → `configure/{mod.rs, handler.rs, errors.rs, tests/}` - `destroy.rs` (631 lines) → `destroy/{mod.rs, handler.rs, tests/}` **Structure per handler:** ``` {command}/ ├── mod.rs # Module documentation & public API re-exports ├── handler.rs # Command handler implementation ├── errors.rs # Error types with Traceable trait └── tests/ ├── mod.rs # Test module entry ├── builders.rs # Test builders and fixtures └── integration.rs # Integration tests ``` **Visibility changes:** - Made struct fields `pub(crate)` for test access - Made helper methods `pub(crate)` where tests require them (e.g., `build_failure_context`, `should_destroy_infrastructure`, `cleanup_state_files`) All business logic preserved unchanged. Import paths updated throughout codebase. ## Example Before: ```rust // Single 752-line file mixing concerns src/application/command_handlers/provision.rs ``` After: ```rust // Focused modules src/application/command_handlers/provision/ ├── handler.rs // ~370 lines - business logic only ├── errors.rs // ~80 lines - error types └── tests/ // ~280 lines - organized test infrastructure ``` <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> ---- *This section details on the original issue you should resolve* <issue_title>Refactor Command Handlers to Modular Directory Structure</issue_title> <issue_description>## Overview Refactor the existing single-file command handlers (`provision`, `configure`, `destroy`) to follow the new modular directory structure pattern established in `CreateCommandHandler`. This improves code organization, maintainability, and scalability by separating concerns into dedicated files. Currently, three command handlers use a single-file pattern: - `provision.rs` (752 lines) - Too large, mixing handler logic, errors, and tests - `destroy.rs` (631 lines) - Getting unwieldy - `configure.rs` (370 lines) - Manageable but would benefit from separation The `CreateCommandHandler` demonstrates a better structure with separate files for handler logic, errors, and tests. ## Specification See detailed specification: [docs/issues/refactor-command-handlers-to-modular-structure.md](https://github.com/torrust/torrust-tracker-deployer/blob/main/docs/issues/refactor-command-handlers-to-modular-structure.md) (Link will be updated after file rename with issue number) ## 🏗️ Architecture Requirements **DDD Layer**: Application Layer **Module Path**: `src/application/command_handlers/{provision|configure|destroy}/` **Pattern**: Command Handler with modular organization ### Target Structure Each command handler will follow this pattern: ```text {command}/ ├── mod.rs # Module documentation & public API ├── handler.rs # Command handler implementation ├── errors.rs # Error types with .help() └── tests/ ├── mod.rs # Test module entry ├── builders.rs # Test builders and fixtures └── integration.rs # Integration tests ``` ### Architectural Constraints - [ ] No changes to command handler business logic (refactoring only) - [ ] Error handling follows project conventions - [ ] All existing tests must continue to pass - [ ] Breaking changes to import paths are acceptable (library not yet publicly used) ## Implementation Plan **Important**: Migrate and commit each command handler independently. Run pre-commit checks after completing each one before moving to the next. ### Subtask 1: Migrate ProvisionCommandHandler (3-4 hours) - [ ] Create `provision/` directory with modular structure - [ ] Split `provision.rs` into `handler.rs`, `errors.rs`, `tests/` - [ ] Update all imports across codebase - [ ] Verify tests pass and run pre-commit checks - [ ] **Commit independently before moving to next handler** ### Subtask 2: Migrate ConfigureCommandHandler (2-3 hours) - [ ] Create `configure/` directory with modular structure - [ ] Split `configure.rs` into `handler.rs`, `errors.rs`, `tests/` - [ ] Update all imports across codebase - [ ] Verify tests pass and run pre-commit checks - [ ] **Commit independently before moving to next handler** ### Subtask 3: Migrate DestroyCommandHandler (2-3 hours) - [ ] Create `destroy/` directory with modular structure - [ ] Split `destroy.rs` into `handler.rs`, `errors.rs`, `tests/` - [ ] Update all imports across codebase - [ ] Verify tests pass and run pre-commit checks - [ ] **Commit independently** ### Subtask 4: Final Verification (1 hour) - [ ] Run full test suite - [ ] Run E2E tests - [ ] Verify consistent structure across all handlers - [ ] Update any relevant documentation - [ ] Final pre-commit verification ## Acceptance Criteria **Quality Checks**: - [ ] Pre-commit checks pass: `./scripts/pre-commit.sh` **Refactoring-Specific Criteria**: - [ ] All three command handlers use modular directory structure - [ ] Directory structure matches `create/` pattern exactly - [ ] All existing tests continue to pass - [ ] No changes to business logic (pure refactoring) - [ ] File sizes reduced to manageable levels (< 300 lines per file) - [ ] Clear separation of concerns (handler, errors, tests) - [ ] All imports updated correctly throughout entire codebase **Commit Strategy**: - [ ] Each command handler refactoring committed independently - [ ] Pre-commit checks pass before each commit - [ ] Clear, descriptive commit messages following conventional commits format ## Related Documentation - [Module Organization Guide](https://github.com/torrust/torrust-tracker-deployer/blob/main/docs/contributing/module-organization.md) - [Error Handling Guide](https://github.com/torrust/torrust-tracker-deployer/blob/main/docs/contributing/error-handling.md) - [Testing Conventions](https://github.com/torrust/torrust-tracker-deployer/blob/main/docs/contributing/testing.md) - Reference Implementation: `src/application/command_handlers/create/` ## Benefits - **Maintainability**: Smaller, focused files (200-300 lines vs 752 lines) - **Scalability**: Easy to add new features without bloating files - **Consistency**: All command handlers follow same structure - **Testing**: Clear test organization and better discoverability </issue_description> ## Comments on the Issue (you ar... </details> - Fixes #56 <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. ACKs for top commit: josecelano: ACK 74cffc0 Tree-SHA512: 487b98cd00b8bfdd30e8d1cbe8fa8b32d79ddefb6098cd5f660797841f97a8b192f2292b7d9423db876affeafd4fe2f0865751fe181887f2dc6c375211bf8b53
2 parents 3ba61b7 + 74cffc0 commit 704be06

19 files changed

Lines changed: 1406 additions & 1169 deletions

File tree

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
//! Error types for the Configure command handler
2+
3+
use crate::shared::command::CommandError;
4+
5+
/// Comprehensive error type for the `ConfigureCommandHandler`
6+
#[derive(Debug, thiserror::Error)]
7+
pub enum ConfigureCommandHandlerError {
8+
#[error("Command execution failed: {0}")]
9+
Command(#[from] CommandError),
10+
11+
#[error("Failed to persist environment state: {0}")]
12+
StatePersistence(#[from] crate::domain::environment::repository::RepositoryError),
13+
}
14+
15+
impl crate::shared::Traceable for ConfigureCommandHandlerError {
16+
fn trace_format(&self) -> String {
17+
match self {
18+
Self::Command(e) => {
19+
format!("ConfigureCommandHandlerError: Command execution failed - {e}")
20+
}
21+
Self::StatePersistence(e) => {
22+
format!("ConfigureCommandHandlerError: Failed to persist environment state - {e}")
23+
}
24+
}
25+
}
26+
27+
fn trace_source(&self) -> Option<&dyn crate::shared::Traceable> {
28+
match self {
29+
Self::Command(e) => Some(e),
30+
Self::StatePersistence(_) => None,
31+
}
32+
}
33+
34+
fn error_kind(&self) -> crate::shared::ErrorKind {
35+
match self {
36+
Self::Command(_) => crate::shared::ErrorKind::CommandExecution,
37+
Self::StatePersistence(_) => crate::shared::ErrorKind::StatePersistence,
38+
}
39+
}
40+
}

src/application/command_handlers/configure.rs renamed to src/application/command_handlers/configure/handler.rs

Lines changed: 8 additions & 152 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
1+
//! Configure command handler implementation
2+
13
use std::sync::Arc;
24

3-
use tracing::{info, instrument, warn};
5+
use tracing::{info, instrument};
46

7+
use super::errors::ConfigureCommandHandlerError;
58
use crate::adapters::ansible::AnsibleClient;
69
use crate::application::steps::{InstallDockerComposeStep, InstallDockerStep};
710
use crate::domain::environment::repository::EnvironmentRepository;
@@ -10,46 +13,8 @@ use crate::domain::environment::state::{
1013
};
1114
use crate::domain::environment::{Configured, Configuring, Environment, Provisioned, TraceId};
1215
use crate::infrastructure::trace::ConfigureTraceWriter;
13-
use crate::shared::command::CommandError;
1416
use crate::shared::error::Traceable;
1517

16-
/// Comprehensive error type for the `ConfigureCommandHandler`
17-
#[derive(Debug, thiserror::Error)]
18-
pub enum ConfigureCommandHandlerError {
19-
#[error("Command execution failed: {0}")]
20-
Command(#[from] CommandError),
21-
22-
#[error("Failed to persist environment state: {0}")]
23-
StatePersistence(#[from] crate::domain::environment::repository::RepositoryError),
24-
}
25-
26-
impl crate::shared::Traceable for ConfigureCommandHandlerError {
27-
fn trace_format(&self) -> String {
28-
match self {
29-
Self::Command(e) => {
30-
format!("ConfigureCommandHandlerError: Command execution failed - {e}")
31-
}
32-
Self::StatePersistence(e) => {
33-
format!("ConfigureCommandHandlerError: Failed to persist environment state - {e}")
34-
}
35-
}
36-
}
37-
38-
fn trace_source(&self) -> Option<&dyn crate::shared::Traceable> {
39-
match self {
40-
Self::Command(e) => Some(e),
41-
Self::StatePersistence(_) => None,
42-
}
43-
}
44-
45-
fn error_kind(&self) -> crate::shared::ErrorKind {
46-
match self {
47-
Self::Command(_) => crate::shared::ErrorKind::CommandExecution,
48-
Self::StatePersistence(_) => crate::shared::ErrorKind::StatePersistence,
49-
}
50-
}
51-
}
52-
5318
/// `ConfigureCommandHandler` orchestrates the complete infrastructure configuration workflow
5419
///
5520
/// The `ConfigureCommandHandler` orchestrates the complete infrastructure configuration workflow.
@@ -69,9 +34,9 @@ impl crate::shared::Traceable for ConfigureCommandHandlerError {
6934
/// State is persisted after each transition using the injected repository.
7035
/// Persistence failures are logged but don't fail the command (state remains valid in memory).
7136
pub struct ConfigureCommandHandler {
72-
ansible_client: Arc<AnsibleClient>,
73-
clock: Arc<dyn crate::shared::Clock>,
74-
repository: Arc<dyn EnvironmentRepository>,
37+
pub(crate) ansible_client: Arc<AnsibleClient>,
38+
pub(crate) clock: Arc<dyn crate::shared::Clock>,
39+
pub(crate) repository: Arc<dyn EnvironmentRepository>,
7540
}
7641

7742
impl ConfigureCommandHandler {
@@ -194,7 +159,6 @@ impl ConfigureCommandHandler {
194159
Ok(configured)
195160
}
196161

197-
/// Persist configuring state
198162
/// Build failure context for a configuration error and generate trace file
199163
///
200164
/// This helper method builds structured error context including the failed step,
@@ -214,7 +178,7 @@ impl ConfigureCommandHandler {
214178
/// # Returns
215179
///
216180
/// A structured `ConfigureFailureContext` with timing, error details, and trace file path
217-
fn build_failure_context(
181+
pub(crate) fn build_failure_context(
218182
&self,
219183
environment: &Environment<Configuring>,
220184
error: &ConfigureCommandHandlerError,
@@ -260,111 +224,3 @@ impl ConfigureCommandHandler {
260224
context
261225
}
262226
}
263-
264-
#[cfg(test)]
265-
mod tests {
266-
use super::*;
267-
use tempfile::TempDir;
268-
269-
// Helper function to create a test environment in Configuring state
270-
fn create_test_environment(_temp_dir: &TempDir) -> (Environment<Configuring>, TempDir) {
271-
use crate::domain::environment::testing::EnvironmentTestBuilder;
272-
273-
let (env, _data_dir, _build_dir, env_temp_dir) = EnvironmentTestBuilder::new()
274-
.with_name("test-env")
275-
.build_with_custom_paths();
276-
277-
// Environment is created with paths inside env_temp_dir
278-
// which will be automatically cleaned up when env_temp_dir is dropped
279-
280-
// Transition Created -> Provisioning -> Provisioned -> Configuring
281-
(
282-
env.start_provisioning().provisioned().start_configuring(),
283-
env_temp_dir,
284-
)
285-
}
286-
287-
/// Test builder for `ConfigureCommandHandler` that manages dependencies and lifecycle
288-
///
289-
/// This builder simplifies test setup by:
290-
/// - Managing `TempDir` lifecycle
291-
/// - Providing sensible defaults for all dependencies
292-
/// - Returning only the command and necessary test artifacts
293-
pub struct ConfigureCommandHandlerTestBuilder {
294-
temp_dir: TempDir,
295-
}
296-
297-
impl ConfigureCommandHandlerTestBuilder {
298-
/// Create a new test builder with default configuration
299-
pub fn new() -> Self {
300-
let temp_dir = TempDir::new().expect("Failed to create temp dir");
301-
Self { temp_dir }
302-
}
303-
304-
/// Build the `ConfigureCommandHandler` with all dependencies
305-
///
306-
/// Returns: (`command`, `temp_dir`)
307-
/// The `temp_dir` must be kept alive for the duration of the test.
308-
pub fn build(self) -> (ConfigureCommandHandler, TempDir) {
309-
let ansible_client = Arc::new(AnsibleClient::new(self.temp_dir.path()));
310-
let clock: Arc<dyn crate::shared::Clock> = Arc::new(crate::shared::SystemClock);
311-
312-
let repository_factory =
313-
crate::infrastructure::persistence::repository_factory::RepositoryFactory::new(
314-
std::time::Duration::from_secs(30),
315-
);
316-
let repository = repository_factory.create(self.temp_dir.path().to_path_buf());
317-
318-
let command_handler = ConfigureCommandHandler::new(ansible_client, clock, repository);
319-
320-
(command_handler, self.temp_dir)
321-
}
322-
}
323-
324-
#[test]
325-
fn it_should_create_configure_command_handler_with_all_dependencies() {
326-
let (command_handler, _temp_dir) = ConfigureCommandHandlerTestBuilder::new().build();
327-
328-
// Verify the command handler was created (basic structure test)
329-
// This test just verifies that the command handler can be created with the dependencies
330-
assert_eq!(Arc::strong_count(&command_handler.ansible_client), 1);
331-
}
332-
333-
#[test]
334-
fn it_should_have_correct_error_type_conversions() {
335-
// Test that all error types can convert to ConfigureCommandHandlerError
336-
let command_error = CommandError::StartupFailed {
337-
command: "test".to_string(),
338-
source: std::io::Error::new(std::io::ErrorKind::NotFound, "test"),
339-
};
340-
let configure_error: ConfigureCommandHandlerError = command_error.into();
341-
drop(configure_error);
342-
}
343-
344-
#[test]
345-
fn it_should_build_failure_context_from_command_error() {
346-
use chrono::{TimeZone, Utc};
347-
348-
let (command, temp_dir) = ConfigureCommandHandlerTestBuilder::new().build();
349-
350-
// Create test environment for trace generation
351-
let (environment, _env_temp_dir) = create_test_environment(&temp_dir);
352-
353-
let error = ConfigureCommandHandlerError::Command(CommandError::ExecutionFailed {
354-
command: "test".to_string(),
355-
exit_code: "1".to_string(),
356-
stdout: String::new(),
357-
stderr: "test error".to_string(),
358-
});
359-
360-
let started_at = Utc.with_ymd_and_hms(2025, 10, 7, 12, 0, 0).unwrap();
361-
let current_step = ConfigureStep::InstallDocker;
362-
let context = command.build_failure_context(&environment, &error, current_step, started_at);
363-
assert_eq!(context.failed_step, ConfigureStep::InstallDocker);
364-
assert_eq!(
365-
context.error_kind,
366-
crate::shared::ErrorKind::CommandExecution
367-
);
368-
assert_eq!(context.base.execution_started_at, started_at);
369-
}
370-
}
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
//! Configure Command Module
2+
//!
3+
//! This module implements the delivery-agnostic `ConfigureCommandHandler`
4+
//! for orchestrating infrastructure configuration business logic.
5+
//!
6+
//! ## Architecture
7+
//!
8+
//! The `ConfigureCommandHandler` implements the Command Pattern and uses Dependency Injection
9+
//! to interact with infrastructure services through interfaces:
10+
//!
11+
//! - **Repository Pattern**: Persists environment state via `EnvironmentRepository`
12+
//! - **Clock Abstraction**: Provides deterministic time for testing via `Clock` trait
13+
//! - **Domain-Driven Design**: Uses domain objects from `domain::environment`
14+
//!
15+
//! ## Design Principles
16+
//!
17+
//! - **Delivery-Agnostic**: Works with CLI, REST API, or any delivery mechanism
18+
//! - **Synchronous**: Follows existing patterns (no async/await)
19+
//! - **Explicit State Transitions**: Type-safe state machine for environment lifecycle
20+
//! - **Explicit Errors**: All errors implement `.help()` with actionable guidance
21+
//!
22+
//! ## Configuration Workflow
23+
//!
24+
//! The command handler orchestrates a multi-step workflow:
25+
//!
26+
//! 1. **Install Docker** - Install Docker engine on the provisioned VM
27+
//! 2. **Install Docker Compose** - Install Docker Compose for container orchestration
28+
//!
29+
//! ## State Management
30+
//!
31+
//! The command handler integrates with the type-state pattern for environment lifecycle:
32+
//!
33+
//! - Accepts `Environment<Provisioned>` as input
34+
//! - Transitions to `Environment<Configuring>` at start
35+
//! - Returns `Environment<Configured>` on success
36+
//! - Transitions to `Environment<ConfigureFailed>` on error
37+
//!
38+
//! State is persisted after each transition using the injected repository.
39+
40+
pub mod errors;
41+
pub mod handler;
42+
43+
#[cfg(test)]
44+
mod tests;
45+
46+
// Re-export main types for convenience
47+
pub use errors::ConfigureCommandHandlerError;
48+
pub use handler::ConfigureCommandHandler;
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
//! Test builders for Configure Command
2+
//!
3+
//! This module provides test builders that simplify test setup by managing
4+
//! dependencies and lifecycle for `ConfigureCommandHandler` tests.
5+
6+
use std::sync::Arc;
7+
8+
use tempfile::TempDir;
9+
10+
use crate::adapters::ansible::AnsibleClient;
11+
use crate::application::command_handlers::configure::ConfigureCommandHandler;
12+
use crate::domain::environment::{Configuring, Environment};
13+
14+
/// Helper function to create a test environment in Configuring state
15+
pub fn create_test_environment(_temp_dir: &TempDir) -> (Environment<Configuring>, TempDir) {
16+
use crate::domain::environment::testing::EnvironmentTestBuilder;
17+
18+
let (env, _data_dir, _build_dir, env_temp_dir) = EnvironmentTestBuilder::new()
19+
.with_name("test-env")
20+
.build_with_custom_paths();
21+
22+
// Environment is created with paths inside env_temp_dir
23+
// which will be automatically cleaned up when env_temp_dir is dropped
24+
25+
// Transition Created -> Provisioning -> Provisioned -> Configuring
26+
(
27+
env.start_provisioning().provisioned().start_configuring(),
28+
env_temp_dir,
29+
)
30+
}
31+
32+
/// Test builder for `ConfigureCommandHandler` that manages dependencies and lifecycle
33+
///
34+
/// This builder simplifies test setup by:
35+
/// - Managing `TempDir` lifecycle
36+
/// - Providing sensible defaults for all dependencies
37+
/// - Returning only the command and necessary test artifacts
38+
pub struct ConfigureCommandHandlerTestBuilder {
39+
temp_dir: TempDir,
40+
}
41+
42+
impl ConfigureCommandHandlerTestBuilder {
43+
/// Create a new test builder with default configuration
44+
#[must_use]
45+
pub fn new() -> Self {
46+
let temp_dir = TempDir::new().expect("Failed to create temp dir");
47+
Self { temp_dir }
48+
}
49+
50+
/// Build the `ConfigureCommandHandler` with all dependencies
51+
///
52+
/// Returns: (`command`, `temp_dir`)
53+
/// The `temp_dir` must be kept alive for the duration of the test.
54+
pub fn build(self) -> (ConfigureCommandHandler, TempDir) {
55+
let ansible_client = Arc::new(AnsibleClient::new(self.temp_dir.path()));
56+
let clock: Arc<dyn crate::shared::Clock> = Arc::new(crate::shared::SystemClock);
57+
58+
let repository_factory =
59+
crate::infrastructure::persistence::repository_factory::RepositoryFactory::new(
60+
std::time::Duration::from_secs(30),
61+
);
62+
let repository = repository_factory.create(self.temp_dir.path().to_path_buf());
63+
64+
let command_handler = ConfigureCommandHandler::new(ansible_client, clock, repository);
65+
66+
(command_handler, self.temp_dir)
67+
}
68+
}
69+
70+
impl Default for ConfigureCommandHandlerTestBuilder {
71+
fn default() -> Self {
72+
Self::new()
73+
}
74+
}

0 commit comments

Comments
 (0)