From 8e36cb56c56ac89bec4de97b423bfbbbfc9c935a Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Fri, 14 Nov 2025 16:25:14 +0000 Subject: [PATCH 01/19] docs: [#174] add issue spec for implementing provision console command - Create detailed specification for provision console command - Document all 7 phases including manual E2E testing - Add architecture requirements and acceptance criteria - Update roadmap to reference issue #174 --- ...174-implement-provision-console-command.md | 516 ++++++++++++++++++ docs/roadmap.md | 2 +- 2 files changed, 517 insertions(+), 1 deletion(-) create mode 100644 docs/issues/174-implement-provision-console-command.md diff --git a/docs/issues/174-implement-provision-console-command.md b/docs/issues/174-implement-provision-console-command.md new file mode 100644 index 00000000..428aee17 --- /dev/null +++ b/docs/issues/174-implement-provision-console-command.md @@ -0,0 +1,516 @@ +# Implement Provision Console Command + +**Issue**: #174 +**Parent Epic**: #2 - Scaffolding for main app +**Related**: + +- Roadmap task 1.6: [docs/roadmap.md](../roadmap.md) +- Application layer: `ProvisionCommandHandler` (already implemented) +- DDD architecture: [docs/codebase-architecture.md](../codebase-architecture.md) +- Module organization: [docs/contributing/module-organization.md](../contributing/module-organization.md) +- Error handling: [docs/contributing/error-handling.md](../contributing/error-handling.md) + +## Overview + +Implement the presentation layer for the `provision` console command to enable users to provision VM infrastructure from the CLI. The application layer `ProvisionCommandHandler` is already implemented - this task focuses on creating the console interface (CLI command, controller, router integration, and user interaction). + +## Goals + +- [ ] Add `provision` subcommand to CLI command definitions +- [ ] Create presentation layer controller following the destroy controller pattern +- [ ] Integrate controller into the dispatch router +- [ ] Provide clear user feedback during provisioning workflow +- [ ] Handle errors with actionable messages + +## 🏗️ Architecture Requirements + +**DDD Layer**: Presentation +**Module Path**: `src/presentation/` +**Pattern**: CLI Subcommand + Controller (single command pattern) + +### Module Structure Requirements + +- [ ] Follow DDD layer separation (see [docs/codebase-architecture.md](../codebase-architecture.md)) +- [ ] Use `ExecutionContext` pattern for dependency injection +- [ ] Controller follows destroy controller reference pattern +- [ ] Error handling uses explicit enum errors (see [docs/contributing/error-handling.md](../contributing/error-handling.md)) +- [ ] Module organization follows project conventions (see [docs/contributing/module-organization.md](../contributing/module-organization.md)) + +### Architectural Constraints + +- [ ] **No business logic in presentation layer** - delegate to `ProvisionCommandHandler` +- [ ] **Single command pattern** - No subcommands, direct execution like destroy +- [ ] **Dependencies flow toward application layer** - Controller depends on `ProvisionCommandHandler` +- [ ] **User output through `UserOutput` trait** - Consistent output formatting +- [ ] **Error messages are actionable** - Include help methods with troubleshooting steps + +### Anti-Patterns to Avoid + +- ❌ Implementing provisioning logic in controller (already in `ProvisionCommandHandler`) +- ❌ Direct infrastructure access from presentation layer +- ❌ Using `anyhow` for command-specific errors +- ❌ Mixing routing and execution logic + +## Specifications + +### CLI Command Definition + +Add the `Provision` command to `src/presentation/input/cli/commands.rs`: + +```rust +/// Available CLI commands +#[derive(Subcommand, Debug)] +pub enum Commands { + // ... existing commands ... + + /// Provision a new deployment environment infrastructure + /// + /// This command provisions the virtual machine infrastructure for a deployment + /// environment that was previously created. It will: + /// - Render and apply OpenTofu templates + /// - Create LXD VM instances + /// - Configure networking + /// - Wait for SSH connectivity + /// - Wait for cloud-init completion + /// + /// The environment must be in "Created" state (use 'create environment' first). + Provision { + /// Name of the environment to provision + /// + /// The environment name must match an existing environment that was + /// previously created and is in "Created" state. + environment: String, + }, +} +``` + +### Controller Structure + +Create `src/presentation/controllers/provision/` following the destroy controller pattern: + +```text +src/presentation/controllers/provision/ +├── handler.rs # Main command handler function +├── errors.rs # ProvisionSubcommandError with help methods +├── tests/ # Command-specific tests +│ └── mod.rs # Test module organization +└── mod.rs # Module exports +``` + +### Controller Handler API + +**File**: `src/presentation/controllers/provision/handler.rs` + +```rust +//! Provision Command Handler +//! +//! This module handles the provision command execution at the presentation layer, +//! including environment validation, repository initialization, and user interaction. + +use std::sync::Arc; +use std::path::Path; + +use crate::application::command_handlers::ProvisionCommandHandler; +use crate::domain::environment::name::EnvironmentName; +use crate::domain::environment::repository::EnvironmentRepository; +use crate::domain::environment::state::Provisioned; +use crate::domain::environment::Environment; +use crate::infrastructure::persistence::repository_factory::RepositoryFactory; +use crate::presentation::views::progress::ProgressReporter; +use crate::presentation::views::UserOutput; +use crate::shared::clock::Clock; + +use super::errors::ProvisionSubcommandError; + +/// Number of main steps in the provision workflow +const PROVISION_WORKFLOW_STEPS: usize = 9; + +/// Handle provision command using `ExecutionContext` pattern +/// +/// # Arguments +/// +/// * `environment_name` - Name of the environment to provision +/// * `working_dir` - Working directory path for operations +/// * `context` - Execution context providing access to services +/// +/// # Returns +/// +/// * `Ok(Environment)` - Environment provisioned successfully +/// * `Err(ProvisionSubcommandError)` - Provision operation failed +/// +/// # Errors +/// +/// Returns `ProvisionSubcommandError` when: +/// * Environment name is invalid or contains special characters +/// * Working directory is not accessible or doesn't exist +/// * Environment is not found or not in "Created" state +/// * Infrastructure provisioning fails (OpenTofu/LXD errors) +/// * SSH connectivity cannot be established +/// * Cloud-init does not complete successfully +#[allow(clippy::result_large_err)] +pub fn handle( + environment_name: &str, + working_dir: &Path, + context: &crate::presentation::dispatch::context::ExecutionContext, +) -> Result, ProvisionSubcommandError> { + // Implementation follows destroy controller pattern + // 1. Validate environment name + // 2. Initialize repository + // 3. Load environment (must be in Created state) + // 4. Create progress reporter + // 5. Execute ProvisionCommandHandler + // 6. Report success +} +``` + +### Error Type + +**File**: `src/presentation/controllers/provision/errors.rs` + +```rust +//! Provision Subcommand Error Types +//! +//! This module defines errors specific to the provision console subcommand. + +use std::fmt; +use crate::application::command_handlers::provision::errors::ProvisionCommandHandlerError; +use crate::domain::environment::name::InvalidEnvironmentName; +use crate::domain::environment::repository::RepositoryError; + +/// Error type for provision subcommand execution +#[derive(Debug)] +pub enum ProvisionSubcommandError { + /// Environment name validation failed + InvalidName(InvalidEnvironmentName), + + /// Repository initialization or access failed + Repository(RepositoryError), + + /// Environment not found or invalid state + EnvironmentNotFound(String), + + /// Environment is not in Created state + InvalidState { + environment: String, + current_state: String, + required_state: String, + }, + + /// Provision command handler execution failed + Handler(ProvisionCommandHandlerError), + + /// Progress reporter encountered poisoned mutex + ProgressReporterPoisoned, +} + +impl ProvisionSubcommandError { + /// Get actionable help message for troubleshooting + #[must_use] + pub fn help(&self) -> String { + match self { + Self::InvalidName(e) => format!( + "Environment name is invalid: {}\n\ + \n\ + Valid names must:\n\ + - Start with a letter or underscore\n\ + - Contain only letters, numbers, hyphens, and underscores\n\ + - Be between 1 and 63 characters\n\ + \n\ + Examples: 'prod-01', 'staging_env', 'test-environment'", + e + ), + Self::Repository(e) => format!( + "Failed to access environment repository: {}\n\ + \n\ + Troubleshooting:\n\ + 1. Verify the working directory exists and is writable\n\ + 2. Check file system permissions\n\ + 3. Ensure sufficient disk space\n\ + 4. Check for file locks by other processes", + e + ), + Self::EnvironmentNotFound(name) => format!( + "Environment '{}' not found\n\ + \n\ + The environment must be created before provisioning.\n\ + \n\ + Steps:\n\ + 1. Create environment configuration: torrust-tracker-deployer create template\n\ + 2. Edit the generated template with your settings\n\ + 3. Create the environment: torrust-tracker-deployer create environment -f \n\ + 4. Then provision: torrust-tracker-deployer provision {}", + name, name + ), + Self::InvalidState { environment, current_state, required_state } => format!( + "Environment '{}' is in '{}' state, but '{}' state is required\n\ + \n\ + The provision command requires an environment in 'Created' state.\n\ + \n\ + Current state: {}\n\ + \n\ + Troubleshooting:\n\ + - If already provisioned: Environment is ready to use\n\ + - If provisioning failed: Review error logs and retry after fixing issues\n\ + - If in wrong state: Create a new environment or use appropriate command", + environment, current_state, required_state, current_state + ), + Self::Handler(e) => format!( + "Provision command failed: {}\n\ + \n\ + See error details above for specific failure information.\n\ + Check logs for detailed trace information.", + e + ), + Self::ProgressReporterPoisoned => { + "Progress reporter encountered poisoned mutex\n\ + \n\ + This is an internal error. Please report this issue with:\n\ + - Command that was running\n\ + - Log file contents\n\ + - Steps to reproduce" + .to_string() + } + } + } +} + +// Implement Display, Error, and From traits following destroy pattern +``` + +### Router Integration + +**File**: `src/presentation/dispatch/router.rs` + +Add the new command to the router's match statement: + +```rust +pub fn route_command( + command: Commands, + working_dir: &Path, + context: &ExecutionContext, +) -> Result<(), CommandError> { + match command { + Commands::Create { action } => { + create::route_command(action, working_dir, context)?; + Ok(()) + } + Commands::Destroy { environment } => { + destroy::handle(&environment, working_dir, context)?; + Ok(()) + } + Commands::Provision { environment } => { + provision::handle(&environment, working_dir, context)?; + Ok(()) + } + } +} +``` + +### User Feedback + +The controller should provide clear progress updates during the provisioning workflow: + +```text +Starting provision for environment 'my-env'... + +[Step 1/9] Rendering OpenTofu templates... +[Step 2/9] Initializing OpenTofu... +[Step 3/9] Validating infrastructure configuration... +[Step 4/9] Planning infrastructure... +[Step 5/9] Applying infrastructure... +[Step 6/9] Getting instance information... +[Step 7/9] Rendering Ansible templates... +[Step 8/9] Waiting for SSH connectivity... +[Step 9/9] Waiting for cloud-init completion... + +✓ Environment 'my-env' provisioned successfully + Instance IP: 10.x.x.x + State: Provisioned +``` + +## Implementation Plan + +### Phase 1: CLI Command Definition (30 minutes) + +- [ ] Add `Provision` variant to `Commands` enum in `src/presentation/input/cli/commands.rs` +- [ ] Add comprehensive documentation comments +- [ ] Test CLI parsing with `cargo build` + +### Phase 2: Controller Structure (1 hour) + +- [ ] Create `src/presentation/controllers/provision/` directory +- [ ] Create `mod.rs` with module exports +- [ ] Create `errors.rs` with `ProvisionSubcommandError` enum +- [ ] Implement `help()` method for all error variants +- [ ] Implement `Display` and `Error` traits +- [ ] Implement `From` conversions for error types + +### Phase 3: Controller Handler (2 hours) + +- [ ] Create `handler.rs` with main `handle()` function +- [ ] Implement environment name validation +- [ ] Implement repository initialization +- [ ] Load environment from repository (validate Created state) +- [ ] Create progress reporter with 9 steps +- [ ] Call `ProvisionCommandHandler::execute()` +- [ ] Handle success and error cases +- [ ] Add comprehensive documentation + +### Phase 4: Router Integration (30 minutes) + +- [ ] Add `provision` module to `src/presentation/controllers/mod.rs` +- [ ] Update `src/presentation/dispatch/router.rs` with provision route +- [ ] Update `CommandError` if needed for provision errors +- [ ] Test routing with minimal integration test + +### Phase 5: Testing (1.5 hours) + +- [ ] Create `src/presentation/controllers/provision/tests/mod.rs` +- [ ] Add unit tests for error help messages +- [ ] Add unit tests for handler with mock services +- [ ] Add integration test for successful provision +- [ ] Add integration test for error cases (invalid state, not found) +- [ ] Test CLI parsing and routing + +### Phase 6: Documentation and Review (30 minutes) + +- [ ] Update module documentation +- [ ] Verify all error messages are actionable +- [ ] Check code follows module organization conventions +- [ ] Verify import style (short names, no long paths) +- [ ] Run linters and fix any issues + +### Phase 7: Manual End-to-End Testing (45 minutes) + +Perform complete workflow testing to verify the provision command integrates correctly with create and destroy commands: + +- [ ] **Step 1**: Create temporary test directory + - [ ] Create a clean temporary directory for testing + - [ ] Navigate to the test directory + +- [ ] **Step 2**: Run create command + - [ ] Generate template: `torrust-tracker-deployer create template` + - [ ] Edit template with valid test configuration + - [ ] Create environment: `torrust-tracker-deployer create environment -f environment-template.json` + - [ ] Verify command completes successfully + - [ ] Verify environment state file created in `data/` directory + - [ ] Verify state shows `Created` status + - [ ] Verify all required fields are present in state file + +- [ ] **Step 3**: Run provision command (NEW - being tested) + - [ ] Provision infrastructure: `torrust-tracker-deployer provision ` + - [ ] Verify all 9 progress steps are displayed + - [ ] Verify command completes successfully + - [ ] Verify success message includes instance IP address + - [ ] Verify environment state file updated in `data/` directory + - [ ] Verify state shows `Provisioned` status + - [ ] Verify instance IP is saved in state file + - [ ] Verify `build/` directory contains generated templates + +- [ ] **Step 4**: Run destroy command + - [ ] Destroy environment: `torrust-tracker-deployer destroy ` + - [ ] Verify command completes successfully + - [ ] Verify environment state file updated to `Destroyed` status + - [ ] Verify infrastructure is actually torn down (LXD instance removed) + +- [ ] **Step 5**: Validate state consistency + - [ ] Verify state transitions are correct: Created → Provisioned → Destroyed + - [ ] Verify no orphaned files in `data/` or `build/` directories + - [ ] Verify log files contain complete trace information + +- [ ] **Step 6**: Test error scenarios + - [ ] Try provisioning non-existent environment (should fail with clear error) + - [ ] Try provisioning already provisioned environment (should fail with state error) + - [ ] Verify all error messages are actionable and helpful + +## Acceptance Criteria + +> **Note for Contributors**: These criteria define what the PR reviewer will check. Use this as your pre-review checklist before submitting the PR to minimize back-and-forth iterations. + +**Quality Checks**: + +- [ ] Pre-commit checks pass: `./scripts/pre-commit.sh` + +**CLI Integration**: + +- [ ] `torrust-tracker-deployer provision ` command is available +- [ ] `torrust-tracker-deployer provision --help` shows clear documentation +- [ ] CLI parsing correctly extracts environment name parameter + +**Controller Implementation**: + +- [ ] Controller follows destroy controller reference pattern +- [ ] Handler function uses `ExecutionContext` pattern +- [ ] Error type provides actionable help messages for all variants +- [ ] Progress reporter shows all 9 provisioning steps +- [ ] Success message includes instance IP address + +**Router Integration**: + +- [ ] Provision command is registered in dispatch router +- [ ] Router correctly routes to provision controller +- [ ] Error handling flows correctly through layers + +**Error Handling**: + +- [ ] Invalid environment name shows validation rules +- [ ] Environment not found suggests create workflow +- [ ] Invalid state error explains current state and required state +- [ ] All error messages follow actionability principles +- [ ] Errors include context for traceability + +**Testing**: + +- [ ] Unit tests cover error help messages +- [ ] Unit tests cover handler logic with mocks +- [ ] Integration tests verify successful provision workflow +- [ ] Integration tests verify error cases +- [ ] All tests pass locally + +**Code Quality**: + +- [ ] Follows DDD layer placement guidelines +- [ ] Respects dependency flow rules +- [ ] Uses appropriate module organization +- [ ] Import style uses short names +- [ ] Documentation is comprehensive +- [ ] No clippy warnings +- [ ] Code is formatted with rustfmt + +**User Experience**: + +- [ ] Progress updates are clear and informative +- [ ] Success message includes all relevant information +- [ ] Error messages guide users to solutions +- [ ] Command integrates seamlessly with existing CLI + +## Related Documentation + +- [Roadmap](../roadmap.md) - Task 1.6 +- [Codebase Architecture](../codebase-architecture.md) - DDD layers and patterns +- [Module Organization](../contributing/module-organization.md) - Code organization conventions +- [Error Handling](../contributing/error-handling.md) - Error handling principles +- [Destroy Controller](../../src/presentation/controllers/destroy/) - Reference implementation +- [Application Layer Handler](../../src/application/command_handlers/provision/handler.rs) - Business logic + +## Notes + +### Why This Task is Important + +The `ProvisionCommandHandler` in the application layer contains all the business logic for provisioning infrastructure, but users cannot access it from the CLI. This task creates the user interface layer that makes the functionality available and user-friendly. + +### Design Decisions + +1. **Single Command Pattern**: Following destroy controller pattern since provision is a single command without subcommands + +2. **State Validation**: Must validate environment is in "Created" state before provisioning + +3. **Progress Reporting**: Provision has 9 steps (more than destroy's 3), requires clear progress feedback + +4. **Error Messages**: Focus on guiding users through the create → provision → configure workflow + +5. **No Business Logic**: All provisioning logic stays in `ProvisionCommandHandler` - controller only handles presentation concerns + +### Estimated Time + +Total: ~6 hours for complete implementation, testing, and documentation diff --git a/docs/roadmap.md b/docs/roadmap.md index 62ac1e21..a158c996 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -41,7 +41,7 @@ When starting work on a new feature: - Folder module structure with focused submodules - [x] **1.5** Create command `torrust-tracker-deployer create` to create a new environment ✅ Completed - EPIC: [Implement Create Environment Command](https://github.com/torrust/torrust-tracker-deployer/issues/34) - GitHub Issue #34 -- [ ] **1.6** Create command `torrust-tracker-deployer provision` to provision VM infrastructure (UI layer only) +- [ ] **1.6** Create command `torrust-tracker-deployer provision` to provision VM infrastructure (UI layer only) - [Issue #174](https://github.com/torrust/torrust-tracker-deployer/issues/174) - **Note:** The App layer ProvisionCommand is already implemented, this task focuses on the console subcommand interface - Implementation should call the existing ProvisionCommand business logic - Handle user input, validation, and output presentation From 25c36245ffd13cd4c7eb742fa75a79e8aead5224 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 08:14:34 +0000 Subject: [PATCH 02/19] refactor: [#174] remove AnsibleClient from ProvisionCommandHandler constructor Move AnsibleClient from constructor dependency to local creation within wait_for_readiness method. This is the first step in separating global dependencies (Clock, Repository) from runtime/environment-specific dependencies (AnsibleClient, TofuTemplateRenderer, etc.). Benefits: - Reduces presentation layer complexity by avoiding environment-specific dependency construction in controllers - Creates environment-specific services in application layer where environment context is available - Keeps environment path derivation logic centralized Trade-offs: - Harder to mock environment-specific dependencies in tests - Can be mitigated in future with factory pattern if needed Changes: - Remove ansible_client field from ProvisionCommandHandler - Create AnsibleClient locally in wait_for_readiness with environment's working directory - Update test builders and E2E tests to remove ansible_client injection --- .../command_handlers/provision/handler.rs | 18 +++++++++++------- .../provision/tests/builders.rs | 4 ---- .../virtual_machine/run_provision_command.rs | 1 - 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/application/command_handlers/provision/handler.rs b/src/application/command_handlers/provision/handler.rs index 427f7827..67588a5d 100644 --- a/src/application/command_handlers/provision/handler.rs +++ b/src/application/command_handlers/provision/handler.rs @@ -50,7 +50,6 @@ use crate::shared::error::Traceable; pub struct ProvisionCommandHandler { pub(crate) tofu_template_renderer: Arc, pub(crate) ansible_template_renderer: Arc, - pub(crate) ansible_client: Arc, pub(crate) opentofu_client: Arc, pub(crate) clock: Arc, pub(crate) repository: TypedEnvironmentRepository, @@ -62,7 +61,6 @@ impl ProvisionCommandHandler { pub fn new( tofu_template_renderer: Arc, ansible_template_renderer: Arc, - ansible_client: Arc, opentofu_client: Arc, clock: Arc, repository: Arc, @@ -70,7 +68,6 @@ impl ProvisionCommandHandler { Self { tofu_template_renderer, ansible_template_renderer, - ansible_client, opentofu_client, clock, repository: TypedEnvironmentRepository::new(repository), @@ -202,9 +199,13 @@ impl ProvisionCommandHandler { .map_err(|e| (e, current_step))?; let current_step = ProvisionStep::WaitSshConnectivity; - self.wait_for_readiness(ssh_credentials, instance_ip) - .await - .map_err(|e| (e, current_step))?; + self.wait_for_readiness( + &environment.ansible_build_dir(), + ssh_credentials, + instance_ip, + ) + .await + .map_err(|e| (e, current_step))?; // Transition to Provisioned state let provisioned = environment.clone().provisioned(); @@ -303,6 +304,7 @@ impl ProvisionCommandHandler { /// Returns an error if SSH connectivity fails or cloud-init does not complete async fn wait_for_readiness( &self, + ansible_working_dir: &std::path::Path, ssh_credentials: &SshCredentials, instance_ip: IpAddr, ) -> Result<(), ProvisionCommandHandlerError> { @@ -311,7 +313,9 @@ impl ProvisionCommandHandler { .execute() .await?; - WaitForCloudInitStep::new(Arc::clone(&self.ansible_client)).execute()?; + let ansible_client = Arc::new(AnsibleClient::new(ansible_working_dir)); + + WaitForCloudInitStep::new(Arc::clone(&ansible_client)).execute()?; Ok(()) } diff --git a/src/application/command_handlers/provision/tests/builders.rs b/src/application/command_handlers/provision/tests/builders.rs index 1e5dadb9..9e6cdfe5 100644 --- a/src/application/command_handlers/provision/tests/builders.rs +++ b/src/application/command_handlers/provision/tests/builders.rs @@ -7,7 +7,6 @@ use std::sync::Arc; use tempfile::TempDir; -use crate::adapters::ansible::AnsibleClient; use crate::adapters::ssh::SshCredentials; use crate::application::command_handlers::provision::ProvisionCommandHandler; use crate::domain::{InstanceName, ProfileName}; @@ -79,8 +78,6 @@ impl ProvisionCommandHandlerTestBuilder { template_manager, )); - let ansible_client = Arc::new(AnsibleClient::new(self.temp_dir.path())); - let opentofu_client = Arc::new(crate::adapters::tofu::client::OpenTofuClient::new( self.temp_dir.path(), )); @@ -96,7 +93,6 @@ impl ProvisionCommandHandlerTestBuilder { let command_handler = ProvisionCommandHandler::new( tofu_renderer, ansible_renderer, - ansible_client, opentofu_client, clock, repository, diff --git a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs index 575206c3..e3a17088 100644 --- a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs +++ b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs @@ -149,7 +149,6 @@ pub async fn run_provision_command( let provision_command_handler = ProvisionCommandHandler::new( Arc::clone(&test_context.services.tofu_template_renderer), Arc::clone(&test_context.services.ansible_template_renderer), - Arc::clone(&test_context.services.ansible_client), Arc::clone(&test_context.services.opentofu_client), Arc::clone(&test_context.services.clock), repository, From 71fbb529e28f29cbdb96d638604415a74fad291c Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 08:24:49 +0000 Subject: [PATCH 03/19] refactor: [#174] make ProvisionCommandHandler fields private and clean up tests Make all ProvisionCommandHandler struct fields private to enforce proper encapsulation. Remove test that relied on internal field access. Changes: - Change all pub(crate) fields to private in ProvisionCommandHandler - Remove it_should_create_provision_command_handler_with_all_dependencies test (relied on accessing private fields) - Add #[allow(dead_code)] to builder methods (will be used in future tests) - Add #[cfg(test)] to test modules for proper scoping --- .../command_handlers/provision/handler.rs | 10 +++++----- .../provision/tests/builders.rs | 2 ++ .../provision/tests/integration.rs | 20 ------------------- .../command_handlers/provision/tests/mod.rs | 2 ++ 4 files changed, 9 insertions(+), 25 deletions(-) diff --git a/src/application/command_handlers/provision/handler.rs b/src/application/command_handlers/provision/handler.rs index 67588a5d..3151bb51 100644 --- a/src/application/command_handlers/provision/handler.rs +++ b/src/application/command_handlers/provision/handler.rs @@ -48,11 +48,11 @@ use crate::shared::error::Traceable; /// State is persisted after each transition using the injected repository. /// Persistence failures are logged but don't fail the command handler (state remains valid in memory). pub struct ProvisionCommandHandler { - pub(crate) tofu_template_renderer: Arc, - pub(crate) ansible_template_renderer: Arc, - pub(crate) opentofu_client: Arc, - pub(crate) clock: Arc, - pub(crate) repository: TypedEnvironmentRepository, + tofu_template_renderer: Arc, + ansible_template_renderer: Arc, + opentofu_client: Arc, + clock: Arc, + repository: TypedEnvironmentRepository, } impl ProvisionCommandHandler { diff --git a/src/application/command_handlers/provision/tests/builders.rs b/src/application/command_handlers/provision/tests/builders.rs index 9e6cdfe5..5b385097 100644 --- a/src/application/command_handlers/provision/tests/builders.rs +++ b/src/application/command_handlers/provision/tests/builders.rs @@ -22,6 +22,7 @@ use crate::shared::Username; /// - Allowing selective customization of dependencies /// - Returning only the command handler and necessary test artifacts pub struct ProvisionCommandHandlerTestBuilder { + #[allow(dead_code)] temp_dir: TempDir, ssh_credentials: Option, } @@ -48,6 +49,7 @@ impl ProvisionCommandHandlerTestBuilder { /// /// Returns: (`command_handler`, `temp_dir`, `ssh_credentials`) /// The `temp_dir` must be kept alive for the duration of the test. + #[allow(dead_code)] pub fn build(self) -> (ProvisionCommandHandler, TempDir, SshCredentials) { let template_manager = Arc::new(crate::domain::template::TemplateManager::new( self.temp_dir.path(), diff --git a/src/application/command_handlers/provision/tests/integration.rs b/src/application/command_handlers/provision/tests/integration.rs index 3be5e982..d4ac30d7 100644 --- a/src/application/command_handlers/provision/tests/integration.rs +++ b/src/application/command_handlers/provision/tests/integration.rs @@ -2,32 +2,12 @@ //! //! This module contains integration tests for the `ProvisionCommandHandler`. -use std::sync::Arc; - -use super::builders::ProvisionCommandHandlerTestBuilder; use crate::adapters::ssh::SshError; use crate::adapters::tofu::client::OpenTofuError; use crate::application::command_handlers::provision::ProvisionCommandHandlerError; use crate::infrastructure::external_tools::tofu::ProvisionTemplateError; use crate::shared::command::CommandError; -#[test] -fn it_should_create_provision_command_handler_with_all_dependencies() { - let (command_handler, _temp_dir, _ssh_credentials) = - ProvisionCommandHandlerTestBuilder::new().build(); - - // Verify the command handler was created (basic structure test) - // This test just verifies that the command handler can be created with the dependencies - assert_eq!( - Arc::strong_count(&command_handler.tofu_template_renderer), - 1 - ); - assert_eq!( - Arc::strong_count(&command_handler.ansible_template_renderer), - 1 - ); -} - #[test] fn it_should_have_correct_error_type_conversions() { // Test that all error types can convert to ProvisionCommandHandlerError diff --git a/src/application/command_handlers/provision/tests/mod.rs b/src/application/command_handlers/provision/tests/mod.rs index ea67279e..45913649 100644 --- a/src/application/command_handlers/provision/tests/mod.rs +++ b/src/application/command_handlers/provision/tests/mod.rs @@ -2,5 +2,7 @@ //! //! This module contains test infrastructure and test cases for the `ProvisionCommandHandler`. +#[cfg(test)] pub mod builders; +#[cfg(test)] pub mod integration; From b79d7cf52b6e4e797ffc8a556c6f150e3282bc51 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 08:31:39 +0000 Subject: [PATCH 04/19] refactor: [#174] create AnsibleClient at workflow start instead of in wait_for_readiness Move AnsibleClient creation from wait_for_readiness method to the start of execute_provisioning_with_tracking. This improves the design by: - Creating environment-specific dependency at the workflow level where environment context is available - Reducing method parameters (ansible_working_dir -> ansible_client) - Making dependency flow more explicit and clearer - Keeping environment-specific service creation centralized The client is now created once with environment.ansible_build_dir() and passed to methods that need it, rather than passing paths and creating clients in multiple places. --- .../command_handlers/provision/handler.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/application/command_handlers/provision/handler.rs b/src/application/command_handlers/provision/handler.rs index 3151bb51..b4103ef4 100644 --- a/src/application/command_handlers/provision/handler.rs +++ b/src/application/command_handlers/provision/handler.rs @@ -177,6 +177,8 @@ impl ProvisionCommandHandler { { let ssh_credentials = environment.ssh_credentials(); + let ansible_client = Arc::new(AnsibleClient::new(environment.ansible_build_dir())); + // Track current step and execute each step // If an error occurs, we return it along with the current step @@ -199,13 +201,9 @@ impl ProvisionCommandHandler { .map_err(|e| (e, current_step))?; let current_step = ProvisionStep::WaitSshConnectivity; - self.wait_for_readiness( - &environment.ansible_build_dir(), - ssh_credentials, - instance_ip, - ) - .await - .map_err(|e| (e, current_step))?; + self.wait_for_readiness(&ansible_client, ssh_credentials, instance_ip) + .await + .map_err(|e| (e, current_step))?; // Transition to Provisioned state let provisioned = environment.clone().provisioned(); @@ -304,18 +302,17 @@ impl ProvisionCommandHandler { /// Returns an error if SSH connectivity fails or cloud-init does not complete async fn wait_for_readiness( &self, - ansible_working_dir: &std::path::Path, + ansible_client: &Arc, ssh_credentials: &SshCredentials, instance_ip: IpAddr, ) -> Result<(), ProvisionCommandHandlerError> { let ssh_config = SshConfig::with_default_port(ssh_credentials.clone(), instance_ip); + WaitForSSHConnectivityStep::new(ssh_config) .execute() .await?; - let ansible_client = Arc::new(AnsibleClient::new(ansible_working_dir)); - - WaitForCloudInitStep::new(Arc::clone(&ansible_client)).execute()?; + WaitForCloudInitStep::new(Arc::clone(ansible_client)).execute()?; Ok(()) } From b93affb95d3b414673be4f2d3ce320125e35be87 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 08:45:45 +0000 Subject: [PATCH 05/19] refactor: [#174] remove OpenTofuClient from ProvisionCommandHandler constructor Move OpenTofuClient from constructor dependency to local creation within execute_provisioning_with_tracking method. This continues the pattern of separating global dependencies from runtime/environment-specific ones. Changes: - Remove opentofu_client field from ProvisionCommandHandler struct - Create OpenTofuClient locally with environment.tofu_build_dir() - Convert create_instance() and get_instance_info() to static methods that accept opentofu_client as parameter - Update test builders and E2E tests to remove opentofu_client injection - Add OpenTofuClient import to use type directly Benefits: - OpenTofuClient now created with environment-specific working directory - Clearer separation between global and runtime dependencies - Reduced constructor complexity (one less parameter) - Static helper methods are more testable independently --- .../command_handlers/provision/handler.rs | 32 +++++++++++-------- .../provision/tests/builders.rs | 13 ++------ .../virtual_machine/run_provision_command.rs | 1 - 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/src/application/command_handlers/provision/handler.rs b/src/application/command_handlers/provision/handler.rs index b4103ef4..094673d9 100644 --- a/src/application/command_handlers/provision/handler.rs +++ b/src/application/command_handlers/provision/handler.rs @@ -9,6 +9,7 @@ use super::errors::ProvisionCommandHandlerError; use crate::adapters::ansible::AnsibleClient; use crate::adapters::ssh::{SshConfig, SshCredentials}; use crate::adapters::tofu::client::InstanceInfo; +use crate::adapters::OpenTofuClient; use crate::application::command_handlers::common::StepResult; use crate::application::steps::{ ApplyInfrastructureStep, GetInstanceInfoStep, InitializeInfrastructureStep, @@ -50,7 +51,6 @@ use crate::shared::error::Traceable; pub struct ProvisionCommandHandler { tofu_template_renderer: Arc, ansible_template_renderer: Arc, - opentofu_client: Arc, clock: Arc, repository: TypedEnvironmentRepository, } @@ -61,14 +61,12 @@ impl ProvisionCommandHandler { pub fn new( tofu_template_renderer: Arc, ansible_template_renderer: Arc, - opentofu_client: Arc, clock: Arc, repository: Arc, ) -> Self { Self { tofu_template_renderer, ansible_template_renderer, - opentofu_client, clock, repository: TypedEnvironmentRepository::new(repository), } @@ -178,6 +176,7 @@ impl ProvisionCommandHandler { let ssh_credentials = environment.ssh_credentials(); let ansible_client = Arc::new(AnsibleClient::new(environment.ansible_build_dir())); + let opentofu_client = Arc::new(OpenTofuClient::new(environment.tofu_build_dir())); // Track current step and execute each step // If an error occurs, we return it along with the current step @@ -188,10 +187,11 @@ impl ProvisionCommandHandler { .map_err(|e| (e, current_step))?; let current_step = ProvisionStep::OpenTofuInit; - self.create_instance().map_err(|e| (e, current_step))?; + Self::create_instance(&opentofu_client).map_err(|e| (e, current_step))?; let current_step = ProvisionStep::GetInstanceInfo; - let instance_info = self.get_instance_info().map_err(|e| (e, current_step))?; + let instance_info = + Self::get_instance_info(&opentofu_client).map_err(|e| (e, current_step))?; let instance_ip = instance_info.ip_address; let current_step = ProvisionStep::RenderAnsibleTemplates; @@ -238,11 +238,14 @@ impl ProvisionCommandHandler { /// # Errors /// /// Returns an error if any `OpenTofu` operation fails - fn create_instance(&self) -> Result<(), ProvisionCommandHandlerError> { - InitializeInfrastructureStep::new(Arc::clone(&self.opentofu_client)).execute()?; - ValidateInfrastructureStep::new(Arc::clone(&self.opentofu_client)).execute()?; - PlanInfrastructureStep::new(Arc::clone(&self.opentofu_client)).execute()?; - ApplyInfrastructureStep::new(Arc::clone(&self.opentofu_client)).execute()?; + fn create_instance( + opentofu_client: &Arc, + ) -> Result<(), ProvisionCommandHandlerError> { + InitializeInfrastructureStep::new(Arc::clone(opentofu_client)).execute()?; + ValidateInfrastructureStep::new(Arc::clone(opentofu_client)).execute()?; + PlanInfrastructureStep::new(Arc::clone(opentofu_client)).execute()?; + ApplyInfrastructureStep::new(Arc::clone(opentofu_client)).execute()?; + Ok(()) } @@ -253,9 +256,10 @@ impl ProvisionCommandHandler { /// # Errors /// /// Returns an error if instance information cannot be retrieved - fn get_instance_info(&self) -> Result { - let instance_info = - GetInstanceInfoStep::new(Arc::clone(&self.opentofu_client)).execute()?; + fn get_instance_info( + opentofu_client: &Arc, + ) -> Result { + let instance_info = GetInstanceInfoStep::new(Arc::clone(opentofu_client)).execute()?; Ok(instance_info) } @@ -278,6 +282,7 @@ impl ProvisionCommandHandler { ssh_port: u16, ) -> Result<(), ProvisionCommandHandlerError> { let socket_addr = std::net::SocketAddr::new(instance_ip, ssh_port); + RenderAnsibleTemplatesStep::new( Arc::clone(&self.ansible_template_renderer), ssh_credentials.clone(), @@ -285,6 +290,7 @@ impl ProvisionCommandHandler { ) .execute() .await?; + Ok(()) } diff --git a/src/application/command_handlers/provision/tests/builders.rs b/src/application/command_handlers/provision/tests/builders.rs index 5b385097..589d270e 100644 --- a/src/application/command_handlers/provision/tests/builders.rs +++ b/src/application/command_handlers/provision/tests/builders.rs @@ -80,10 +80,6 @@ impl ProvisionCommandHandlerTestBuilder { template_manager, )); - let opentofu_client = Arc::new(crate::adapters::tofu::client::OpenTofuClient::new( - self.temp_dir.path(), - )); - let clock: Arc = Arc::new(crate::shared::SystemClock); let repository_factory = @@ -92,13 +88,8 @@ impl ProvisionCommandHandlerTestBuilder { ); let repository = repository_factory.create(self.temp_dir.path().to_path_buf()); - let command_handler = ProvisionCommandHandler::new( - tofu_renderer, - ansible_renderer, - opentofu_client, - clock, - repository, - ); + let command_handler = + ProvisionCommandHandler::new(tofu_renderer, ansible_renderer, clock, repository); (command_handler, self.temp_dir, ssh_credentials) } diff --git a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs index e3a17088..f2b96bc5 100644 --- a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs +++ b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs @@ -149,7 +149,6 @@ pub async fn run_provision_command( let provision_command_handler = ProvisionCommandHandler::new( Arc::clone(&test_context.services.tofu_template_renderer), Arc::clone(&test_context.services.ansible_template_renderer), - Arc::clone(&test_context.services.opentofu_client), Arc::clone(&test_context.services.clock), repository, ); From fa3881731d0dd8213593729e035c53fb5b69daa3 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 09:57:26 +0000 Subject: [PATCH 06/19] refactor: [#174] inject TemplateManager as global dependency Add TemplateManager to ProvisionCommandHandler constructor as a global dependency. This is a preparatory step for removing TofuTemplateRenderer and AnsibleTemplateRenderer from the constructor. The TemplateManager is a truly global service that doesn't depend on environment context, making it suitable for constructor injection. It will be used to build template renderers locally within the command handler when environment context is available. Changes: - Add template_manager parameter to ProvisionCommandHandler::new() - Store as _template_manager field (prefixed with _ as currently unused) - Update test builders to pass TemplateManager - Update E2E tests to pass TemplateManager from test context Next step: Use TemplateManager to build TofuTemplateRenderer and AnsibleTemplateRenderer locally with environment-specific data. --- src/application/command_handlers/provision/handler.rs | 4 ++++ .../command_handlers/provision/tests/builders.rs | 11 ++++++++--- .../tasks/virtual_machine/run_provision_command.rs | 1 + 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/application/command_handlers/provision/handler.rs b/src/application/command_handlers/provision/handler.rs index 094673d9..57135d60 100644 --- a/src/application/command_handlers/provision/handler.rs +++ b/src/application/command_handlers/provision/handler.rs @@ -19,6 +19,7 @@ use crate::application::steps::{ use crate::domain::environment::repository::{EnvironmentRepository, TypedEnvironmentRepository}; use crate::domain::environment::state::{ProvisionFailureContext, ProvisionStep}; use crate::domain::environment::{Created, Environment, Provisioned, Provisioning}; +use crate::domain::TemplateManager; use crate::infrastructure::external_tools::ansible::AnsibleTemplateRenderer; use crate::infrastructure::external_tools::tofu::TofuTemplateRenderer; use crate::shared::error::Traceable; @@ -52,6 +53,7 @@ pub struct ProvisionCommandHandler { tofu_template_renderer: Arc, ansible_template_renderer: Arc, clock: Arc, + _template_manager: Arc, repository: TypedEnvironmentRepository, } @@ -62,12 +64,14 @@ impl ProvisionCommandHandler { tofu_template_renderer: Arc, ansible_template_renderer: Arc, clock: Arc, + template_manager: Arc, repository: Arc, ) -> Self { Self { tofu_template_renderer, ansible_template_renderer, clock, + _template_manager: template_manager, repository: TypedEnvironmentRepository::new(repository), } } diff --git a/src/application/command_handlers/provision/tests/builders.rs b/src/application/command_handlers/provision/tests/builders.rs index 589d270e..b04c8137 100644 --- a/src/application/command_handlers/provision/tests/builders.rs +++ b/src/application/command_handlers/provision/tests/builders.rs @@ -77,7 +77,7 @@ impl ProvisionCommandHandlerTestBuilder { let ansible_renderer = Arc::new(AnsibleTemplateRenderer::new( self.temp_dir.path(), - template_manager, + template_manager.clone(), )); let clock: Arc = Arc::new(crate::shared::SystemClock); @@ -88,8 +88,13 @@ impl ProvisionCommandHandlerTestBuilder { ); let repository = repository_factory.create(self.temp_dir.path().to_path_buf()); - let command_handler = - ProvisionCommandHandler::new(tofu_renderer, ansible_renderer, clock, repository); + let command_handler = ProvisionCommandHandler::new( + tofu_renderer, + ansible_renderer, + clock, + template_manager, + repository, + ); (command_handler, self.temp_dir, ssh_credentials) } diff --git a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs index f2b96bc5..e79a5703 100644 --- a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs +++ b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs @@ -150,6 +150,7 @@ pub async fn run_provision_command( Arc::clone(&test_context.services.tofu_template_renderer), Arc::clone(&test_context.services.ansible_template_renderer), Arc::clone(&test_context.services.clock), + Arc::clone(&test_context.services.template_manager), repository, ); From bf936387571ef7539fa8fe1adced285fc2a06cce Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 10:14:01 +0000 Subject: [PATCH 07/19] refactor: [#174] remove template renderers from ProvisionCommandHandler constructor Remove TofuTemplateRenderer and AnsibleTemplateRenderer from constructor and create them locally within execute_provisioning_with_tracking using environment-specific context. This completes the refactoring to separate global dependencies from runtime/environment-specific dependencies. Changes: - Remove tofu_template_renderer and ansible_template_renderer fields - Remove underscore prefix from template_manager (now actively used) - Create TofuTemplateRenderer locally with environment data: - template_manager, build_dir, ssh_credentials, instance_name, profile_name - Create AnsibleTemplateRenderer locally with environment data: - build_dir, template_manager - Pass renderers as parameters to helper methods - Remove renderer construction from test builders and E2E tests - Simplify constructor to only accept truly global dependencies Constructor now only accepts global dependencies: - Clock (system time) - TemplateManager (global template service) - Repository (persistence) All environment-specific services are created within the workflow where environment context is available. This keeps the presentation layer thin and centralizes environment-specific logic in the application layer. --- .../command_handlers/provision/handler.rs | 49 ++++++++++++------- .../provision/tests/builders.rs | 25 +--------- .../virtual_machine/run_provision_command.rs | 2 - 3 files changed, 33 insertions(+), 43 deletions(-) diff --git a/src/application/command_handlers/provision/handler.rs b/src/application/command_handlers/provision/handler.rs index 57135d60..890c82f3 100644 --- a/src/application/command_handlers/provision/handler.rs +++ b/src/application/command_handlers/provision/handler.rs @@ -50,10 +50,8 @@ use crate::shared::error::Traceable; /// State is persisted after each transition using the injected repository. /// Persistence failures are logged but don't fail the command handler (state remains valid in memory). pub struct ProvisionCommandHandler { - tofu_template_renderer: Arc, - ansible_template_renderer: Arc, clock: Arc, - _template_manager: Arc, + template_manager: Arc, repository: TypedEnvironmentRepository, } @@ -61,17 +59,13 @@ impl ProvisionCommandHandler { /// Create a new `ProvisionCommandHandler` #[must_use] pub fn new( - tofu_template_renderer: Arc, - ansible_template_renderer: Arc, clock: Arc, template_manager: Arc, repository: Arc, ) -> Self { Self { - tofu_template_renderer, - ansible_template_renderer, clock, - _template_manager: template_manager, + template_manager, repository: TypedEnvironmentRepository::new(repository), } } @@ -179,14 +173,26 @@ impl ProvisionCommandHandler { { let ssh_credentials = environment.ssh_credentials(); - let ansible_client = Arc::new(AnsibleClient::new(environment.ansible_build_dir())); + let tofu_template_renderer = Arc::new(TofuTemplateRenderer::new( + self.template_manager.clone(), + environment.build_dir(), + environment.ssh_credentials().clone(), + environment.instance_name().clone(), + environment.profile_name().clone(), + )); let opentofu_client = Arc::new(OpenTofuClient::new(environment.tofu_build_dir())); + let ansible_client = Arc::new(AnsibleClient::new(environment.ansible_build_dir())); + let ansible_template_renderer = Arc::new(AnsibleTemplateRenderer::new( + environment.build_dir(), + self.template_manager.clone(), + )); + // Track current step and execute each step // If an error occurs, we return it along with the current step let current_step = ProvisionStep::RenderOpenTofuTemplates; - self.render_opentofu_templates() + self.render_opentofu_templates(&tofu_template_renderer) .await .map_err(|e| (e, current_step))?; @@ -199,10 +205,14 @@ impl ProvisionCommandHandler { let instance_ip = instance_info.ip_address; let current_step = ProvisionStep::RenderAnsibleTemplates; - let ssh_port = environment.ssh_port(); - self.render_ansible_templates(ssh_credentials, instance_ip, ssh_port) - .await - .map_err(|e| (e, current_step))?; + self.render_ansible_templates( + &ansible_template_renderer, + ssh_credentials, + instance_ip, + environment.ssh_port(), + ) + .await + .map_err(|e| (e, current_step))?; let current_step = ProvisionStep::WaitSshConnectivity; self.wait_for_readiness(&ansible_client, ssh_credentials, instance_ip) @@ -224,10 +234,14 @@ impl ProvisionCommandHandler { /// # Errors /// /// Returns an error if template rendering fails - async fn render_opentofu_templates(&self) -> Result<(), ProvisionCommandHandlerError> { - RenderOpenTofuTemplatesStep::new(Arc::clone(&self.tofu_template_renderer)) + async fn render_opentofu_templates( + &self, + tofu_template_renderer: &Arc, + ) -> Result<(), ProvisionCommandHandlerError> { + RenderOpenTofuTemplatesStep::new(tofu_template_renderer.clone()) .execute() .await?; + Ok(()) } @@ -281,6 +295,7 @@ impl ProvisionCommandHandler { /// Returns an error if template rendering fails async fn render_ansible_templates( &self, + ansible_template_renderer: &Arc, ssh_credentials: &SshCredentials, instance_ip: IpAddr, ssh_port: u16, @@ -288,7 +303,7 @@ impl ProvisionCommandHandler { let socket_addr = std::net::SocketAddr::new(instance_ip, ssh_port); RenderAnsibleTemplatesStep::new( - Arc::clone(&self.ansible_template_renderer), + ansible_template_renderer.clone(), ssh_credentials.clone(), socket_addr, ) diff --git a/src/application/command_handlers/provision/tests/builders.rs b/src/application/command_handlers/provision/tests/builders.rs index b04c8137..392ce361 100644 --- a/src/application/command_handlers/provision/tests/builders.rs +++ b/src/application/command_handlers/provision/tests/builders.rs @@ -9,9 +9,6 @@ use tempfile::TempDir; use crate::adapters::ssh::SshCredentials; use crate::application::command_handlers::provision::ProvisionCommandHandler; -use crate::domain::{InstanceName, ProfileName}; -use crate::infrastructure::external_tools::ansible::AnsibleTemplateRenderer; -use crate::infrastructure::external_tools::tofu::TofuTemplateRenderer; use crate::shared::Username; /// Test builder for `ProvisionCommandHandler` that manages dependencies and lifecycle @@ -66,20 +63,6 @@ impl ProvisionCommandHandlerTestBuilder { ) }); - let tofu_renderer = Arc::new(TofuTemplateRenderer::new( - template_manager.clone(), - self.temp_dir.path(), - ssh_credentials.clone(), - InstanceName::new("torrust-tracker-vm".to_string()) - .expect("Valid hardcoded instance name"), - ProfileName::new("default-profile".to_string()).expect("Valid hardcoded profile name"), - )); - - let ansible_renderer = Arc::new(AnsibleTemplateRenderer::new( - self.temp_dir.path(), - template_manager.clone(), - )); - let clock: Arc = Arc::new(crate::shared::SystemClock); let repository_factory = @@ -88,13 +71,7 @@ impl ProvisionCommandHandlerTestBuilder { ); let repository = repository_factory.create(self.temp_dir.path().to_path_buf()); - let command_handler = ProvisionCommandHandler::new( - tofu_renderer, - ansible_renderer, - clock, - template_manager, - repository, - ); + let command_handler = ProvisionCommandHandler::new(clock, template_manager, repository); (command_handler, self.temp_dir, ssh_credentials) } diff --git a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs index e79a5703..aff77fe5 100644 --- a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs +++ b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs @@ -147,8 +147,6 @@ pub async fn run_provision_command( // Use the new ProvisionCommandHandler to handle all infrastructure provisioning steps let provision_command_handler = ProvisionCommandHandler::new( - Arc::clone(&test_context.services.tofu_template_renderer), - Arc::clone(&test_context.services.ansible_template_renderer), Arc::clone(&test_context.services.clock), Arc::clone(&test_context.services.template_manager), repository, From 25fff8487f226efbd1797aa09b7e4690d304d7e4 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 10:31:51 +0000 Subject: [PATCH 08/19] refactor: [#174] replace Repository with RepositoryFactory in constructor Replace environment-specific Repository with global RepositoryFactory in ProvisionCommandHandler constructor. Create repository locally within execute() method using environment's data directory. This completes the separation of global vs environment-specific dependencies. Changes: - Replace repository field with repository_factory in struct - Replace repository parameter with repository_factory in constructor - Create repository locally in execute() using: repository_factory.create(environment.data_dir()) - Pass repository to all persistence operations (save_provisioning, save_provisioned, save_provision_failed) - Update test builders to inject Arc - Update E2E container to wrap RepositoryFactory in Arc - Remove repository creation from E2E test helper Constructor now only has truly global dependencies: - Clock (system time service) - TemplateManager (global template service) - RepositoryFactory (creates environment-specific repositories) All environment-specific services are created at runtime when environment context (data_dir, build_dir, ssh_credentials, etc.) is available. --- .../command_handlers/provision/handler.rs | 20 ++++++++++++------- .../provision/tests/builders.rs | 10 ++++------ src/testing/e2e/container.rs | 4 ++-- .../virtual_machine/run_provision_command.rs | 5 +---- 4 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/application/command_handlers/provision/handler.rs b/src/application/command_handlers/provision/handler.rs index 890c82f3..b2982556 100644 --- a/src/application/command_handlers/provision/handler.rs +++ b/src/application/command_handlers/provision/handler.rs @@ -16,12 +16,13 @@ use crate::application::steps::{ PlanInfrastructureStep, RenderAnsibleTemplatesStep, RenderOpenTofuTemplatesStep, ValidateInfrastructureStep, WaitForCloudInitStep, WaitForSSHConnectivityStep, }; -use crate::domain::environment::repository::{EnvironmentRepository, TypedEnvironmentRepository}; +use crate::domain::environment::repository::TypedEnvironmentRepository; use crate::domain::environment::state::{ProvisionFailureContext, ProvisionStep}; use crate::domain::environment::{Created, Environment, Provisioned, Provisioning}; use crate::domain::TemplateManager; use crate::infrastructure::external_tools::ansible::AnsibleTemplateRenderer; use crate::infrastructure::external_tools::tofu::TofuTemplateRenderer; +use crate::infrastructure::persistence::repository_factory::RepositoryFactory; use crate::shared::error::Traceable; /// `ProvisionCommandHandler` orchestrates the complete infrastructure provisioning workflow @@ -52,7 +53,7 @@ use crate::shared::error::Traceable; pub struct ProvisionCommandHandler { clock: Arc, template_manager: Arc, - repository: TypedEnvironmentRepository, + repository_factory: Arc, } impl ProvisionCommandHandler { @@ -61,12 +62,12 @@ impl ProvisionCommandHandler { pub fn new( clock: Arc, template_manager: Arc, - repository: Arc, + repository_factory: Arc, ) -> Self { Self { clock, template_manager, - repository: TypedEnvironmentRepository::new(repository), + repository_factory, } } @@ -114,8 +115,13 @@ impl ProvisionCommandHandler { // Transition to Provisioning state let environment = environment.start_provisioning(); + let repository = TypedEnvironmentRepository::new( + self.repository_factory + .create(environment.data_dir().clone()), + ); + // Persist intermediate state - self.repository.save_provisioning(&environment)?; + repository.save_provisioning(&environment)?; // Execute provisioning steps with explicit step tracking // This allows us to know exactly which step failed if an error occurs @@ -125,7 +131,7 @@ impl ProvisionCommandHandler { let provisioned = provisioned.with_instance_ip(instance_ip); // Persist final state - self.repository.save_provisioned(&provisioned)?; + repository.save_provisioned(&provisioned)?; info!( command = "provision", @@ -144,7 +150,7 @@ impl ProvisionCommandHandler { let failed = environment.provision_failed(context); // Persist error state - self.repository.save_provision_failed(&failed)?; + repository.save_provision_failed(&failed)?; Err(e) } diff --git a/src/application/command_handlers/provision/tests/builders.rs b/src/application/command_handlers/provision/tests/builders.rs index 392ce361..946541b4 100644 --- a/src/application/command_handlers/provision/tests/builders.rs +++ b/src/application/command_handlers/provision/tests/builders.rs @@ -9,6 +9,7 @@ use tempfile::TempDir; use crate::adapters::ssh::SshCredentials; use crate::application::command_handlers::provision::ProvisionCommandHandler; +use crate::infrastructure::persistence::repository_factory::RepositoryFactory; use crate::shared::Username; /// Test builder for `ProvisionCommandHandler` that manages dependencies and lifecycle @@ -65,13 +66,10 @@ impl ProvisionCommandHandlerTestBuilder { let clock: Arc = Arc::new(crate::shared::SystemClock); - let repository_factory = - crate::infrastructure::persistence::repository_factory::RepositoryFactory::new( - std::time::Duration::from_secs(30), - ); - let repository = repository_factory.create(self.temp_dir.path().to_path_buf()); + let repository_factory = RepositoryFactory::new(std::time::Duration::from_secs(30)); - let command_handler = ProvisionCommandHandler::new(clock, template_manager, repository); + let command_handler = + ProvisionCommandHandler::new(clock, template_manager, repository_factory.into()); (command_handler, self.temp_dir, ssh_credentials) } diff --git a/src/testing/e2e/container.rs b/src/testing/e2e/container.rs index cc60a39d..a57fc61a 100644 --- a/src/testing/e2e/container.rs +++ b/src/testing/e2e/container.rs @@ -60,7 +60,7 @@ pub struct Services { // Persistence layer /// Factory for creating environment-specific repositories - pub repository_factory: RepositoryFactory, + pub repository_factory: Arc, } impl Services { @@ -120,7 +120,7 @@ impl Services { clock, // Persistence layer - repository_factory, + repository_factory: Arc::new(repository_factory), } } } diff --git a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs index aff77fe5..d4580bc6 100644 --- a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs +++ b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs @@ -142,14 +142,11 @@ pub async fn run_provision_command( ) -> Result<(), ProvisionTaskError> { info!("Provisioning test infrastructure"); - // Create repository for this environment - let repository = test_context.create_repository(); - // Use the new ProvisionCommandHandler to handle all infrastructure provisioning steps let provision_command_handler = ProvisionCommandHandler::new( Arc::clone(&test_context.services.clock), Arc::clone(&test_context.services.template_manager), - repository, + Arc::clone(&test_context.services.repository_factory), ); // Execute provisioning with environment in Created state From 08a5b0b681996b2e8169661dabd61f91c9674a97 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 10:52:36 +0000 Subject: [PATCH 09/19] refactor: [#174] extract dependency building methods in ProvisionCommandHandler Extract dependency creation logic into separate methods for better organization: - Extract build_infrastructure_dependencies() from provision_infrastructure() * Creates TofuTemplateRenderer and OpenTofuClient * Returns tuple of Arc-wrapped dependencies - Extract build_configuration_dependencies() from prepare_for_configuration() * Creates AnsibleClient and AnsibleTemplateRenderer * Returns tuple of Arc-wrapped dependencies Benefits: - Clear separation between dependency setup and workflow execution - Consistent pattern across both provisioning phases - Improved readability with focused, single-responsibility methods - Each method now has distinct purpose: build deps vs execute workflow This completes the ProvisionCommandHandler refactoring series: - Reduced constructor from 7 to 3 global dependencies - Extracted workflow orchestration methods - Separated dependency creation from execution - Improved code organization and maintainability --- .../command_handlers/provision/handler.rs | 155 ++++++++++++++---- 1 file changed, 127 insertions(+), 28 deletions(-) diff --git a/src/application/command_handlers/provision/handler.rs b/src/application/command_handlers/provision/handler.rs index b2982556..60fd741f 100644 --- a/src/application/command_handlers/provision/handler.rs +++ b/src/application/command_handlers/provision/handler.rs @@ -123,9 +123,9 @@ impl ProvisionCommandHandler { // Persist intermediate state repository.save_provisioning(&environment)?; - // Execute provisioning steps with explicit step tracking + // Execute provisioning workflow with explicit step tracking // This allows us to know exactly which step failed if an error occurs - match self.execute_provisioning_with_tracking(&environment).await { + match self.execute_provisioning_workflow(&environment).await { Ok((provisioned, instance_ip)) => { // Store instance IP in the environment context let provisioned = provisioned.with_instance_ip(instance_ip); @@ -157,11 +157,15 @@ impl ProvisionCommandHandler { } } - /// Execute the provisioning steps with step tracking + /// Execute the provisioning workflow /// - /// This method executes all provisioning steps while tracking which step is currently - /// being executed. If an error occurs, it returns both the error and the step that - /// was being executed, enabling accurate failure context generation. + /// This method orchestrates the complete provisioning workflow across multiple phases: + /// 1. Infrastructure provisioning (`OpenTofu`) + /// 2. Configuration preparation (Ansible templates and system readiness) + /// 3. State transition to Provisioned + /// + /// If an error occurs, it returns both the error and the step that was being + /// executed, enabling accurate failure context generation. /// /// # Errors /// @@ -172,30 +176,47 @@ impl ProvisionCommandHandler { /// Returns a tuple of: /// - The provisioned environment /// - The instance IP address - async fn execute_provisioning_with_tracking( + async fn execute_provisioning_workflow( &self, environment: &Environment, ) -> StepResult<(Environment, IpAddr), ProvisionCommandHandlerError, ProvisionStep> { - let ssh_credentials = environment.ssh_credentials(); + let instance_ip = self.provision_infrastructure(environment).await?; - let tofu_template_renderer = Arc::new(TofuTemplateRenderer::new( - self.template_manager.clone(), - environment.build_dir(), - environment.ssh_credentials().clone(), - environment.instance_name().clone(), - environment.profile_name().clone(), - )); - let opentofu_client = Arc::new(OpenTofuClient::new(environment.tofu_build_dir())); + self.prepare_for_configuration(environment, instance_ip) + .await?; - let ansible_client = Arc::new(AnsibleClient::new(environment.ansible_build_dir())); - let ansible_template_renderer = Arc::new(AnsibleTemplateRenderer::new( - environment.build_dir(), - self.template_manager.clone(), - )); + let provisioned = environment.clone().provisioned(); + + Ok((provisioned, instance_ip)) + } - // Track current step and execute each step - // If an error occurs, we return it along with the current step + // Private helper methods - organized from higher to lower level of abstraction + + /// Provision infrastructure using `OpenTofu` + /// + /// This method handles the complete `OpenTofu`-based infrastructure provisioning: + /// - Render `OpenTofu` templates + /// - Initialize, validate, plan, and apply infrastructure + /// - Retrieve instance information + /// + /// # Arguments + /// + /// * `environment` - The environment in Provisioning state + /// + /// # Returns + /// + /// Returns the IP address of the provisioned instance + /// + /// # Errors + /// + /// Returns a tuple of (error, `current_step`) if any provisioning step fails + async fn provision_infrastructure( + &self, + environment: &Environment, + ) -> StepResult { + let (tofu_template_renderer, opentofu_client) = + self.build_infrastructure_dependencies(environment); let current_step = ProvisionStep::RenderOpenTofuTemplates; self.render_opentofu_templates(&tofu_template_renderer) @@ -210,6 +231,63 @@ impl ProvisionCommandHandler { Self::get_instance_info(&opentofu_client).map_err(|e| (e, current_step))?; let instance_ip = instance_info.ip_address; + Ok(instance_ip) + } + + /// Build dependencies for infrastructure provisioning + /// + /// Creates the template renderer and `OpenTofu` client needed for infrastructure provisioning. + /// + /// # Arguments + /// + /// * `environment` - The environment in Provisioning state + /// + /// # Returns + /// + /// Returns a tuple of: + /// - `TofuTemplateRenderer` - For rendering `OpenTofu` templates + /// - `OpenTofuClient` - For executing `OpenTofu` operations + fn build_infrastructure_dependencies( + &self, + environment: &Environment, + ) -> (Arc, Arc) { + let tofu_template_renderer = Arc::new(TofuTemplateRenderer::new( + self.template_manager.clone(), + environment.build_dir(), + environment.ssh_credentials().clone(), + environment.instance_name().clone(), + environment.profile_name().clone(), + )); + let opentofu_client = Arc::new(OpenTofuClient::new(environment.tofu_build_dir())); + + (tofu_template_renderer, opentofu_client) + } + + /// Prepare for configuration stages + /// + /// This method handles preparation for future configuration stages: + /// - Render Ansible templates with runtime instance IP + /// - Wait for SSH connectivity + /// - Wait for cloud-init completion + /// + /// # Arguments + /// + /// * `environment` - The environment in Provisioning state + /// * `instance_ip` - IP address of the provisioned instance + /// + /// # Errors + /// + /// Returns a tuple of (error, `current_step`) if any preparation step fails + async fn prepare_for_configuration( + &self, + environment: &Environment, + instance_ip: IpAddr, + ) -> StepResult<(), ProvisionCommandHandlerError, ProvisionStep> { + let (ansible_client, ansible_template_renderer) = + self.build_configuration_dependencies(environment); + + let ssh_credentials = environment.ssh_credentials(); + let current_step = ProvisionStep::RenderAnsibleTemplates; self.render_ansible_templates( &ansible_template_renderer, @@ -225,13 +303,34 @@ impl ProvisionCommandHandler { .await .map_err(|e| (e, current_step))?; - // Transition to Provisioned state - let provisioned = environment.clone().provisioned(); - - Ok((provisioned, instance_ip)) + Ok(()) } - // Private helper methods - organized from higher to lower level of abstraction + /// Build dependencies for configuration preparation + /// + /// Creates the Ansible client and template renderer needed for configuration preparation. + /// + /// # Arguments + /// + /// * `environment` - The environment in Provisioning state + /// + /// # Returns + /// + /// Returns a tuple of: + /// - `AnsibleClient` - For executing Ansible playbooks + /// - `AnsibleTemplateRenderer` - For rendering Ansible templates + fn build_configuration_dependencies( + &self, + environment: &Environment, + ) -> (Arc, Arc) { + let ansible_client = Arc::new(AnsibleClient::new(environment.ansible_build_dir())); + let ansible_template_renderer = Arc::new(AnsibleTemplateRenderer::new( + environment.build_dir(), + self.template_manager.clone(), + )); + + (ansible_client, ansible_template_renderer) + } /// Render `OpenTofu` templates /// From f03b5a447e69dad4558a1a665dd2f48438b3beb8 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 11:35:35 +0000 Subject: [PATCH 10/19] feat: [#174] implement provision console command with architectural improvements - Add provision controller following destroy pattern - Implement ExecutionContext pattern for dependency injection - Add TemplateManager to Container and ExecutionContext - Use domain's try_into_created() for state validation - Simplify imports (remove unnecessary long paths) - Follow DDD layer separation (presentation/application/domain) Architecture improvements: - TemplateManager now managed by Container (proper DI) - Removed application layer validation from presentation layer - State validation delegated to domain layer via try_into_created() - Simplified import paths where disambiguation not needed All tests passing (1141/1141) --- src/bootstrap/container.rs | 25 + src/presentation/controllers/mod.rs | 2 +- .../controllers/provision/errors.rs | 324 ++++++++++ .../controllers/provision/handler.rs | 561 ++++++++++++++++++ src/presentation/controllers/provision/mod.rs | 87 +++ .../controllers/provision/tests/mod.rs | 12 + src/presentation/dispatch/context.rs | 28 + src/presentation/dispatch/router.rs | 11 +- src/presentation/errors.rs | 15 + src/presentation/input/cli/commands.rs | 19 + src/presentation/input/cli/mod.rs | 32 +- 11 files changed, 1101 insertions(+), 15 deletions(-) create mode 100644 src/presentation/controllers/provision/errors.rs create mode 100644 src/presentation/controllers/provision/handler.rs create mode 100644 src/presentation/controllers/provision/mod.rs create mode 100644 src/presentation/controllers/provision/tests/mod.rs diff --git a/src/bootstrap/container.rs b/src/bootstrap/container.rs index a7492f25..32833c76 100644 --- a/src/bootstrap/container.rs +++ b/src/bootstrap/container.rs @@ -8,6 +8,7 @@ use std::sync::Arc; use parking_lot::ReentrantMutex; +use crate::domain::TemplateManager; use crate::infrastructure::persistence::repository_factory::RepositoryFactory; use crate::presentation::controllers::constants::DEFAULT_LOCK_TIMEOUT; use crate::presentation::views::{UserOutput, VerbosityLevel}; @@ -35,6 +36,7 @@ pub struct Container { user_output: Arc>>, repository_factory: Arc, clock: Arc, + template_manager: Arc, } impl Container { @@ -68,11 +70,14 @@ impl Container { )))); let repository_factory = Arc::new(RepositoryFactory::new(DEFAULT_LOCK_TIMEOUT)); let clock: Arc = Arc::new(SystemClock); + let template_manager = + Arc::new(TemplateManager::new(std::path::PathBuf::from("templates"))); Self { user_output, repository_factory, clock, + template_manager, } } @@ -137,6 +142,26 @@ impl Container { pub fn clock(&self) -> Arc { Arc::clone(&self.clock) } + + /// Get shared reference to template manager service + /// + /// Returns an `Arc` that can be cheaply cloned and shared + /// across threads and function calls. + /// + /// # Example + /// + /// ```rust + /// use torrust_tracker_deployer_lib::bootstrap::container::Container; + /// use torrust_tracker_deployer_lib::presentation::views::VerbosityLevel; + /// + /// let container = Container::new(VerbosityLevel::Normal); + /// let template_manager = container.template_manager(); + /// // Use template_manager for template operations + /// ``` + #[must_use] + pub fn template_manager(&self) -> Arc { + Arc::clone(&self.template_manager) + } } impl Default for Container { diff --git a/src/presentation/controllers/mod.rs b/src/presentation/controllers/mod.rs index dd37978b..b2a14769 100644 --- a/src/presentation/controllers/mod.rs +++ b/src/presentation/controllers/mod.rs @@ -231,12 +231,12 @@ pub mod constants; pub mod create; pub mod destroy; +pub mod provision; // Shared test utilities #[cfg(test)] pub mod tests; // Future command modules will be added here: -// pub mod provision; // pub mod configure; // pub mod release; diff --git a/src/presentation/controllers/provision/errors.rs b/src/presentation/controllers/provision/errors.rs new file mode 100644 index 00000000..6d36f66b --- /dev/null +++ b/src/presentation/controllers/provision/errors.rs @@ -0,0 +1,324 @@ +//! Error types for the Provision Subcommand +//! +//! This module defines error types that can occur during CLI provision command execution. +//! All errors follow the project's error handling principles by providing clear, +//! contextual, and actionable error messages with `.help()` methods. + +use thiserror::Error; + +use crate::application::command_handlers::provision::errors::ProvisionCommandHandlerError; +use crate::domain::environment::name::EnvironmentNameError; +use crate::presentation::views::progress::ProgressReporterError; + +/// Provision command specific errors +/// +/// This enum contains all error variants specific to the provision command, +/// including environment validation, repository access, and provisioning failures. +/// Each variant includes relevant context and actionable error messages. +#[derive(Debug, Error)] +pub enum ProvisionSubcommandError { + // ===== Environment Validation Errors ===== + /// Environment name validation failed + /// + /// The provided environment name doesn't meet the validation requirements. + /// Use `.help()` for detailed troubleshooting steps. + #[error("Invalid environment name '{name}': {source} +Tip: Environment names must be 1-63 characters, start with letter/digit, contain only letters/digits/hyphens")] + InvalidEnvironmentName { + name: String, + #[source] + source: EnvironmentNameError, + }, + + /// Environment not found or inaccessible + /// + /// The environment couldn't be loaded from persistent storage. + /// Use `.help()` for detailed troubleshooting steps. + #[error( + "Environment '{name}' not found or inaccessible from data directory '{data_dir}' +Tip: Check if environment exists: ls -la {data_dir}/" + )] + EnvironmentNotAccessible { name: String, data_dir: String }, + + /// Environment is not in Created state + /// + /// The environment is in the wrong state for provisioning. + /// Use `.help()` for detailed troubleshooting steps. + #[error( + "Environment '{name}' is in '{current_state}' state, but 'Created' state is required for provisioning +Tip: Only environments in 'Created' state can be provisioned" + )] + InvalidEnvironmentState { name: String, current_state: String }, + + // ===== Repository Access Errors ===== + /// Repository operation failed + /// + /// Failed to create or access the environment repository. + /// Use `.help()` for detailed troubleshooting steps. + #[error( + "Failed to access environment repository at '{data_dir}': {reason} +Tip: Check directory permissions and disk space" + )] + RepositoryAccessFailed { data_dir: String, reason: String }, + + // ===== Provision Operation Errors ===== + /// Provision operation failed + /// + /// The provisioning process encountered an error during execution. + /// Use `.help()` for detailed troubleshooting steps. + #[error( + "Failed to provision environment '{name}': {source} +Tip: Check logs and try running with --log-output file-and-stderr for more details" + )] + ProvisionOperationFailed { + name: String, + #[source] + source: ProvisionCommandHandlerError, + }, + + // ===== Internal Errors ===== + /// Progress reporting failed + /// + /// Failed to report progress to the user due to an internal error. + /// This indicates a critical internal error. + #[error( + "Failed to report progress: {source} +Tip: This is a critical bug - please report it with full logs using --log-output file-and-stderr" + )] + ProgressReportingFailed { + #[source] + source: ProgressReporterError, + }, +} + +// ============================================================================ +// ERROR CONVERSIONS +// ============================================================================ + +impl From for ProvisionSubcommandError { + fn from(source: ProgressReporterError) -> Self { + Self::ProgressReportingFailed { source } + } +} + +impl ProvisionSubcommandError { + /// Get detailed troubleshooting guidance for this error + /// + /// This method provides comprehensive troubleshooting steps that can be + /// displayed to users when they need more help resolving the error. + /// + /// # Example + /// + /// Using with Container and `ExecutionContext` (recommended): + /// + /// ```rust + /// use std::path::Path; + /// use std::sync::Arc; + /// use torrust_tracker_deployer_lib::bootstrap::Container; + /// use torrust_tracker_deployer_lib::presentation::dispatch::ExecutionContext; + /// use torrust_tracker_deployer_lib::presentation::controllers::provision; + /// use torrust_tracker_deployer_lib::presentation::views::VerbosityLevel; + /// + /// let container = Container::new(VerbosityLevel::Normal); + /// let context = ExecutionContext::new(Arc::new(container)); + /// + /// if let Err(e) = provision::handle("test-env", Path::new("."), &context) { + /// eprintln!("Error: {e}"); + /// eprintln!("\nTroubleshooting:\n{}", e.help()); + /// } + /// ``` + /// + /// Direct usage (for testing): + /// + /// ```rust + /// use std::path::{Path, PathBuf}; + /// use std::sync::Arc; + /// use std::time::Duration; + /// use parking_lot::ReentrantMutex; + /// use std::cell::RefCell; + /// use torrust_tracker_deployer_lib::presentation::controllers::provision; + /// use torrust_tracker_deployer_lib::presentation::views::{UserOutput, VerbosityLevel}; + /// use torrust_tracker_deployer_lib::infrastructure::persistence::repository_factory::RepositoryFactory; + /// use torrust_tracker_deployer_lib::shared::clock::SystemClock; + /// use torrust_tracker_deployer_lib::domain::TemplateManager; + /// + /// let output = Arc::new(ReentrantMutex::new(RefCell::new(UserOutput::new(VerbosityLevel::Normal)))); + /// let repository_factory = Arc::new(RepositoryFactory::new(Duration::from_secs(30))); + /// let clock = Arc::new(SystemClock); + /// let template_manager = Arc::new(TemplateManager::new(PathBuf::from("templates"))); + /// if let Err(e) = provision::handle_provision_command("test-env", Path::new("."), repository_factory, clock, template_manager, &output) { + /// eprintln!("Error: {e}"); + /// eprintln!("\nTroubleshooting:\n{}", e.help()); + /// } + /// ``` + #[must_use] + #[allow(clippy::too_many_lines)] // Help text is comprehensive for user guidance + pub fn help(&self) -> &'static str { + match self { + Self::InvalidEnvironmentName { .. } => { + "Invalid Environment Name - Detailed Troubleshooting: + +1. Check environment name format: + - Length: Must be 1-63 characters + - Start: Must begin with a letter or digit + - Characters: Only letters, digits, and hyphens allowed + - No special characters: Avoid spaces, underscores, dots + +2. Valid examples: + - 'production' + - 'staging-01' + - 'dev-environment' + +3. Invalid examples: + - 'prod_01' (underscore not allowed) + - '-production' (cannot start with hyphen) + - 'prod.env' (dots not allowed) + +For more information, see environment naming documentation." + } + + Self::EnvironmentNotAccessible { .. } => { + "Environment Not Accessible - Detailed Troubleshooting: + +1. Verify environment exists: + - List environments: ls -la data/ + - Check for environment.json file in data// + +2. Check file permissions: + - Read permission: chmod +r data//environment.json + - Directory access: chmod +rx data// + +3. Verify data directory: + - Ensure data/ directory exists + - Check disk space: df -h + - Verify no file locks: lsof data/ + +4. Common solutions: + - Create environment first: torrust-tracker-deployer create environment -f + - Check working directory: pwd + - Verify correct environment name + +For more information, see the create command documentation." + } + + Self::InvalidEnvironmentState { .. } => { + "Invalid Environment State - Detailed Troubleshooting: + +The provision command requires an environment in 'Created' state. + +1. Check current state: + - View environment file: cat data//environment.json + - Look for 'state' field + +2. State transitions: + - Created → Provisioning → Provisioned (success path) + - Created → Provisioning → ProvisionFailed (error path) + +3. If environment is already Provisioned: + - Environment is ready to use + - No need to provision again + - Proceed to next deployment steps + +4. If environment is in ProvisionFailed state: + - Review error logs to understand failure + - Fix underlying issues (network, permissions, etc.) + - Create a new environment and try again + +5. If environment is in unexpected state: + - Consider creating a new environment + - Report issue if state seems invalid + +For more information, see environment lifecycle documentation." + } + + Self::RepositoryAccessFailed { .. } => { + "Repository Access Failed - Detailed Troubleshooting: + +1. Check directory permissions: + - Read/write access: ls -la data/ + - Fix permissions: chmod -R u+rw data/ + +2. Verify disk space: + - Check available space: df -h + - Free up space if needed + +3. Check for file locks: + - List open files: lsof data/ + - Kill processes if safe to do so + +4. Verify directory exists: + - Create if missing: mkdir -p data/ + - Check parent directory permissions + +5. Common issues: + - Running multiple instances simultaneously + - File system mounted read-only + - Insufficient permissions + +For persistent issues, check system logs and file system health." + } + + Self::ProvisionOperationFailed { .. } => { + "Provision Operation Failed - Detailed Troubleshooting: + +1. Review detailed error logs: + - Run with verbose output: --log-output file-and-stderr + - Check log files in data/logs/ + +2. Common failure points: + - OpenTofu initialization or apply failures + - LXD/VM provisioning issues + - SSH connectivity problems + - Cloud-init timeout or failures + +3. Infrastructure-specific troubleshooting: + + OpenTofu/LXD issues: + - Check LXD status: lxc list + - Verify LXD daemon: systemctl status lxd + - Review OpenTofu state: cd build/tofu/lxd && tofu state list + + SSH connectivity: + - Verify SSH keys exist: ls -la ~/.ssh/ + - Check VM is running: lxc list + - Test SSH manually: ssh -i @ + + Cloud-init: + - Check cloud-init status: lxc exec -- cloud-init status + - View cloud-init logs: lxc exec -- cat /var/log/cloud-init.log + +4. Recovery steps: + - Review error messages and logs + - Fix underlying issues + - Destroy failed environment: torrust-tracker-deployer destroy + - Create new environment and retry + +For more information, see the provisioning troubleshooting guide." + } + + Self::ProgressReportingFailed { .. } => { + "Progress Reporting Failed - Critical Internal Error: + +This is a critical internal error that should not occur during normal operation. + +1. Immediate actions: + - Save full error output + - Copy log files from data/logs/ + - Note the exact command that was running + +2. Report the issue: + - Create GitHub issue with full details + - Include: command, error output, logs, system info + - Describe steps to reproduce + +3. Temporary workarounds: + - Try running command again + - Restart application + - Check for system resource issues (memory, file descriptors) + +This error indicates a bug in the progress reporting system. +Please report it so we can fix it." + } + } + } +} diff --git a/src/presentation/controllers/provision/handler.rs b/src/presentation/controllers/provision/handler.rs new file mode 100644 index 00000000..63ae14d3 --- /dev/null +++ b/src/presentation/controllers/provision/handler.rs @@ -0,0 +1,561 @@ +//! Provision Command Handler +//! +//! This module handles the provision command execution at the presentation layer, +//! including environment validation, repository initialization, and user interaction. + +use std::cell::RefCell; +use std::sync::Arc; + +use parking_lot::ReentrantMutex; + +use crate::application::command_handlers::ProvisionCommandHandler; +use crate::domain::environment::name::EnvironmentName; +use crate::domain::environment::repository::EnvironmentRepository; +use crate::domain::environment::state::Provisioned; +use crate::domain::environment::Environment; +use crate::domain::TemplateManager; +use crate::infrastructure::persistence::repository_factory::RepositoryFactory; +use crate::presentation::dispatch::context::ExecutionContext; +use crate::presentation::views::progress::ProgressReporter; +use crate::presentation::views::UserOutput; +use crate::shared::clock::Clock; + +use super::errors::ProvisionSubcommandError; + +// ============================================================================ +// CONSTANTS +// ============================================================================ + +/// Number of main steps in the provision workflow +const PROVISION_WORKFLOW_STEPS: usize = 9; + +// ============================================================================ +// HIGH-LEVEL API (EXECUTION CONTEXT PATTERN) +// ============================================================================ + +/// Handle provision command using `ExecutionContext` pattern +/// +/// This function provides a clean interface for provisioning deployment environments, +/// integrating with the `ExecutionContext` pattern for dependency injection. +/// +/// # Arguments +/// +/// * `environment_name` - Name of the environment to provision +/// * `working_dir` - Working directory path for operations +/// * `context` - Execution context providing access to services +/// +/// # Returns +/// +/// * `Ok(Environment)` - Environment provisioned successfully +/// * `Err(ProvisionSubcommandError)` - Provision operation failed +/// +/// # Errors +/// +/// Returns `ProvisionSubcommandError` when: +/// * Environment name is invalid or contains special characters +/// * Working directory is not accessible or doesn't exist +/// * Environment is not found or not in "Created" state +/// * Infrastructure provisioning fails (OpenTofu/LXD errors) +/// * SSH connectivity cannot be established +/// * Cloud-init does not complete successfully +/// +/// # Examples +/// +/// ```rust +/// use std::path::Path; +/// use std::sync::Arc; +/// use torrust_tracker_deployer_lib::presentation::controllers::provision; +/// use torrust_tracker_deployer_lib::presentation::dispatch::context::ExecutionContext; +/// use torrust_tracker_deployer_lib::bootstrap::container::Container; +/// use torrust_tracker_deployer_lib::presentation::views::VerbosityLevel; +/// +/// # async fn example() -> Result<(), Box> { +/// let container = Arc::new(Container::new(VerbosityLevel::Normal)); +/// let context = ExecutionContext::new(container); +/// let working_dir = Path::new("./test"); +/// +/// provision::handle("my-env", working_dir, &context)?; +/// # Ok(()) +/// # } +/// ``` +#[allow(clippy::result_large_err)] // Error contains detailed context for user guidance +pub fn handle( + environment_name: &str, + working_dir: &std::path::Path, + context: &ExecutionContext, +) -> Result, ProvisionSubcommandError> { + handle_provision_command( + environment_name, + working_dir, + context.repository_factory(), + context.clock(), + context.template_manager(), + &context.user_output(), + ) +} + +// ============================================================================ +// INTERMEDIATE API (DIRECT DEPENDENCY INJECTION) +// ============================================================================ + +/// Handle the provision command +/// +/// This is a thin wrapper over `ProvisionCommandController` that serves as +/// the public entry point for the provision command. +/// +/// # Arguments +/// +/// * `environment_name` - The name of the environment to provision +/// * `working_dir` - Root directory for environment data storage +/// * `repository_factory` - Factory for creating environment repositories +/// * `clock` - Clock service for timing operations +/// * `template_manager` - Template manager for rendering configuration files +/// * `user_output` - Shared user output service for consistent output formatting +/// +/// # Errors +/// +/// Returns an error if: +/// - Environment name is invalid (format validation fails) +/// - Environment cannot be loaded from repository +/// - Environment is not in "Created" state +/// - Infrastructure provisioning fails +/// - Progress reporting encounters a poisoned mutex +/// +/// All errors include detailed context and actionable troubleshooting guidance. +/// +/// # Returns +/// +/// Returns `Ok(Environment)` on success, or a `ProvisionSubcommandError` on failure. +/// +/// # Example +/// +/// Using with Container and `ExecutionContext` (recommended): +/// +/// ```rust +/// use std::path::Path; +/// use std::sync::Arc; +/// use torrust_tracker_deployer_lib::bootstrap::Container; +/// use torrust_tracker_deployer_lib::presentation::dispatch::ExecutionContext; +/// use torrust_tracker_deployer_lib::presentation::controllers::provision; +/// use torrust_tracker_deployer_lib::presentation::views::VerbosityLevel; +/// +/// let container = Container::new(VerbosityLevel::Normal); +/// let context = ExecutionContext::new(Arc::new(container)); +/// +/// if let Err(e) = provision::handle("test-env", Path::new("."), &context) { +/// eprintln!("Provision failed: {e}"); +/// eprintln!("Help: {}", e.help()); +/// } +/// ``` +/// +/// Direct usage (for testing or specialized scenarios): +/// +/// ```rust +/// use std::path::{Path, PathBuf}; +/// use std::sync::Arc; +/// use parking_lot::ReentrantMutex; +/// use std::cell::RefCell; +/// use torrust_tracker_deployer_lib::presentation::controllers::provision; +/// use torrust_tracker_deployer_lib::presentation::views::{UserOutput, VerbosityLevel}; +/// use torrust_tracker_deployer_lib::infrastructure::persistence::repository_factory::RepositoryFactory; +/// use torrust_tracker_deployer_lib::presentation::controllers::constants::DEFAULT_LOCK_TIMEOUT; +/// use torrust_tracker_deployer_lib::shared::SystemClock; +/// use torrust_tracker_deployer_lib::domain::TemplateManager; +/// +/// let user_output = Arc::new(ReentrantMutex::new(RefCell::new(UserOutput::new(VerbosityLevel::Normal)))); +/// let repository_factory = Arc::new(RepositoryFactory::new(DEFAULT_LOCK_TIMEOUT)); +/// let clock = Arc::new(SystemClock); +/// let template_manager = Arc::new(TemplateManager::new(PathBuf::from("templates"))); +/// if let Err(e) = provision::handle_provision_command("test-env", Path::new("."), repository_factory, clock, template_manager, &user_output) { +/// eprintln!("Provision failed: {e}"); +/// eprintln!("Help: {}", e.help()); +/// } +/// ``` +#[allow(clippy::result_large_err)] // Error contains detailed context for user guidance +#[allow(clippy::needless_pass_by_value)] // Arc parameters are moved to constructor for ownership +pub fn handle_provision_command( + environment_name: &str, + working_dir: &std::path::Path, + repository_factory: Arc, + clock: Arc, + template_manager: Arc, + user_output: &Arc>>, +) -> Result, ProvisionSubcommandError> { + ProvisionCommandController::new( + working_dir.to_path_buf(), + repository_factory, + clock, + template_manager, + user_output.clone(), + ) + .execute(environment_name) +} + +// ============================================================================ +// PRESENTATION LAYER CONTROLLER (IMPLEMENTATION DETAILS) +// ============================================================================ + +/// Presentation layer controller for provision command workflow +/// +/// Coordinates user interaction, progress reporting, and input validation +/// before delegating to the application layer `ProvisionCommandHandler`. +/// +/// # Responsibilities +/// +/// - Validate user input (environment name format) +/// - Show progress updates to the user +/// - Format success/error messages for display +/// - Delegate business logic to application layer +/// +/// # Architecture +/// +/// This controller sits in the presentation layer and handles all user-facing +/// concerns. It delegates actual business logic to the application layer's +/// `ProvisionCommandHandler`, maintaining clear separation of concerns. +#[allow(unused)] // Temporary during refactoring +pub struct ProvisionCommandController { + repository: Arc, + repository_factory: Arc, + clock: Arc, + template_manager: Arc, + user_output: Arc>>, + progress: ProgressReporter, +} + +impl ProvisionCommandController { + /// Create a new provision command controller from working directory + /// + /// Creates a `ProvisionCommandController` with direct service injection from working directory and user output. + /// This follows the single container architecture pattern. + #[allow(clippy::needless_pass_by_value)] // Constructor takes ownership of Arc parameters + pub fn new( + working_dir: std::path::PathBuf, + repository_factory: Arc, + clock: Arc, + template_manager: Arc, + user_output: Arc>>, + ) -> Self { + let repository = repository_factory.create(working_dir); + let progress = ProgressReporter::new(user_output.clone(), PROVISION_WORKFLOW_STEPS); + + Self { + repository, + repository_factory, + clock, + template_manager, + user_output, + progress, + } + } + + /// Execute the complete provision workflow + /// + /// Orchestrates all steps of the provision command: + /// 1. Validate environment name + /// 2. Load and validate environment state + /// 3. Create command handler + /// 4. Provision infrastructure + /// 5. Complete with success message + /// + /// # Arguments + /// + /// * `environment_name` - The name of the environment to provision + /// + /// # Errors + /// + /// Returns an error if: + /// - Environment name is invalid (format validation fails) + /// - Environment cannot be loaded from repository + /// - Environment is not in "Created" state + /// - Infrastructure provisioning fails + /// - Progress reporting encounters a poisoned mutex + /// + /// # Returns + /// + /// Returns `Ok(Environment)` on success, or a `ProvisionSubcommandError` if any step fails. + #[allow(clippy::result_large_err)] + pub fn execute( + &mut self, + environment_name: &str, + ) -> Result, ProvisionSubcommandError> { + let env_name = self.validate_environment_name(environment_name)?; + + let handler = self.create_command_handler()?; + + let provisioned = self.provision_infrastructure(&handler, &env_name)?; + + self.complete_workflow(environment_name)?; + + Ok(provisioned) + } + + /// Validate the environment name format + /// + /// Shows progress to user and validates that the environment name + /// meets domain requirements (1-63 chars, alphanumeric + hyphens). + #[allow(clippy::result_large_err)] + fn validate_environment_name( + &mut self, + name: &str, + ) -> Result { + self.progress.start_step("Validating environment")?; + + let env_name = EnvironmentName::new(name.to_string()).map_err(|source| { + ProvisionSubcommandError::InvalidEnvironmentName { + name: name.to_string(), + source, + } + })?; + + self.progress + .complete_step(Some(&format!("Environment name validated: {name}")))?; + + Ok(env_name) + } + + /// Create application layer command handler + /// + /// Creates the application layer command handler with all required + /// dependencies (repository factory, clock, template manager, etc.). + #[allow(clippy::result_large_err)] + fn create_command_handler( + &mut self, + ) -> Result { + self.progress.start_step("Creating command handler")?; + let handler = ProvisionCommandHandler::new( + self.clock.clone(), + self.template_manager.clone(), + self.repository_factory.clone(), + ); + self.progress.complete_step(None)?; + + Ok(handler) + } + + /// Execute infrastructure provisioning via application layer + /// + /// Delegates to the application layer `ProvisionCommandHandler` to + /// orchestrate the actual infrastructure provisioning workflow. + /// + /// This method: + /// 1. Loads the environment from repository + /// 2. Calls the application handler (which validates state internally) + #[allow(clippy::result_large_err)] + fn provision_infrastructure( + &mut self, + handler: &ProvisionCommandHandler, + env_name: &EnvironmentName, + ) -> Result, ProvisionSubcommandError> { + self.progress + .start_step("Loading environment from repository")?; + + // Load environment from repository + let environment = self.repository.load(env_name).map_err(|e| { + ProvisionSubcommandError::RepositoryAccessFailed { + data_dir: env_name.to_string(), + reason: e.to_string(), + } + })?; + + // Check if environment exists + let environment = + environment.ok_or_else(|| ProvisionSubcommandError::EnvironmentNotAccessible { + name: env_name.to_string(), + data_dir: "data".to_string(), + })?; + + // Convert to Created state - domain layer handles state validation + let environment = environment.try_into_created().map_err(|e| { + ProvisionSubcommandError::InvalidEnvironmentState { + name: env_name.to_string(), + current_state: e.to_string(), + } + })?; + + self.progress + .complete_step(Some("Environment loaded and validated"))?; + + self.progress.start_step("Provisioning infrastructure")?; + + // Use tokio runtime to execute async handler + let runtime = tokio::runtime::Runtime::new().map_err(|e| { + ProvisionSubcommandError::RepositoryAccessFailed { + data_dir: "runtime".to_string(), + reason: format!("Failed to create tokio runtime: {e}"), + } + })?; + + let provisioned = runtime + .block_on(handler.execute(environment)) + .map_err( + |source| ProvisionSubcommandError::ProvisionOperationFailed { + name: env_name.to_string(), + source, + }, + )?; + + self.progress + .complete_step(Some("Infrastructure provisioned"))?; + Ok(provisioned) + } + + /// Complete the workflow with success message + /// + /// Shows final success message to the user with workflow summary. + #[allow(clippy::result_large_err)] + fn complete_workflow(&mut self, name: &str) -> Result<(), ProvisionSubcommandError> { + self.progress + .complete(&format!("Environment '{name}' provisioned successfully"))?; + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::presentation::controllers::constants::DEFAULT_LOCK_TIMEOUT; + use crate::presentation::views::testing::TestUserOutput; + use crate::presentation::views::VerbosityLevel; + use crate::shared::SystemClock; + + /// Create test dependencies for provision command handler tests + /// + /// Returns the common dependencies needed for testing `handle_provision_command`: + /// - `user_output`: `ReentrantMutex`-wrapped `UserOutput` for thread-safe access + /// - `repository_factory`: Factory for creating environment repositories + /// - `clock`: System clock for timing operations + /// - `template_manager`: Template manager for rendering configuration files + #[allow(clippy::type_complexity)] // Test helper with complex but clear types + fn create_test_dependencies() -> ( + Arc>>, + Arc, + Arc, + Arc, + ) { + use tempfile::TempDir; + + let (user_output, _, _) = + TestUserOutput::new(VerbosityLevel::Normal).into_reentrant_wrapped(); + let repository_factory = Arc::new(RepositoryFactory::new(DEFAULT_LOCK_TIMEOUT)); + let clock = Arc::new(SystemClock); + + // Create a temporary templates directory for testing + let temp_dir = TempDir::new().unwrap(); + let template_manager = Arc::new(TemplateManager::new(temp_dir.path())); + + (user_output, repository_factory, clock, template_manager) + } + + #[test] + fn it_should_return_error_for_invalid_environment_name() { + use tempfile::TempDir; + + let temp_dir = TempDir::new().unwrap(); + + let (user_output, repository_factory, clock, template_manager) = create_test_dependencies(); + + // Test with invalid environment name (contains underscore) + let result = handle_provision_command( + "invalid_name", + temp_dir.path(), + repository_factory, + clock, + template_manager, + &user_output, + ); + + assert!(result.is_err()); + match result.unwrap_err() { + ProvisionSubcommandError::InvalidEnvironmentName { name, .. } => { + assert_eq!(name, "invalid_name"); + } + other => panic!("Expected InvalidEnvironmentName, got: {other:?}"), + } + } + + #[test] + fn it_should_return_error_for_empty_environment_name() { + use tempfile::TempDir; + + let temp_dir = TempDir::new().unwrap(); + + let (user_output, repository_factory, clock, template_manager) = create_test_dependencies(); + + let result = handle_provision_command( + "", + temp_dir.path(), + repository_factory, + clock, + template_manager, + &user_output, + ); + + assert!(result.is_err()); + match result.unwrap_err() { + ProvisionSubcommandError::InvalidEnvironmentName { name, .. } => { + assert_eq!(name, ""); + } + other => panic!("Expected InvalidEnvironmentName, got: {other:?}"), + } + } + + #[test] + fn it_should_return_error_for_nonexistent_environment() { + use tempfile::TempDir; + + let temp_dir = TempDir::new().unwrap(); + + let (user_output, repository_factory, clock, template_manager) = create_test_dependencies(); + + // Try to provision an environment that doesn't exist + let result = handle_provision_command( + "nonexistent-env", + temp_dir.path(), + repository_factory, + clock, + template_manager, + &user_output, + ); + + assert!(result.is_err()); + // Should get EnvironmentNotAccessible because environment doesn't exist + match result.unwrap_err() { + ProvisionSubcommandError::EnvironmentNotAccessible { name, .. } => { + assert_eq!(name, "nonexistent-env"); + } + other => panic!("Expected EnvironmentNotAccessible, got: {other:?}"), + } + } + + #[test] + fn it_should_accept_valid_environment_name() { + use std::fs; + use tempfile::TempDir; + + let temp_dir = TempDir::new().unwrap(); + let working_dir = temp_dir.path(); + + let (user_output, repository_factory, clock, template_manager) = create_test_dependencies(); + + // Create a mock environment directory to test validation + let env_dir = working_dir.join("test-env"); + fs::create_dir_all(&env_dir).unwrap(); + + // Valid environment name should pass validation, but will fail + // at provision operation since we don't have a real environment setup + let result = handle_provision_command( + "test-env", + working_dir, + repository_factory, + clock, + template_manager, + &user_output, + ); + + // Should fail at operation, not at name validation + if let Err(ProvisionSubcommandError::InvalidEnvironmentName { .. }) = result { + panic!("Should not fail at name validation for 'test-env'"); + } + // Expected - valid name but operation fails or other errors acceptable in test context + } +} diff --git a/src/presentation/controllers/provision/mod.rs b/src/presentation/controllers/provision/mod.rs new file mode 100644 index 00000000..52ce4c50 --- /dev/null +++ b/src/presentation/controllers/provision/mod.rs @@ -0,0 +1,87 @@ +//! Provision Command Presentation Module +//! +//! This module implements the CLI presentation layer for the provision command, +//! handling argument processing and user interaction. +//! +//! ## Architecture +//! +//! The provision command presentation layer follows the DDD pattern, orchestrating +//! the application layer's `ProvisionCommandHandler` while providing user-friendly +//! output and error handling. +//! +//! ## Components +//! +//! - `errors` - Presentation layer error types with `.help()` methods +//! - `handler` - Main command handler orchestrating the workflow +//! +//! ## Usage Example +//! +//! ### Basic Usage +//! +//! ```rust +//! use std::path::Path; +//! use std::sync::Arc; +//! use torrust_tracker_deployer_lib::bootstrap::Container; +//! use torrust_tracker_deployer_lib::presentation::dispatch::ExecutionContext; +//! use torrust_tracker_deployer_lib::presentation::controllers::provision; +//! use torrust_tracker_deployer_lib::presentation::views::VerbosityLevel; +//! +//! let container = Container::new(VerbosityLevel::Normal); +//! let context = ExecutionContext::new(Arc::new(container)); +//! +//! // Call the provision handler +//! let result = provision::handler::handle("my-environment", Path::new("."), &context); +//! ``` +//! +//! ### Direct Usage (For Testing) +//! +//! ```rust +//! use std::path::Path; +//! use std::sync::Arc; +//! use torrust_tracker_deployer_lib::bootstrap::Container; +//! use torrust_tracker_deployer_lib::presentation::dispatch::ExecutionContext; +//! use torrust_tracker_deployer_lib::presentation::controllers::provision; +//! use torrust_tracker_deployer_lib::presentation::views::VerbosityLevel; +//! +//! let container = Container::new(VerbosityLevel::Normal); +//! let context = ExecutionContext::new(Arc::new(container)); +//! +//! if let Err(e) = provision::handle("test-env", Path::new("."), &context) { +//! eprintln!("Provision failed: {e}"); +//! eprintln!("\n{}", e.help()); +//! } +//! ``` +//! +//! ## Direct Usage (For Testing) +//! +//! ```rust +//! use std::path::{Path, PathBuf}; +//! use std::sync::Arc; +//! use std::time::Duration; +//! use parking_lot::ReentrantMutex; +//! use std::cell::RefCell; +//! use torrust_tracker_deployer_lib::presentation::controllers::provision; +//! use torrust_tracker_deployer_lib::presentation::views::{UserOutput, VerbosityLevel}; +//! use torrust_tracker_deployer_lib::infrastructure::persistence::repository_factory::RepositoryFactory; +//! use torrust_tracker_deployer_lib::shared::clock::SystemClock; +//! use torrust_tracker_deployer_lib::domain::TemplateManager; +//! +//! let output = Arc::new(ReentrantMutex::new(RefCell::new(UserOutput::new(VerbosityLevel::Normal)))); +//! let repository_factory = Arc::new(RepositoryFactory::new(Duration::from_secs(30))); +//! let clock = Arc::new(SystemClock); +//! let template_manager = Arc::new(TemplateManager::new(PathBuf::from("templates"))); +//! if let Err(e) = provision::handle_provision_command("test-env", Path::new("."), repository_factory, clock, template_manager, &output) { +//! eprintln!("Provision failed: {e}"); +//! eprintln!("\n{}", e.help()); +//! } +//! ``` + +pub mod errors; +pub mod handler; + +#[cfg(test)] +mod tests; + +// Re-export commonly used types for convenience +pub use errors::ProvisionSubcommandError; +pub use handler::{handle, handle_provision_command}; diff --git a/src/presentation/controllers/provision/tests/mod.rs b/src/presentation/controllers/provision/tests/mod.rs new file mode 100644 index 00000000..7ffa6197 --- /dev/null +++ b/src/presentation/controllers/provision/tests/mod.rs @@ -0,0 +1,12 @@ +//! Tests for the Provision Command Controller +//! +//! This module contains tests for the provision command presentation layer, +//! verifying input validation, error handling, and user interaction. + +// Test module structure for future implementation +// Tests will verify: +// - Environment name validation +// - Environment state validation +// - Repository access handling +// - Progress reporting +// - Error messages and help text diff --git a/src/presentation/dispatch/context.rs b/src/presentation/dispatch/context.rs index cfb718e5..1e5dc2e4 100644 --- a/src/presentation/dispatch/context.rs +++ b/src/presentation/dispatch/context.rs @@ -49,6 +49,7 @@ use std::sync::Arc; use parking_lot::ReentrantMutex; use crate::bootstrap::Container; +use crate::domain::TemplateManager; use crate::infrastructure::persistence::repository_factory::RepositoryFactory; use crate::presentation::views::UserOutput; use crate::shared::clock::Clock; @@ -213,4 +214,31 @@ impl ExecutionContext { pub fn clock(&self) -> Arc { self.container.clock() } + + /// Get shared reference to template manager service + /// + /// Returns the template manager service for template operations. + /// The service is wrapped in `Arc` for shared access. + /// + /// # Examples + /// + /// ```rust,no_run + /// use torrust_tracker_deployer_lib::bootstrap::Container; + /// use torrust_tracker_deployer_lib::presentation::views::VerbosityLevel; + /// use torrust_tracker_deployer_lib::presentation::dispatch::ExecutionContext; + /// use std::sync::Arc; + /// + /// # fn example() -> Result<(), Box> { + /// let container = Container::new(VerbosityLevel::Normal); + /// let context = ExecutionContext::new(Arc::new(container)); + /// + /// let template_manager = context.template_manager(); + /// // Use template_manager for template operations + /// # Ok(()) + /// # } + /// ``` + #[must_use] + pub fn template_manager(&self) -> Arc { + self.container.template_manager() + } } diff --git a/src/presentation/dispatch/router.rs b/src/presentation/dispatch/router.rs index 8dd48c8b..7bc6c90a 100644 --- a/src/presentation/dispatch/router.rs +++ b/src/presentation/dispatch/router.rs @@ -52,7 +52,7 @@ use std::path::Path; -use crate::presentation::controllers::{create, destroy}; +use crate::presentation::controllers::{create, destroy, provision}; use crate::presentation::errors::CommandError; use crate::presentation::input::Commands; @@ -116,12 +116,11 @@ pub fn route_command( Commands::Destroy { environment } => { destroy::handle(&environment, working_dir, context)?; Ok(()) + } + Commands::Provision { environment } => { + provision::handle(&environment, working_dir, context)?; + Ok(()) } // Future commands will be added here as the Controller Layer expands: - // - // Commands::Provision { environment, provider } => { - // provision::handle_provision_command(&environment, &provider, context)?; - // Ok(()) - // } // // Commands::Configure { environment } => { // configure::handle_configure_command(&environment, context)?; diff --git a/src/presentation/errors.rs b/src/presentation/errors.rs index cc41c48d..e2e8324b 100644 --- a/src/presentation/errors.rs +++ b/src/presentation/errors.rs @@ -21,6 +21,7 @@ use thiserror::Error; use crate::presentation::controllers::{ create::CreateCommandError, destroy::DestroySubcommandError, + provision::ProvisionSubcommandError, }; /// Errors that can occur during CLI command execution @@ -44,6 +45,13 @@ pub enum CommandError { #[error("Destroy command failed: {0}")] Destroy(Box), + /// Provision command specific errors + /// + /// Encapsulates all errors that can occur during infrastructure provisioning. + /// Use `.help()` for detailed troubleshooting steps. + #[error("Provision command failed: {0}")] + Provision(Box), + /// User output lock acquisition failed /// /// Failed to acquire the mutex lock for user output. This typically indicates @@ -64,6 +72,12 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(error: ProvisionSubcommandError) -> Self { + Self::Provision(Box::new(error)) + } +} + impl CommandError { /// Get detailed troubleshooting guidance for this error /// @@ -102,6 +116,7 @@ impl CommandError { match self { Self::Create(e) => e.help(), Self::Destroy(e) => e.help(), + Self::Provision(e) => e.help(), Self::UserOutputLockFailed => { "User Output Lock Failed - Detailed Troubleshooting: diff --git a/src/presentation/input/cli/commands.rs b/src/presentation/input/cli/commands.rs index ba8b8d5b..a2d7e3d8 100644 --- a/src/presentation/input/cli/commands.rs +++ b/src/presentation/input/cli/commands.rs @@ -34,6 +34,25 @@ pub enum Commands { /// created through the provision command. environment: String, }, + + /// Provision a new deployment environment infrastructure + /// + /// This command provisions the virtual machine infrastructure for a deployment + /// environment that was previously created. It will: + /// - Render and apply `OpenTofu` templates + /// - Create LXD VM instances + /// - Configure networking + /// - Wait for SSH connectivity + /// - Wait for cloud-init completion + /// + /// The environment must be in "Created" state (use 'create environment' first). + Provision { + /// Name of the environment to provision + /// + /// The environment name must match an existing environment that was + /// previously created and is in "Created" state. + environment: String, + }, // Future commands will be added here: // // /// Provision a new deployment environment diff --git a/src/presentation/input/cli/mod.rs b/src/presentation/input/cli/mod.rs index ff3cc80d..9f1a09e6 100644 --- a/src/presentation/input/cli/mod.rs +++ b/src/presentation/input/cli/mod.rs @@ -47,7 +47,9 @@ mod tests { Commands::Destroy { environment } => { assert_eq!(environment, "test-env"); } - Commands::Create { .. } => panic!("Expected Destroy command"), + Commands::Create { .. } | Commands::Provision { .. } => { + panic!("Expected Destroy command") + } } } @@ -63,7 +65,9 @@ mod tests { Commands::Destroy { environment } => { assert_eq!(environment, env_name); } - Commands::Create { .. } => panic!("Expected Destroy command"), + Commands::Create { .. } | Commands::Provision { .. } => { + panic!("Expected Destroy command") + } } } } @@ -104,7 +108,9 @@ mod tests { Commands::Destroy { environment } => { assert_eq!(environment, "test-env"); } - Commands::Create { .. } => panic!("Expected Destroy command"), + Commands::Create { .. } | Commands::Provision { .. } => { + panic!("Expected Destroy command") + } } // Log options are set but we don't compare them as they don't implement PartialEq @@ -188,7 +194,9 @@ mod tests { panic!("Expected Environment action") } }, - Commands::Destroy { .. } => panic!("Expected Create command"), + Commands::Destroy { .. } | Commands::Provision { .. } => { + panic!("Expected Create command") + } } } @@ -212,7 +220,9 @@ mod tests { panic!("Expected Environment action") } }, - Commands::Destroy { .. } => panic!("Expected Create command"), + Commands::Destroy { .. } | Commands::Provision { .. } => { + panic!("Expected Create command") + } } } @@ -257,7 +267,9 @@ mod tests { panic!("Expected Environment action") } }, - Commands::Destroy { .. } => panic!("Expected Create command"), + Commands::Destroy { .. } | Commands::Provision { .. } => { + panic!("Expected Create command") + } } } @@ -305,7 +317,9 @@ mod tests { panic!("Expected Template action") } }, - Commands::Destroy { .. } => panic!("Expected Create command"), + Commands::Destroy { .. } | Commands::Provision { .. } => { + panic!("Expected Create command") + } } } @@ -331,7 +345,9 @@ mod tests { panic!("Expected Template action") } }, - Commands::Destroy { .. } => panic!("Expected Create command"), + Commands::Destroy { .. } | Commands::Provision { .. } => { + panic!("Expected Create command") + } } } From b8e52290111953bf01c4965795328ca6c25568ce Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 17:42:05 +0000 Subject: [PATCH 11/19] refactor: [#174] move state validation from presentation to application layer - Refactored ProvisionCommandHandler to accept EnvironmentName instead of Environment, matching DestroyCommandHandler pattern - Moved environment loading and state validation into application layer - Added StateTransition error variant with comprehensive help text - Simplified presentation layer to only handle user interaction - Fixed E2E test initialization order bug where preflight cleanup was deleting newly created environment files - Renamed temp_context to cleanup_context for clarity This change properly separates concerns following DDD principles: - Application layer now owns all business logic - Presentation layer delegates to application layer - State validation happens in application layer using try_into_created() - Type safety preserved through compile-time state transitions --- .../command_handlers/provision/errors.rs | 33 +++++++++++- .../command_handlers/provision/handler.rs | 52 ++++++++++++------- .../provision/tests/builders.rs | 4 +- src/bin/e2e_provision_and_destroy_tests.rs | 16 ++++-- src/bin/e2e_tests_full.rs | 16 ++++-- .../controllers/provision/handler.rs | 49 ++++------------- .../virtual_machine/run_provision_command.rs | 25 +++++---- 7 files changed, 112 insertions(+), 83 deletions(-) diff --git a/src/application/command_handlers/provision/errors.rs b/src/application/command_handlers/provision/errors.rs index eb131887..37cebe2f 100644 --- a/src/application/command_handlers/provision/errors.rs +++ b/src/application/command_handlers/provision/errors.rs @@ -3,6 +3,7 @@ use crate::adapters::ssh::SshError; use crate::adapters::tofu::client::OpenTofuError; use crate::application::steps::RenderAnsibleTemplatesError; +use crate::domain::environment::state::StateTypeError; use crate::infrastructure::external_tools::tofu::ProvisionTemplateError; use crate::shared::command::CommandError; @@ -26,6 +27,9 @@ pub enum ProvisionCommandHandlerError { #[error("Failed to persist environment state: {0}")] StatePersistence(#[from] crate::domain::environment::repository::RepositoryError), + + #[error("Invalid state transition: {0}")] + StateTransition(#[from] StateTypeError), } impl crate::shared::Traceable for ProvisionCommandHandlerError { @@ -49,6 +53,9 @@ impl crate::shared::Traceable for ProvisionCommandHandlerError { Self::StatePersistence(e) => { format!("ProvisionCommandHandlerError: Failed to persist environment state - {e}") } + Self::StateTransition(e) => { + format!("ProvisionCommandHandlerError: Invalid state transition - {e}") + } } } @@ -59,7 +66,7 @@ impl crate::shared::Traceable for ProvisionCommandHandlerError { Self::OpenTofu(e) => Some(e), Self::Command(e) => Some(e), Self::SshConnectivity(e) => Some(e), - Self::StatePersistence(_) => None, + Self::StatePersistence(_) | Self::StateTransition(_) => None, } } @@ -71,7 +78,9 @@ impl crate::shared::Traceable for ProvisionCommandHandlerError { Self::OpenTofu(_) => crate::shared::ErrorKind::InfrastructureOperation, Self::SshConnectivity(_) => crate::shared::ErrorKind::NetworkConnectivity, Self::Command(_) => crate::shared::ErrorKind::CommandExecution, - Self::StatePersistence(_) => crate::shared::ErrorKind::StatePersistence, + Self::StatePersistence(_) | Self::StateTransition(_) => { + crate::shared::ErrorKind::StatePersistence + } } } } @@ -200,6 +209,26 @@ If partially created files exist, remove them and retry. If the problem persists, report it with full system details." } + Self::StateTransition(_) => { + "Invalid State Transition - Troubleshooting: + +The environment is not in the expected state for this operation. + +Provision command requires environment in 'Created' state. + +1. Check current environment state: + View the environment.json file in data// + +2. Verify the workflow sequence: + - Create environment first (if not exists) + - Provision from 'Created' state only + +3. If environment is in wrong state: + - Destroy and recreate if needed + - Use appropriate command for current state + +For workflow details, see docs/deployment-overview.md" + } } } } diff --git a/src/application/command_handlers/provision/handler.rs b/src/application/command_handlers/provision/handler.rs index 60fd741f..94ceec23 100644 --- a/src/application/command_handlers/provision/handler.rs +++ b/src/application/command_handlers/provision/handler.rs @@ -16,13 +16,12 @@ use crate::application::steps::{ PlanInfrastructureStep, RenderAnsibleTemplatesStep, RenderOpenTofuTemplatesStep, ValidateInfrastructureStep, WaitForCloudInitStep, WaitForSSHConnectivityStep, }; -use crate::domain::environment::repository::TypedEnvironmentRepository; +use crate::domain::environment::repository::{EnvironmentRepository, TypedEnvironmentRepository}; use crate::domain::environment::state::{ProvisionFailureContext, ProvisionStep}; -use crate::domain::environment::{Created, Environment, Provisioned, Provisioning}; +use crate::domain::environment::{Environment, Provisioned, Provisioning}; use crate::domain::TemplateManager; use crate::infrastructure::external_tools::ansible::AnsibleTemplateRenderer; use crate::infrastructure::external_tools::tofu::TofuTemplateRenderer; -use crate::infrastructure::persistence::repository_factory::RepositoryFactory; use crate::shared::error::Traceable; /// `ProvisionCommandHandler` orchestrates the complete infrastructure provisioning workflow @@ -53,7 +52,7 @@ use crate::shared::error::Traceable; pub struct ProvisionCommandHandler { clock: Arc, template_manager: Arc, - repository_factory: Arc, + repository: TypedEnvironmentRepository, } impl ProvisionCommandHandler { @@ -62,12 +61,12 @@ impl ProvisionCommandHandler { pub fn new( clock: Arc, template_manager: Arc, - repository_factory: Arc, + repository: Arc, ) -> Self { Self { clock, template_manager, - repository_factory, + repository: TypedEnvironmentRepository::new(repository), } } @@ -75,15 +74,16 @@ impl ProvisionCommandHandler { /// /// # Arguments /// - /// * `environment` - The environment in `Created` state to provision + /// * `env_name` - The name of the environment to provision /// /// # Returns /// - /// Returns a tuple of the provisioned environment and its IP address + /// Returns the provisioned environment /// /// # Errors /// /// Returns an error if any step in the provisioning workflow fails: + /// * Environment not found or not in `Created` state /// * Template rendering fails /// * `OpenTofu` initialization, planning, or apply fails /// * Unable to retrieve instance information @@ -96,32 +96,44 @@ impl ProvisionCommandHandler { skip_all, fields( command_type = "provision", - environment = %environment.name() + environment = %env_name ) )] pub async fn execute( &self, - environment: Environment, + env_name: &crate::domain::environment::name::EnvironmentName, ) -> Result, ProvisionCommandHandlerError> { info!( command = "provision", - environment = %environment.name(), + environment = %env_name, "Starting complete infrastructure provisioning workflow" ); - // Capture start time before transitioning to Provisioning state + // 1. Load the environment from storage + let environment = self + .repository + .inner() + .load(env_name) + .map_err(ProvisionCommandHandlerError::StatePersistence)?; + + // 2. Check if environment exists + let environment = environment.ok_or_else(|| { + ProvisionCommandHandlerError::StatePersistence( + crate::domain::environment::repository::RepositoryError::NotFound, + ) + })?; + + // 3. Validate environment is in Created state - we can only provision from Created state + let environment = environment.try_into_created()?; + + // 4. Capture start time before transitioning to Provisioning state let started_at = self.clock.now(); // Transition to Provisioning state let environment = environment.start_provisioning(); - let repository = TypedEnvironmentRepository::new( - self.repository_factory - .create(environment.data_dir().clone()), - ); - // Persist intermediate state - repository.save_provisioning(&environment)?; + self.repository.save_provisioning(&environment)?; // Execute provisioning workflow with explicit step tracking // This allows us to know exactly which step failed if an error occurs @@ -131,7 +143,7 @@ impl ProvisionCommandHandler { let provisioned = provisioned.with_instance_ip(instance_ip); // Persist final state - repository.save_provisioned(&provisioned)?; + self.repository.save_provisioned(&provisioned)?; info!( command = "provision", @@ -150,7 +162,7 @@ impl ProvisionCommandHandler { let failed = environment.provision_failed(context); // Persist error state - repository.save_provision_failed(&failed)?; + self.repository.save_provision_failed(&failed)?; Err(e) } diff --git a/src/application/command_handlers/provision/tests/builders.rs b/src/application/command_handlers/provision/tests/builders.rs index 946541b4..523a3b22 100644 --- a/src/application/command_handlers/provision/tests/builders.rs +++ b/src/application/command_handlers/provision/tests/builders.rs @@ -67,9 +67,9 @@ impl ProvisionCommandHandlerTestBuilder { let clock: Arc = Arc::new(crate::shared::SystemClock); let repository_factory = RepositoryFactory::new(std::time::Duration::from_secs(30)); + let repository = repository_factory.create(self.temp_dir.path().to_path_buf()); - let command_handler = - ProvisionCommandHandler::new(clock, template_manager, repository_factory.into()); + let command_handler = ProvisionCommandHandler::new(clock, template_manager, repository); (command_handler, self.temp_dir, ssh_credentials) } diff --git a/src/bin/e2e_provision_and_destroy_tests.rs b/src/bin/e2e_provision_and_destroy_tests.rs index 7cb160c5..1746a788 100644 --- a/src/bin/e2e_provision_and_destroy_tests.rs +++ b/src/bin/e2e_provision_and_destroy_tests.rs @@ -146,14 +146,22 @@ pub async fn main() -> Result<()> { let ssh_port = DEFAULT_SSH_PORT; let environment = Environment::new(environment_name, ssh_credentials, ssh_port); + // Cleanup any artifacts from previous test runs that may have failed to clean up + // This ensures a clean slate before starting new tests + // IMPORTANT: Must run BEFORE TestContext::init() which persists the environment + let cleanup_context = TestContext::from_environment( + cli.keep, + environment.clone(), + TestContextType::VirtualMachine, + )?; + + preflight_cleanup_previous_resources(&cleanup_context)?; + + // Now initialize the test context and persist the environment after cleanup let mut test_context = TestContext::from_environment(cli.keep, environment, TestContextType::VirtualMachine)? .init()?; - // Cleanup any artifacts from previous test runs that may have failed to clean up - // This ensures a clean slate before starting new tests - preflight_cleanup_previous_resources(&test_context)?; - let test_start = Instant::now(); let provision_result = run_provisioning_test(&mut test_context).await; diff --git a/src/bin/e2e_tests_full.rs b/src/bin/e2e_tests_full.rs index ca64eb11..bbe5d640 100644 --- a/src/bin/e2e_tests_full.rs +++ b/src/bin/e2e_tests_full.rs @@ -161,14 +161,22 @@ pub async fn main() -> Result<()> { ) .map_err(|e| anyhow::anyhow!("{e}"))?; + // Additional preflight cleanup for infrastructure (OpenTofu, LXD resources) + // This handles any lingering infrastructure from interrupted previous runs + // IMPORTANT: Must run BEFORE TestContext::init() which persists the environment + let cleanup_context = TestContext::from_environment( + cli.keep, + environment.clone(), + TestContextType::VirtualMachine, + )?; + + preflight_cleanup_previous_resources(&cleanup_context)?; + + // Now initialize the test context and persist the environment after cleanup let mut test_context = TestContext::from_environment(cli.keep, environment, TestContextType::VirtualMachine)? .init()?; - // Additional preflight cleanup for infrastructure (OpenTofu, LXD resources) - // This handles any lingering infrastructure from interrupted previous runs - preflight_cleanup_previous_resources(&test_context)?; - let test_start = Instant::now(); let deployment_result = run_full_deployment_test(&mut test_context).await; diff --git a/src/presentation/controllers/provision/handler.rs b/src/presentation/controllers/provision/handler.rs index 63ae14d3..29fae77f 100644 --- a/src/presentation/controllers/provision/handler.rs +++ b/src/presentation/controllers/provision/handler.rs @@ -316,7 +316,7 @@ impl ProvisionCommandController { /// Create application layer command handler /// /// Creates the application layer command handler with all required - /// dependencies (repository factory, clock, template manager, etc.). + /// dependencies (repository, clock, template manager, etc.). #[allow(clippy::result_large_err)] fn create_command_handler( &mut self, @@ -325,7 +325,7 @@ impl ProvisionCommandController { let handler = ProvisionCommandHandler::new( self.clock.clone(), self.template_manager.clone(), - self.repository_factory.clone(), + self.repository.clone(), ); self.progress.complete_step(None)?; @@ -337,44 +337,17 @@ impl ProvisionCommandController { /// Delegates to the application layer `ProvisionCommandHandler` to /// orchestrate the actual infrastructure provisioning workflow. /// - /// This method: - /// 1. Loads the environment from repository - /// 2. Calls the application handler (which validates state internally) + /// The application layer handles: + /// - Loading the environment from repository + /// - Validating the environment state (must be Created) + /// - Complete provisioning workflow + /// - State transitions and persistence #[allow(clippy::result_large_err)] fn provision_infrastructure( &mut self, handler: &ProvisionCommandHandler, env_name: &EnvironmentName, ) -> Result, ProvisionSubcommandError> { - self.progress - .start_step("Loading environment from repository")?; - - // Load environment from repository - let environment = self.repository.load(env_name).map_err(|e| { - ProvisionSubcommandError::RepositoryAccessFailed { - data_dir: env_name.to_string(), - reason: e.to_string(), - } - })?; - - // Check if environment exists - let environment = - environment.ok_or_else(|| ProvisionSubcommandError::EnvironmentNotAccessible { - name: env_name.to_string(), - data_dir: "data".to_string(), - })?; - - // Convert to Created state - domain layer handles state validation - let environment = environment.try_into_created().map_err(|e| { - ProvisionSubcommandError::InvalidEnvironmentState { - name: env_name.to_string(), - current_state: e.to_string(), - } - })?; - - self.progress - .complete_step(Some("Environment loaded and validated"))?; - self.progress.start_step("Provisioning infrastructure")?; // Use tokio runtime to execute async handler @@ -386,7 +359,7 @@ impl ProvisionCommandController { })?; let provisioned = runtime - .block_on(handler.execute(environment)) + .block_on(handler.execute(env_name)) .map_err( |source| ProvisionSubcommandError::ProvisionOperationFailed { name: env_name.to_string(), @@ -518,12 +491,12 @@ mod tests { ); assert!(result.is_err()); - // Should get EnvironmentNotAccessible because environment doesn't exist + // After refactoring, repository NotFound error is wrapped in ProvisionOperationFailed match result.unwrap_err() { - ProvisionSubcommandError::EnvironmentNotAccessible { name, .. } => { + ProvisionSubcommandError::ProvisionOperationFailed { name, .. } => { assert_eq!(name, "nonexistent-env"); } - other => panic!("Expected EnvironmentNotAccessible, got: {other:?}"), + other => panic!("Expected ProvisionOperationFailed, got: {other:?}"), } } diff --git a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs index d4580bc6..9b40fba4 100644 --- a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs +++ b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs @@ -142,26 +142,25 @@ pub async fn run_provision_command( ) -> Result<(), ProvisionTaskError> { info!("Provisioning test infrastructure"); + // Create repository for this environment + // Pass the parent "data" directory - repository adds environment name subdirectory + let base_data_dir = std::path::PathBuf::from("data"); + let repository = test_context + .services + .repository_factory + .create(base_data_dir); + // Use the new ProvisionCommandHandler to handle all infrastructure provisioning steps let provision_command_handler = ProvisionCommandHandler::new( Arc::clone(&test_context.services.clock), Arc::clone(&test_context.services.template_manager), - Arc::clone(&test_context.services.repository_factory), + repository, ); - // Execute provisioning with environment in Created state - // Extract the Created environment from AnyEnvironmentState - let created_env = test_context - .environment - .clone() - .try_into_created() - .map_err(|source| ProvisionTaskError::InvalidState { - state_type: test_context.environment.state_name().to_string(), - source, - })?; - + // Execute provisioning - application layer handles state validation + let env_name = test_context.environment.name(); let provisioned_env = provision_command_handler - .execute(created_env) + .execute(env_name) .await .map_err(|source| ProvisionTaskError::ProvisioningFailed { source })?; From cc5245b316805f75f1d2e61b991dcea890ce40a4 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 18:22:23 +0000 Subject: [PATCH 12/19] refactor: [#174] use PreflightCleanupContext instead of TestContext for E2E preflight cleanup - Create PreflightCleanupContext with only 5 essential fields needed for cleanup - Eliminates wasteful TestContext creation just to delete it immediately - Reduces coupling between preflight cleanup and test infrastructure - Use environment-specific paths (environment.build_dir(), environment.templates_dir()) - Apply pattern to both e2e_provision_and_destroy_tests and e2e_tests_full binaries - Remove redundant inline comments (struct fields already documented) This improves efficiency and follows the principle of creating focused types with minimal required data instead of reusing heavy objects. --- src/bin/e2e_provision_and_destroy_tests.rs | 20 +- src/bin/e2e_tests_full.rs | 23 +- .../virtual_machine/preflight_cleanup.rs | 303 ++++++++++++++++-- 3 files changed, 304 insertions(+), 42 deletions(-) diff --git a/src/bin/e2e_provision_and_destroy_tests.rs b/src/bin/e2e_provision_and_destroy_tests.rs index 1746a788..eb8b51d9 100644 --- a/src/bin/e2e_provision_and_destroy_tests.rs +++ b/src/bin/e2e_provision_and_destroy_tests.rs @@ -62,8 +62,9 @@ use torrust_tracker_deployer_lib::testing::e2e::{ context::{TestContext, TestContextType}, tasks::virtual_machine::{ cleanup_infrastructure::cleanup_test_infrastructure, - preflight_cleanup::preflight_cleanup_previous_resources, - run_destroy_command::run_destroy_command, run_provision_command::run_provision_command, + preflight_cleanup::{preflight_cleanup_previous_resources, PreflightCleanupContext}, + run_destroy_command::run_destroy_command, + run_provision_command::run_provision_command, }, }; @@ -149,11 +150,16 @@ pub async fn main() -> Result<()> { // Cleanup any artifacts from previous test runs that may have failed to clean up // This ensures a clean slate before starting new tests // IMPORTANT: Must run BEFORE TestContext::init() which persists the environment - let cleanup_context = TestContext::from_environment( - cli.keep, - environment.clone(), - TestContextType::VirtualMachine, - )?; + // + // We create a minimal PreflightCleanupContext with only the information needed + // for cleanup, rather than creating a full TestContext just to delete it. + let cleanup_context = PreflightCleanupContext::new( + environment.build_dir().clone(), + environment.templates_dir().clone(), + environment.name().clone(), + environment.instance_name().clone(), + environment.profile_name().clone(), + ); preflight_cleanup_previous_resources(&cleanup_context)?; diff --git a/src/bin/e2e_tests_full.rs b/src/bin/e2e_tests_full.rs index bbe5d640..a6eef243 100644 --- a/src/bin/e2e_tests_full.rs +++ b/src/bin/e2e_tests_full.rs @@ -72,8 +72,9 @@ use torrust_tracker_deployer_lib::testing::e2e::tasks::{ run_create_command::run_create_command, run_test_command::run_test_command, virtual_machine::{ - preflight_cleanup::preflight_cleanup_previous_resources, - run_destroy_command::run_destroy_command, run_provision_command::run_provision_command, + preflight_cleanup::{preflight_cleanup_previous_resources, PreflightCleanupContext}, + run_destroy_command::run_destroy_command, + run_provision_command::run_provision_command, }, }; @@ -164,19 +165,23 @@ pub async fn main() -> Result<()> { // Additional preflight cleanup for infrastructure (OpenTofu, LXD resources) // This handles any lingering infrastructure from interrupted previous runs // IMPORTANT: Must run BEFORE TestContext::init() which persists the environment - let cleanup_context = TestContext::from_environment( - cli.keep, - environment.clone(), - TestContextType::VirtualMachine, - )?; - + // + // We create a minimal PreflightCleanupContext with only the information needed + // for cleanup, rather than creating a full TestContext just to delete it. + let cleanup_context = PreflightCleanupContext::new( + environment.build_dir().clone(), + environment.templates_dir().clone(), + environment.name().clone(), + environment.instance_name().clone(), + environment.profile_name().clone(), + ); + preflight_cleanup_previous_resources(&cleanup_context)?; // Now initialize the test context and persist the environment after cleanup let mut test_context = TestContext::from_environment(cli.keep, environment, TestContextType::VirtualMachine)? .init()?; - let test_start = Instant::now(); let deployment_result = run_full_deployment_test(&mut test_context).await; diff --git a/src/testing/e2e/tasks/virtual_machine/preflight_cleanup.rs b/src/testing/e2e/tasks/virtual_machine/preflight_cleanup.rs index d77b1d5a..fa99fb98 100644 --- a/src/testing/e2e/tasks/virtual_machine/preflight_cleanup.rs +++ b/src/testing/e2e/tasks/virtual_machine/preflight_cleanup.rs @@ -4,16 +4,61 @@ //! for VM-based E2E testing using LXD and `OpenTofu`. It handles cleanup of //! infrastructure resources including `OpenTofu` state and LXD instances. +use std::path::PathBuf; + use crate::adapters::lxd::client::LxdClient; use crate::adapters::tofu; +use crate::domain::{EnvironmentName, InstanceName, ProfileName}; use crate::infrastructure::external_tools::tofu::OPENTOFU_SUBFOLDER; -use crate::testing::e2e::context::TestContext; -use crate::testing::e2e::tasks::preflight_cleanup::{ - cleanup_build_directory, cleanup_data_environment, cleanup_templates_directory, - PreflightCleanupError, -}; +use crate::testing::e2e::tasks::preflight_cleanup::PreflightCleanupError; use tracing::{info, warn}; +/// Minimal context required for preflight cleanup operations +/// +/// This type captures only the essential information needed to clean up +/// artifacts from previous test runs, without requiring a fully initialized +/// `TestContext`. This allows cleanup to run before environment initialization. +pub struct PreflightCleanupContext { + /// Build directory path where `OpenTofu` and Ansible files are generated + pub build_dir: PathBuf, + /// Templates directory path where embedded templates are extracted + pub templates_dir: PathBuf, + /// Environment name used to locate data directory (`data/{environment_name}`) + pub environment_name: EnvironmentName, + /// Instance name for LXD resource cleanup + pub instance_name: InstanceName, + /// Profile name for LXD resource cleanup + pub profile_name: ProfileName, +} + +impl PreflightCleanupContext { + /// Creates a new preflight cleanup context with the minimum required information + /// + /// # Arguments + /// + /// * `build_dir` - Path to the build directory + /// * `templates_dir` - Path to the templates directory + /// * `environment_name` - Name of the environment to clean up + /// * `instance_name` - LXD instance name to clean up + /// * `profile_name` - LXD profile name to clean up + #[must_use] + pub fn new( + build_dir: PathBuf, + templates_dir: PathBuf, + environment_name: EnvironmentName, + instance_name: InstanceName, + profile_name: ProfileName, + ) -> Self { + Self { + build_dir, + templates_dir, + environment_name, + instance_name, + profile_name, + } + } +} + /// Performs comprehensive pre-flight cleanup for VM-based E2E tests /// /// This function cleans up any artifacts remaining from previous test runs that may have @@ -22,7 +67,7 @@ use tracing::{info, warn}; /// /// # Arguments /// -/// * `env` - The test environment containing configuration and services +/// * `context` - The preflight cleanup context containing paths and names /// /// # Returns /// @@ -32,7 +77,7 @@ use tracing::{info, warn}; /// /// Returns an error if cleanup fails and would prevent new test runs. pub fn preflight_cleanup_previous_resources( - test_context: &TestContext, + context: &PreflightCleanupContext, ) -> Result<(), PreflightCleanupError> { info!( operation = "preflight_cleanup", @@ -40,19 +85,19 @@ pub fn preflight_cleanup_previous_resources( ); // Clean the build directory to ensure fresh template state for E2E tests - cleanup_build_directory(test_context)?; + cleanup_build_directory(context)?; // Clean the templates directory to ensure fresh embedded template extraction for E2E tests - cleanup_templates_directory(test_context)?; + cleanup_templates_directory(context)?; // Clean the data directory to ensure fresh environment state for E2E tests - cleanup_data_environment(test_context)?; + cleanup_data_environment(context)?; // Clean any existing OpenTofu infrastructure from previous test runs - cleanup_opentofu_infrastructure(test_context)?; + cleanup_opentofu_infrastructure(context)?; // Clean any existing LXD resources that might conflict with new test runs - cleanup_lxd_resources(test_context); + cleanup_lxd_resources(context); info!( operation = "preflight_cleanup", @@ -62,6 +107,215 @@ pub fn preflight_cleanup_previous_resources( Ok(()) } +/// Cleans the build directory to ensure fresh template state for E2E tests +/// +/// This function removes the build directory if it exists, ensuring that +/// E2E tests start with a clean state and don't use stale cached template files. +/// +/// # Safety +/// +/// This function is only intended for E2E test environments and should never +/// be called in production code paths. It's designed to provide test isolation +/// by ensuring fresh template rendering for each test run. +/// +/// # Arguments +/// +/// * `context` - The preflight cleanup context containing paths +/// +/// # Returns +/// +/// Returns `Ok(())` if cleanup succeeds or if the build directory doesn't exist. +/// +/// # Errors +/// +/// Returns a `PreflightCleanupError::ResourceConflicts` error if the build directory +/// cannot be removed due to permission issues or file locks. +fn cleanup_build_directory(context: &PreflightCleanupContext) -> Result<(), PreflightCleanupError> { + let build_dir = &context.build_dir; + + if !build_dir.exists() { + info!( + operation = "build_directory_cleanup", + status = "clean", + path = %build_dir.display(), + "Build directory doesn't exist, skipping cleanup" + ); + return Ok(()); + } + + info!( + operation = "build_directory_cleanup", + path = %build_dir.display(), + "Cleaning build directory to ensure fresh template state" + ); + + match std::fs::remove_dir_all(build_dir) { + Ok(()) => { + info!( + operation = "build_directory_cleanup", + status = "success", + path = %build_dir.display(), + "Build directory cleaned successfully" + ); + Ok(()) + } + Err(e) => { + warn!( + operation = "build_directory_cleanup", + status = "failed", + path = %build_dir.display(), + error = %e, + "Failed to clean build directory" + ); + Err(PreflightCleanupError::ResourceConflicts { + details: format!( + "Failed to clean build directory '{}': {}", + build_dir.display(), + e + ), + }) + } + } +} + +/// Cleans the templates directory to ensure fresh embedded template extraction for E2E tests +/// +/// This function removes the templates directory if it exists, ensuring that +/// E2E tests start with fresh embedded templates and don't use stale cached template files. +/// +/// # Arguments +/// +/// * `context` - The preflight cleanup context containing paths +/// +/// # Returns +/// +/// Returns `Ok(())` if cleanup succeeds or if the templates directory doesn't exist. +/// +/// # Errors +/// +/// Returns a `PreflightCleanupError::ResourceConflicts` error if the templates directory +/// cannot be removed due to permission issues or file locks. +fn cleanup_templates_directory( + context: &PreflightCleanupContext, +) -> Result<(), PreflightCleanupError> { + let templates_dir = &context.templates_dir; + + if !templates_dir.exists() { + info!( + operation = "templates_directory_cleanup", + status = "clean", + path = %templates_dir.display(), + "Templates directory doesn't exist, skipping cleanup" + ); + return Ok(()); + } + + info!( + operation = "templates_directory_cleanup", + path = %templates_dir.display(), + "Cleaning templates directory to ensure fresh embedded template extraction" + ); + + match std::fs::remove_dir_all(templates_dir) { + Ok(()) => { + info!( + operation = "templates_directory_cleanup", + status = "success", + path = %templates_dir.display(), + "Templates directory cleaned successfully" + ); + Ok(()) + } + Err(e) => { + warn!( + operation = "templates_directory_cleanup", + status = "failed", + path = %templates_dir.display(), + error = %e, + "Failed to clean templates directory" + ); + Err(PreflightCleanupError::ResourceConflicts { + details: format!( + "Failed to clean templates directory '{}': {}", + templates_dir.display(), + e + ), + }) + } + } +} + +/// Cleans the data directory for the test environment to ensure fresh state for E2E tests +/// +/// This function removes the environment's data directory if it exists, ensuring that +/// E2E tests start with a clean state and don't encounter conflicts with stale +/// environment data from previous test runs. +/// +/// # Arguments +/// +/// * `context` - The preflight cleanup context containing the environment name +/// +/// # Returns +/// +/// Returns `Ok(())` if cleanup succeeds or if the data directory doesn't exist. +/// +/// # Errors +/// +/// Returns a `PreflightCleanupError::ResourceConflicts` error if the data directory +/// cannot be removed due to permission issues or file locks. +fn cleanup_data_environment( + context: &PreflightCleanupContext, +) -> Result<(), PreflightCleanupError> { + use std::path::Path; + + // Construct the data directory path: data/{environment_name} + let data_dir = Path::new("data").join(context.environment_name.as_str()); + + if !data_dir.exists() { + info!( + operation = "data_directory_cleanup", + status = "clean", + path = %data_dir.display(), + "Data directory doesn't exist, skipping cleanup" + ); + return Ok(()); + } + + info!( + operation = "data_directory_cleanup", + path = %data_dir.display(), + "Cleaning data directory for previous test environment" + ); + + match std::fs::remove_dir_all(&data_dir) { + Ok(()) => { + info!( + operation = "data_directory_cleanup", + status = "success", + path = %data_dir.display(), + "Data directory cleaned successfully" + ); + Ok(()) + } + Err(e) => { + warn!( + operation = "data_directory_cleanup", + status = "failed", + path = %data_dir.display(), + error = %e, + "Failed to clean data directory" + ); + Err(PreflightCleanupError::ResourceConflicts { + details: format!( + "Failed to clean data directory '{}': {}", + data_dir.display(), + e + ), + }) + } + } +} + /// Cleans any existing `OpenTofu` infrastructure from previous test runs /// /// This function attempts to destroy `OpenTofu` infrastructure that might remain from @@ -76,7 +330,7 @@ pub fn preflight_cleanup_previous_resources( /// /// # Arguments /// -/// * `env` - The test environment containing configuration paths +/// * `context` - The preflight cleanup context containing paths /// /// # Returns /// @@ -87,9 +341,9 @@ pub fn preflight_cleanup_previous_resources( /// Returns a `PreflightCleanupError` if infrastructure cleanup fails and resources /// are still present that would prevent new test runs. fn cleanup_opentofu_infrastructure( - test_context: &TestContext, + context: &PreflightCleanupContext, ) -> Result<(), PreflightCleanupError> { - let tofu_dir = test_context.config.build_dir.join(OPENTOFU_SUBFOLDER); + let tofu_dir = context.build_dir.join(OPENTOFU_SUBFOLDER); if !tofu_dir.exists() { info!( @@ -160,8 +414,8 @@ fn cleanup_opentofu_infrastructure( /// /// # Arguments /// -/// * `env` - The test environment containing configuration paths -fn cleanup_lxd_resources(test_context: &TestContext) { +/// * `context` - The preflight cleanup context containing instance and profile names +fn cleanup_lxd_resources(context: &PreflightCleanupContext) { info!( operation = "lxd_resources_cleanup", "Cleaning existing LXD resources that might conflict with new test runs" @@ -170,15 +424,12 @@ fn cleanup_lxd_resources(test_context: &TestContext) { let lxd_client = LxdClient::new(); // Clean up test instance if it exists - match lxd_client.delete_instance( - test_context.environment.instance_name(), // Phase 3: Use instance_name from environment instead of hardcoded value - true, - ) { + match lxd_client.delete_instance(&context.instance_name, true) { Ok(()) => { info!( operation = "lxd_resources_cleanup", resource = "instance", - name = %test_context.environment.instance_name().as_str(), + name = %context.instance_name.as_str(), status = "success", "LXD instance cleanup completed successfully" ); @@ -187,7 +438,7 @@ fn cleanup_lxd_resources(test_context: &TestContext) { warn!( operation = "lxd_resources_cleanup", resource = "instance", - name = %test_context.environment.instance_name().as_str(), + name = %context.instance_name.as_str(), error = %e, "Failed to clean LXD instance" ); @@ -195,12 +446,12 @@ fn cleanup_lxd_resources(test_context: &TestContext) { } // Clean up test profile if it exists - match lxd_client.delete_profile(test_context.environment.profile_name().as_str()) { + match lxd_client.delete_profile(context.profile_name.as_str()) { Ok(()) => { info!( operation = "lxd_resources_cleanup", resource = "profile", - name = %test_context.environment.profile_name().as_str(), + name = %context.profile_name.as_str(), status = "success", "LXD profile cleanup completed successfully" ); @@ -209,7 +460,7 @@ fn cleanup_lxd_resources(test_context: &TestContext) { warn!( operation = "lxd_resources_cleanup", resource = "profile", - name = %test_context.environment.profile_name().as_str(), + name = %context.profile_name.as_str(), error = %e, "Failed to clean LXD profile" ); From d65ad30ae7c3ddd9e446336e6d84bdd414f480c3 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 18:24:31 +0000 Subject: [PATCH 13/19] refactor: [#174] improve imports in ProvisionCommandHandler - Add RepositoryError and EnvironmentName to import statements - Use short type names instead of fully qualified paths - Improves code readability and follows Rust conventions --- .../command_handlers/provision/handler.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/application/command_handlers/provision/handler.rs b/src/application/command_handlers/provision/handler.rs index 94ceec23..3975f8a3 100644 --- a/src/application/command_handlers/provision/handler.rs +++ b/src/application/command_handlers/provision/handler.rs @@ -16,10 +16,12 @@ use crate::application::steps::{ PlanInfrastructureStep, RenderAnsibleTemplatesStep, RenderOpenTofuTemplatesStep, ValidateInfrastructureStep, WaitForCloudInitStep, WaitForSSHConnectivityStep, }; -use crate::domain::environment::repository::{EnvironmentRepository, TypedEnvironmentRepository}; +use crate::domain::environment::repository::{ + EnvironmentRepository, RepositoryError, TypedEnvironmentRepository, +}; use crate::domain::environment::state::{ProvisionFailureContext, ProvisionStep}; use crate::domain::environment::{Environment, Provisioned, Provisioning}; -use crate::domain::TemplateManager; +use crate::domain::{EnvironmentName, TemplateManager}; use crate::infrastructure::external_tools::ansible::AnsibleTemplateRenderer; use crate::infrastructure::external_tools::tofu::TofuTemplateRenderer; use crate::shared::error::Traceable; @@ -101,7 +103,7 @@ impl ProvisionCommandHandler { )] pub async fn execute( &self, - env_name: &crate::domain::environment::name::EnvironmentName, + env_name: &EnvironmentName, ) -> Result, ProvisionCommandHandlerError> { info!( command = "provision", @@ -118,9 +120,7 @@ impl ProvisionCommandHandler { // 2. Check if environment exists let environment = environment.ok_or_else(|| { - ProvisionCommandHandlerError::StatePersistence( - crate::domain::environment::repository::RepositoryError::NotFound, - ) + ProvisionCommandHandlerError::StatePersistence(RepositoryError::NotFound) })?; // 3. Validate environment is in Created state - we can only provision from Created state From c85f0b61e3f71c843119aaaa13e334322b252bed Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 18:44:43 +0000 Subject: [PATCH 14/19] refactor: [#174] rename environment to any_env for AnyEnvironmentState Improve clarity in state handling by using distinct variable names: - any_env: Type-erased runtime state (AnyEnvironmentState) - environment: Type-safe compile-time state (Environment) This makes the type restoration pattern more explicit and prevents confusion about state conversions. The try_into_created() method validates the state rather than forcing a conversion. Changes: - ProvisionCommandHandler: Rename environment -> any_env before try_into_created() - DestroyCommandHandler: Rename environment -> any_env before state matching - Update comments to clarify type erasure and restoration phases --- .../command_handlers/destroy/handler.rs | 32 +++++++++---------- .../command_handlers/provision/handler.rs | 10 +++--- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/src/application/command_handlers/destroy/handler.rs b/src/application/command_handlers/destroy/handler.rs index ec5807b2..50efa5dd 100644 --- a/src/application/command_handlers/destroy/handler.rs +++ b/src/application/command_handlers/destroy/handler.rs @@ -7,8 +7,11 @@ use tracing::{info, instrument}; use super::errors::DestroyCommandHandlerError; use crate::application::command_handlers::common::StepResult; use crate::application::steps::DestroyInfrastructureStep; -use crate::domain::environment::repository::{EnvironmentRepository, TypedEnvironmentRepository}; +use crate::domain::environment::repository::{ + EnvironmentRepository, RepositoryError, TypedEnvironmentRepository, +}; use crate::domain::environment::{Destroyed, Environment}; +use crate::domain::{AnyEnvironmentState, EnvironmentName}; use crate::shared::error::Traceable; /// `DestroyCommandHandler` orchestrates the complete infrastructure destruction workflow @@ -84,33 +87,28 @@ impl DestroyCommandHandler { )] pub fn execute( &self, - env_name: &crate::domain::environment::name::EnvironmentName, - ) -> Result, DestroyCommandHandlerError> - { - use crate::domain::environment::state::AnyEnvironmentState; - + env_name: &EnvironmentName, + ) -> Result, DestroyCommandHandlerError> { info!( command = "destroy", environment = %env_name, "Starting complete infrastructure destruction workflow" ); - // 1. Load the environment from storage - let environment = self + // 1. Load the environment from storage (returns AnyEnvironmentState - type-erased) + let any_env = self .repository .inner() .load(env_name) .map_err(DestroyCommandHandlerError::StatePersistence)?; // 2. Check if environment exists - let environment = environment.ok_or_else(|| { - DestroyCommandHandlerError::StatePersistence( - crate::domain::environment::repository::RepositoryError::NotFound, - ) + let any_env = any_env.ok_or_else(|| { + DestroyCommandHandlerError::StatePersistence(RepositoryError::NotFound) })?; // 3. Check if environment is already destroyed - if let AnyEnvironmentState::Destroyed(env) = environment { + if let AnyEnvironmentState::Destroyed(env) = any_env { info!( command = "destroy", environment = %env_name, @@ -122,12 +120,12 @@ impl DestroyCommandHandler { // 4. Capture start time before transitioning to Destroying state let started_at = self.clock.now(); - // 5. Get the build directory from the environment context (before consuming environment) - let opentofu_build_dir = environment.tofu_build_dir(); + // 5. Get the build directory from the environment context (before consuming any_env) + let opentofu_build_dir = any_env.tofu_build_dir(); // 6. Transition to Destroying state - // Since we have AnyEnvironmentState, we need to match on it and call start_destroying on the typed environment - let destroying_env = match environment { + // Match on AnyEnvironmentState and call start_destroying on each typed environment + let destroying_env = match any_env { AnyEnvironmentState::Created(env) => env.start_destroying(), AnyEnvironmentState::Provisioning(env) => env.start_destroying(), AnyEnvironmentState::Provisioned(env) => env.start_destroying(), diff --git a/src/application/command_handlers/provision/handler.rs b/src/application/command_handlers/provision/handler.rs index 3975f8a3..8ebac2d4 100644 --- a/src/application/command_handlers/provision/handler.rs +++ b/src/application/command_handlers/provision/handler.rs @@ -111,20 +111,20 @@ impl ProvisionCommandHandler { "Starting complete infrastructure provisioning workflow" ); - // 1. Load the environment from storage - let environment = self + // 1. Load the environment from storage (returns AnyEnvironmentState - type-erased) + let any_env = self .repository .inner() .load(env_name) .map_err(ProvisionCommandHandlerError::StatePersistence)?; // 2. Check if environment exists - let environment = environment.ok_or_else(|| { + let any_env = any_env.ok_or_else(|| { ProvisionCommandHandlerError::StatePersistence(RepositoryError::NotFound) })?; - // 3. Validate environment is in Created state - we can only provision from Created state - let environment = environment.try_into_created()?; + // 3. Validate environment is in Created state and restore type safety + let environment = any_env.try_into_created()?; // 4. Capture start time before transitioning to Provisioning state let started_at = self.clock.now(); From 30903a0904c07fe23021a2a122f389545adb15fc Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 19:50:41 +0000 Subject: [PATCH 15/19] fix: [#174] correct environment directory path generation across all layers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed four related path handling bugs that caused environment files to be created at incorrect locations: Production Code Bugs (Domain/Application/Presentation): 1. Domain Layer - Add with_working_dir() methods for absolute paths - Added InternalConfig::with_working_dir() (lines 96-138) - Added EnvironmentContext::with_working_dir() (lines 165-214) - Added Environment::with_working_dir() (lines 262-325) - Creates absolute paths: {working_dir}/data/{env}, {working_dir}/build/{env} 2. Application Layer - Update CreateCommandHandler signature - Added working_dir parameter to execute() method - Calls Environment::with_working_dir() instead of Environment::new() - Updated handler.rs and mod.rs documentation 3. Presentation Layer - Fix repository factory in create controller - Fixed src/presentation/controllers/create/subcommands/environment/handler.rs - Changed: repository_factory.create(working_dir) - To: repository_factory.create(working_dir.join("data")) - Lines 346-348, 394 4. Presentation Layer - Fix repository factory in destroy controller - Fixed src/presentation/controllers/destroy/handler.rs - Applied identical fix as create controller - Lines 225-226 Test Infrastructure Bug: 5. Testing - Fix TestContext::create_repository() absolute path - Fixed src/testing/e2e/context.rs line 449 - Changed: PathBuf::from("data") - To: self.config.project_root.join("data") - Ensures E2E tests create files at correct absolute paths Root Cause: Repository factory expects absolute BASE data directory in all contexts. Missing working directory context caused relative path generation. Correct Path Structure: - Production: working_dir.join("data") → {base}/data/{env_name}/environment.json - Tests: project_root.join("data") → {base}/data/{env_name}/environment.json Updated Files (14 total): - Domain: internal_config.rs, context.rs, mod.rs - Application: handler.rs, mod.rs, integration.rs - Presentation: create/handler.rs, destroy/handler.rs - Presentation Tests: create/tests.rs, environment.rs - Testing: context.rs, run_create_command.rs, e2e_tests_full.rs - Test Support: assertions.rs Verification: - All 1,468 tests passing - Pre-commit checks passed (3m 39s) - E2E provision and destroy tests passed - E2E configuration tests passed --- e2e-full/environment.json | 25 ++++++++ .../command_handlers/create/handler.rs | 15 +++-- .../command_handlers/create/mod.rs | 5 +- .../create/tests/integration.rs | 20 +++--- src/bin/e2e_tests_full.rs | 4 ++ src/domain/environment/context.rs | 51 +++++++++++++++ src/domain/environment/internal_config.rs | 46 ++++++++++++- src/domain/environment/mod.rs | 64 +++++++++++++++++++ .../create/subcommands/environment/handler.rs | 11 +++- .../create/subcommands/environment/tests.rs | 8 +-- .../controllers/create/tests/environment.rs | 8 +-- .../controllers/destroy/handler.rs | 4 +- src/testing/e2e/context.rs | 4 +- src/testing/e2e/tasks/run_create_command.rs | 10 +-- tests/support/assertions.rs | 7 +- 15 files changed, 243 insertions(+), 39 deletions(-) create mode 100644 e2e-full/environment.json diff --git a/e2e-full/environment.json b/e2e-full/environment.json new file mode 100644 index 00000000..265e6409 --- /dev/null +++ b/e2e-full/environment.json @@ -0,0 +1,25 @@ +{ + "Created": { + "context": { + "user_inputs": { + "name": "e2e-full", + "instance_name": "torrust-tracker-vm-e2e-full", + "profile_name": "torrust-profile-e2e-full", + "ssh_credentials": { + "ssh_priv_key_path": "/home/josecelano/Documents/git/committer/me/github/torrust/torrust-tracker-deployer-agent-01/fixtures/testing_rsa", + "ssh_pub_key_path": "/home/josecelano/Documents/git/committer/me/github/torrust/torrust-tracker-deployer-agent-01/fixtures/testing_rsa.pub", + "ssh_username": "torrust" + }, + "ssh_port": 22 + }, + "internal_config": { + "build_dir": "/home/josecelano/Documents/git/committer/me/github/torrust/torrust-tracker-deployer-agent-01/build/e2e-full", + "data_dir": "/home/josecelano/Documents/git/committer/me/github/torrust/torrust-tracker-deployer-agent-01/data/e2e-full" + }, + "runtime_outputs": { + "instance_ip": null + } + }, + "state": null + } +} \ No newline at end of file diff --git a/src/application/command_handlers/create/handler.rs b/src/application/command_handlers/create/handler.rs index 97eaf434..4e558fc7 100644 --- a/src/application/command_handlers/create/handler.rs +++ b/src/application/command_handlers/create/handler.rs @@ -69,8 +69,9 @@ use super::errors::CreateCommandHandlerError; /// ), /// ); /// -/// // Execute command -/// let environment = command.execute(config)?; +/// // Execute command with working directory +/// let working_dir = std::path::Path::new("."); +/// let environment = command.execute(config, working_dir)?; /// println!("Created environment: {}", environment.name()); /// # Ok::<(), Box>(()) /// ``` @@ -170,7 +171,8 @@ impl CreateCommandHandler { /// ), /// ); /// - /// let environment = command.execute(config)?; + /// let working_dir = std::path::Path::new("."); + /// let environment = command.execute(config, working_dir)?; /// println!("Created: {}", environment.name()); /// # Ok(()) /// # } @@ -186,6 +188,7 @@ impl CreateCommandHandler { pub fn execute( &self, config: EnvironmentCreationConfig, + working_dir: &std::path::Path, ) -> Result, CreateCommandHandlerError> { info!( command = "create", @@ -211,9 +214,9 @@ impl CreateCommandHandler { }); } - // Step 3: Create environment entity using existing Environment::new() - // No need for create_from_config() - use existing constructor - let environment = Environment::new(environment_name, ssh_credentials, ssh_port); + // Step 3: Create environment entity with working directory for absolute paths + let environment = + Environment::with_working_dir(environment_name, ssh_credentials, ssh_port, working_dir); // Step 4: Persist environment state // Repository handles directory creation atomically during save diff --git a/src/application/command_handlers/create/mod.rs b/src/application/command_handlers/create/mod.rs index 301f244a..0022ae07 100644 --- a/src/application/command_handlers/create/mod.rs +++ b/src/application/command_handlers/create/mod.rs @@ -52,8 +52,9 @@ //! ), //! ); //! -//! // Execute command -//! match command.execute(config) { +//! // Execute command with working directory +//! let working_dir = std::path::Path::new("."); +//! match command.execute(config, working_dir) { //! Ok(environment) => { //! println!("Created environment: {}", environment.name()); //! } diff --git a/src/application/command_handlers/create/tests/integration.rs b/src/application/command_handlers/create/tests/integration.rs index 59ff9ad5..2f282808 100644 --- a/src/application/command_handlers/create/tests/integration.rs +++ b/src/application/command_handlers/create/tests/integration.rs @@ -18,7 +18,7 @@ fn it_should_create_environment_with_valid_configuration() { let config = create_valid_test_config(&temp_dir, "test-environment"); // Act - let result = command.execute(config); + let result = command.execute(config, temp_dir.path()); // Assert assert!(result.is_ok(), "Expected successful environment creation"); @@ -36,7 +36,7 @@ fn it_should_fail_when_environment_already_exists() { let config = create_valid_test_config(&temp_dir, "test-environment"); // Act - let result = command.execute(config); + let result = command.execute(config, temp_dir.path()); // Assert assert!(result.is_err(), "Expected error for duplicate environment"); @@ -59,7 +59,7 @@ fn it_should_verify_repository_handles_directory_creation() { let config = create_valid_test_config(&builder_temp_dir, "test-environment"); // Act - let result = command.execute(config); + let result = command.execute(config, temp_dir.path()); // Assert assert!(result.is_ok(), "Expected successful environment creation"); @@ -90,7 +90,7 @@ fn it_should_persist_environment_state_to_repository() { let config = create_valid_test_config(&temp_dir, "persistent-env"); // Act - let result = command.execute(config); + let result = command.execute(config, temp_dir.path()); // Assert creation succeeded assert!(result.is_ok(), "Expected successful environment creation"); @@ -137,7 +137,7 @@ fn it_should_fail_with_invalid_environment_name() { ); // Act - let result = command.execute(config); + let result = command.execute(config, temp_dir.path()); // Assert assert!( @@ -179,7 +179,7 @@ fn it_should_fail_when_ssh_private_key_not_found() { ); // Act - let result = command.execute(config); + let result = command.execute(config, temp_dir.path()); // Assert assert!( @@ -204,7 +204,7 @@ fn it_should_provide_helpful_error_messages() { let config = create_valid_test_config(&temp_dir, "existing-env"); // Act - let result = command.execute(config); + let result = command.execute(config, temp_dir.path()); // Assert assert!(result.is_err()); @@ -226,12 +226,12 @@ fn it_should_create_multiple_different_environments() { // Act: Create first environment let config1 = create_valid_test_config(&temp_dir, "environment-1"); - let result1 = command.execute(config1); + let result1 = command.execute(config1, temp_dir.path()); assert!(result1.is_ok(), "First environment should be created"); // Act: Create second environment let config2 = create_valid_test_config(&temp_dir, "environment-2"); - let result2 = command.execute(config2); + let result2 = command.execute(config2, temp_dir.path()); assert!(result2.is_ok(), "Second environment should be created"); // Assert: Both environments exist @@ -255,7 +255,7 @@ fn it_should_use_deterministic_timestamps_with_mock_clock() { let config = create_valid_test_config(&temp_dir, "test-env"); // Act - let _result = command.execute(config); + let _result = command.execute(config, temp_dir.path()); // Assert: Clock maintains fixed time assert_eq!(command.clock.now(), fixed_time); diff --git a/src/bin/e2e_tests_full.rs b/src/bin/e2e_tests_full.rs index a6eef243..50a584f1 100644 --- a/src/bin/e2e_tests_full.rs +++ b/src/bin/e2e_tests_full.rs @@ -150,10 +150,14 @@ pub async fn main() -> Result<()> { let repository_factory = RepositoryFactory::new(Duration::from_secs(30)); let clock: Arc = Arc::new(SystemClock); + // Get working directory (current directory for E2E tests) + let working_dir = std::env::current_dir().expect("Failed to get current directory"); + // Create environment via CreateCommandHandler let environment = run_create_command( &repository_factory, clock, + &working_dir, "e2e-full", ssh_private_key_path.to_string_lossy().to_string(), ssh_public_key_path.to_string_lossy().to_string(), diff --git a/src/domain/environment/context.rs b/src/domain/environment/context.rs index 21a1853c..870b8ef0 100644 --- a/src/domain/environment/context.rs +++ b/src/domain/environment/context.rs @@ -162,6 +162,57 @@ impl EnvironmentContext { } } + /// Creates a new environment context with directories relative to a working directory + /// + /// This version creates absolute paths for data and build directories by + /// using the provided working directory as the base. + /// + /// # Arguments + /// + /// * `name` - The environment name + /// * `ssh_credentials` - SSH credentials for accessing the instance + /// * `ssh_port` - SSH port (typically 22) + /// * `working_dir` - The base working directory for operations + /// + /// # Examples + /// + /// ```rust + /// use torrust_tracker_deployer_lib::domain::environment::{EnvironmentContext, EnvironmentName}; + /// use torrust_tracker_deployer_lib::adapters::SshCredentials; + /// use torrust_tracker_deployer_lib::shared::Username; + /// use std::path::PathBuf; + /// + /// let env_name = EnvironmentName::new("production".to_string())?; + /// let username = Username::new("torrust".to_string())?; + /// let ssh_credentials = SshCredentials::new( + /// PathBuf::from("keys/prod_rsa"), + /// PathBuf::from("keys/prod_rsa.pub"), + /// username, + /// ); + /// let working_dir = PathBuf::from("/opt/deployments"); + /// + /// let context = EnvironmentContext::with_working_dir(&env_name, ssh_credentials, 22, &working_dir); + /// + /// assert_eq!(context.user_inputs.instance_name.as_str(), "torrust-tracker-vm-production"); + /// assert_eq!(context.internal_config.data_dir, PathBuf::from("/opt/deployments/data/production")); + /// assert_eq!(context.internal_config.build_dir, PathBuf::from("/opt/deployments/build/production")); + /// + /// # Ok::<(), Box>(()) + /// ``` + #[must_use] + pub fn with_working_dir( + name: &EnvironmentName, + ssh_credentials: SshCredentials, + ssh_port: u16, + working_dir: &std::path::Path, + ) -> Self { + Self { + user_inputs: UserInputs::new(name, ssh_credentials, ssh_port), + internal_config: InternalConfig::with_working_dir(name, working_dir), + runtime_outputs: RuntimeOutputs { instance_ip: None }, + } + } + /// Returns the SSH username for this environment #[must_use] pub fn ssh_username(&self) -> &crate::shared::Username { diff --git a/src/domain/environment/internal_config.rs b/src/domain/environment/internal_config.rs index acf02c04..5bddf5ee 100644 --- a/src/domain/environment/internal_config.rs +++ b/src/domain/environment/internal_config.rs @@ -83,7 +83,7 @@ impl InternalConfig { /// ``` #[must_use] pub fn new(env_name: &EnvironmentName) -> Self { - // Generate environment-specific directories + // Generate environment-specific directories (relative paths) let data_dir = PathBuf::from(DATA_DIR_NAME).join(env_name.as_str()); let build_dir = PathBuf::from(BUILD_DIR_NAME).join(env_name.as_str()); @@ -93,6 +93,50 @@ impl InternalConfig { } } + /// Creates a new `InternalConfig` with directories relative to a working directory + /// + /// This version creates absolute paths by prepending the working directory + /// to the generated data and build directories. + /// + /// # Arguments + /// + /// * `env_name` - The environment name used to generate directories + /// * `working_dir` - The base working directory for operations + /// + /// # Returns + /// + /// A new `InternalConfig` with: + /// - `data_dir`: `{working_dir}/data/{env_name}` + /// - `build_dir`: `{working_dir}/build/{env_name}` + /// + /// # Examples + /// + /// ```rust + /// use torrust_tracker_deployer_lib::domain::environment::internal_config::InternalConfig; + /// use torrust_tracker_deployer_lib::domain::environment::EnvironmentName; + /// use std::path::PathBuf; + /// + /// let env_name = EnvironmentName::new("production".to_string())?; + /// let working_dir = PathBuf::from("/opt/deployments"); + /// let config = InternalConfig::with_working_dir(&env_name, &working_dir); + /// + /// assert_eq!(config.data_dir, PathBuf::from("/opt/deployments/data/production")); + /// assert_eq!(config.build_dir, PathBuf::from("/opt/deployments/build/production")); + /// + /// # Ok::<(), Box>(()) + /// ``` + #[must_use] + pub fn with_working_dir(env_name: &EnvironmentName, working_dir: &std::path::Path) -> Self { + // Generate environment-specific directories relative to working directory + let data_dir = working_dir.join(DATA_DIR_NAME).join(env_name.as_str()); + let build_dir = working_dir.join(BUILD_DIR_NAME).join(env_name.as_str()); + + Self { + build_dir, + data_dir, + } + } + /// Returns the templates directory for this environment /// /// Path: `data/{env_name}/templates/` diff --git a/src/domain/environment/mod.rs b/src/domain/environment/mod.rs index 645f84fa..3cd687b1 100644 --- a/src/domain/environment/mod.rs +++ b/src/domain/environment/mod.rs @@ -259,6 +259,70 @@ impl Environment { state: Created, } } + + /// Creates a new environment in Created state with directories relative to a working directory + /// + /// This version creates absolute paths for data and build directories by + /// using the provided working directory as the base. This is the recommended + /// constructor when the working directory is known at environment creation time. + /// + /// # Arguments + /// + /// * `name` - The unique environment name + /// * `ssh_credentials` - SSH credentials for accessing the provisioned instance + /// * `ssh_port` - SSH port for connections (typically 22) + /// * `working_dir` - The base working directory for all operations + /// + /// # Returns + /// + /// A new environment in the `Created` state with paths relative to the working directory. + /// + /// # Examples + /// + /// ```rust + /// use torrust_tracker_deployer_lib::domain::environment::{Environment, EnvironmentName}; + /// use torrust_tracker_deployer_lib::adapters::SshCredentials; + /// use torrust_tracker_deployer_lib::shared::Username; + /// use std::path::PathBuf; + /// + /// let env_name = EnvironmentName::new("production".to_string())?; + /// let username = Username::new("torrust".to_string())?; + /// let ssh_credentials = SshCredentials::new( + /// PathBuf::from("keys/prod_rsa"), + /// PathBuf::from("keys/prod_rsa.pub"), + /// username, + /// ); + /// let ssh_port = 22; + /// let working_dir = PathBuf::from("/opt/deployments"); + /// let environment = Environment::with_working_dir(env_name, ssh_credentials, ssh_port, &working_dir); + /// + /// assert_eq!(environment.instance_name().as_str(), "torrust-tracker-vm-production"); + /// assert_eq!(*environment.data_dir(), PathBuf::from("/opt/deployments/data/production")); + /// assert_eq!(*environment.build_dir(), PathBuf::from("/opt/deployments/build/production")); + /// + /// # Ok::<(), Box>(()) + /// ``` + /// + /// # Panics + /// + /// This function does not panic. All instance name generation is guaranteed + /// to succeed for valid environment names. + #[must_use] + #[allow(clippy::needless_pass_by_value)] // Public API takes ownership for ergonomics + pub fn with_working_dir( + name: EnvironmentName, + ssh_credentials: SshCredentials, + ssh_port: u16, + working_dir: &std::path::Path, + ) -> Environment { + let context = + EnvironmentContext::with_working_dir(&name, ssh_credentials, ssh_port, working_dir); + + Environment { + context, + state: Created, + } + } } // Common transitions available from any state diff --git a/src/presentation/controllers/create/subcommands/environment/handler.rs b/src/presentation/controllers/create/subcommands/environment/handler.rs index 65e3ab84..45e6d2b5 100644 --- a/src/presentation/controllers/create/subcommands/environment/handler.rs +++ b/src/presentation/controllers/create/subcommands/environment/handler.rs @@ -264,7 +264,7 @@ impl CreateEnvironmentCommandController { let command_handler = self.create_command_handler(working_dir)?; - let environment = self.execute_create_command(&command_handler, config)?; + let environment = self.execute_create_command(&command_handler, config, working_dir)?; self.display_creation_results(&environment)?; @@ -343,7 +343,10 @@ impl CreateEnvironmentCommandController { ) -> Result { self.progress.start_step("Creating command handler")?; - let repository = self.repository_factory.create(working_dir.to_path_buf()); + // Repository expects the BASE data directory, not the working directory + // It will append the environment name to create environment-specific paths + let data_dir = working_dir.join("data"); + let repository = self.repository_factory.create(data_dir); let command_handler = CreateCommandHandler::new(repository, self.clock.clone()); @@ -362,6 +365,7 @@ impl CreateEnvironmentCommandController { /// /// * `command_handler` - Pre-created command handler /// * `config` - Validated environment creation configuration + /// * `working_dir` - Working directory path for environment storage /// /// # Returns /// @@ -374,6 +378,7 @@ impl CreateEnvironmentCommandController { &mut self, command_handler: &CreateCommandHandler, config: EnvironmentCreationConfig, + working_dir: &Path, ) -> Result, CreateEnvironmentCommandError> { self.progress.start_step("Creating environment")?; @@ -386,7 +391,7 @@ impl CreateEnvironmentCommandController { .sub_step("Validating configuration and creating environment...")?; let environment = command_handler - .execute(config) + .execute(config, working_dir) .map_err(|source| CreateEnvironmentCommandError::CommandFailed { source })?; self.progress.complete_step(Some(&format!( diff --git a/src/presentation/controllers/create/subcommands/environment/tests.rs b/src/presentation/controllers/create/subcommands/environment/tests.rs index 0658ead8..9e822160 100644 --- a/src/presentation/controllers/create/subcommands/environment/tests.rs +++ b/src/presentation/controllers/create/subcommands/environment/tests.rs @@ -54,8 +54,8 @@ fn it_should_create_environment_from_valid_config() { ); // Verify environment state file was created by repository - // Repository creates: /{env-name}/environment.json - let env_state_file = working_dir.join("test-create-env/environment.json"); + // Repository creates: /data/{env-name}/environment.json + let env_state_file = working_dir.join("data/test-create-env/environment.json"); assert!( env_state_file.exists(), "Environment state file should be created at: {}", @@ -177,8 +177,8 @@ fn it_should_create_environment_in_custom_working_dir() { assert!(result.is_ok(), "Should create in custom working dir"); // Verify environment was created in custom location - // Repository creates: /{env-name}/environment.json - let env_state_file = custom_working_dir.join("custom-location-env/environment.json"); + // Repository creates: /data/{env-name}/environment.json + let env_state_file = custom_working_dir.join("data/custom-location-env/environment.json"); assert!( env_state_file.exists(), "Environment state should be in custom working directory at: {}", diff --git a/src/presentation/controllers/create/tests/environment.rs b/src/presentation/controllers/create/tests/environment.rs index 0507d007..6dc148f5 100644 --- a/src/presentation/controllers/create/tests/environment.rs +++ b/src/presentation/controllers/create/tests/environment.rs @@ -40,10 +40,10 @@ fn it_should_create_environment_from_valid_config() { ); // Verify environment state file was created by repository - // Repository creates: /{env-name}/environment.json + // Repository creates: /data/{env-name}/environment.json let env_state_file = context .working_dir() - .join("integration-test-env/environment.json"); + .join("data/integration-test-env/environment.json"); assert!( env_state_file.exists(), "Environment state file should be created at: {}", @@ -159,8 +159,8 @@ fn it_should_create_environment_in_custom_working_dir() { assert!(result.is_ok(), "Should create in custom working dir"); // Verify environment was created in custom location - // Repository creates: /{env-name}/environment.json - let env_state_file = custom_working_dir.join("custom-dir-env/environment.json"); + // Repository creates: /data/{env-name}/environment.json + let env_state_file = custom_working_dir.join("data/custom-dir-env/environment.json"); assert!( env_state_file.exists(), "Environment state should be in custom working directory: {}", diff --git a/src/presentation/controllers/destroy/handler.rs b/src/presentation/controllers/destroy/handler.rs index ad47a5bf..8e75f314 100644 --- a/src/presentation/controllers/destroy/handler.rs +++ b/src/presentation/controllers/destroy/handler.rs @@ -222,7 +222,9 @@ impl DestroyCommandController { clock: Arc, user_output: Arc>>, ) -> Self { - let repository = repository_factory.create(working_dir); + // Repository expects BASE data directory, will append environment name internally + let data_dir = working_dir.join("data"); + let repository = repository_factory.create(data_dir); let progress = ProgressReporter::new(user_output.clone(), DESTROY_WORKFLOW_STEPS); Self { diff --git a/src/testing/e2e/context.rs b/src/testing/e2e/context.rs index a4f555d0..bb19fed6 100644 --- a/src/testing/e2e/context.rs +++ b/src/testing/e2e/context.rs @@ -445,8 +445,8 @@ impl TestContext { ) -> std::sync::Arc { // Pass the parent "data" directory, not the environment-specific directory // The repository will add the environment name subdirectory automatically - // e.g., "data" + "e2e-provision" = "data/e2e-provision/environment.json" - let base_data_dir = std::path::PathBuf::from("data"); + // e.g., "{project_root}/data" + "e2e-provision" = "{project_root}/data/e2e-provision/environment.json" + let base_data_dir = self.config.project_root.join("data"); self.services.repository_factory.create(base_data_dir) } } diff --git a/src/testing/e2e/tasks/run_create_command.rs b/src/testing/e2e/tasks/run_create_command.rs index b355544c..fba893b5 100644 --- a/src/testing/e2e/tasks/run_create_command.rs +++ b/src/testing/e2e/tasks/run_create_command.rs @@ -42,6 +42,7 @@ use crate::shared::Clock; /// /// * `repository_factory` - Repository factory for creating environment repositories /// * `clock` - Clock service for timestamp generation +/// * `working_dir` - Working directory for environment storage /// * `environment_name` - Name of the environment to create /// * `ssh_private_key_path` - Path to SSH private key file /// * `ssh_public_key_path` - Path to SSH public key file @@ -58,9 +59,11 @@ use crate::shared::Clock; /// - Configuration is invalid /// - Environment already exists /// - Repository operations fail +#[allow(clippy::too_many_arguments)] pub fn run_create_command( repository_factory: &RepositoryFactory, clock: Arc, + working_dir: &std::path::Path, environment_name: &str, ssh_private_key_path: String, ssh_public_key_path: String, @@ -72,9 +75,8 @@ pub fn run_create_command( "Creating environment via CreateCommandHandler" ); - // Create repository using RepositoryFactory with "data" as base directory - let base_data_dir = std::path::PathBuf::from("data"); - let repository = repository_factory.create(base_data_dir); + // Create repository using RepositoryFactory with working directory + let repository = repository_factory.create(working_dir.to_path_buf()); // Create the command handler let create_command = CreateCommandHandler::new(repository, clock); @@ -94,7 +96,7 @@ pub fn run_create_command( // Execute the command let environment = create_command - .execute(config) + .execute(config, working_dir) .map_err(|source| CreateTaskError::CreationFailed { source })?; info!( diff --git a/tests/support/assertions.rs b/tests/support/assertions.rs index 462b38f5..39fb29d8 100644 --- a/tests/support/assertions.rs +++ b/tests/support/assertions.rs @@ -78,7 +78,7 @@ impl EnvironmentStateAssertions { /// Panics if the data directory or environment JSON file doesn't exist. #[allow(dead_code)] pub fn assert_data_directory_structure(&self, env_name: &str) { - let data_dir = self.workspace_path.join(env_name); + let data_dir = self.workspace_path.join("data").join(env_name); assert!( data_dir.exists(), "Data directory should exist at: {}", @@ -112,7 +112,10 @@ impl EnvironmentStateAssertions { } fn environment_json_path(&self, env_name: &str) -> PathBuf { - self.workspace_path.join(env_name).join("environment.json") + self.workspace_path + .join("data") + .join(env_name) + .join("environment.json") } fn read_environment_json(&self, env_name: &str) -> Result { From 37abc11e2294f531abb23fd8a148f8cd31f59b0e Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 22:14:13 +0000 Subject: [PATCH 16/19] fix: [#174] correct repository factory path in provision controller Fixed Bug #5: Provision controller repository path (same as bugs #2/#3) The provision controller was passing working_dir instead of working_dir.join("data") to repository factory. Changes: - Added: let data_dir = working_dir.join("data"); - Changed repository_factory.create() to use data_dir Verified with complete manual E2E test (Phase 7) and full E2E test suite --- .../controllers/provision/handler.rs | 59 +++++-------------- 1 file changed, 14 insertions(+), 45 deletions(-) diff --git a/src/presentation/controllers/provision/handler.rs b/src/presentation/controllers/provision/handler.rs index 29fae77f..0a1813f5 100644 --- a/src/presentation/controllers/provision/handler.rs +++ b/src/presentation/controllers/provision/handler.rs @@ -13,7 +13,6 @@ use crate::domain::environment::name::EnvironmentName; use crate::domain::environment::repository::EnvironmentRepository; use crate::domain::environment::state::Provisioned; use crate::domain::environment::Environment; -use crate::domain::TemplateManager; use crate::infrastructure::persistence::repository_factory::RepositoryFactory; use crate::presentation::dispatch::context::ExecutionContext; use crate::presentation::views::progress::ProgressReporter; @@ -78,7 +77,6 @@ const PROVISION_WORKFLOW_STEPS: usize = 9; /// # Ok(()) /// # } /// ``` -#[allow(clippy::result_large_err)] // Error contains detailed context for user guidance pub fn handle( environment_name: &str, working_dir: &std::path::Path, @@ -89,7 +87,6 @@ pub fn handle( working_dir, context.repository_factory(), context.clock(), - context.template_manager(), &context.user_output(), ) } @@ -109,7 +106,6 @@ pub fn handle( /// * `working_dir` - Root directory for environment data storage /// * `repository_factory` - Factory for creating environment repositories /// * `clock` - Clock service for timing operations -/// * `template_manager` - Template manager for rendering configuration files /// * `user_output` - Shared user output service for consistent output formatting /// /// # Errors @@ -151,7 +147,7 @@ pub fn handle( /// Direct usage (for testing or specialized scenarios): /// /// ```rust -/// use std::path::{Path, PathBuf}; +/// use std::path::Path; /// use std::sync::Arc; /// use parking_lot::ReentrantMutex; /// use std::cell::RefCell; @@ -160,13 +156,11 @@ pub fn handle( /// use torrust_tracker_deployer_lib::infrastructure::persistence::repository_factory::RepositoryFactory; /// use torrust_tracker_deployer_lib::presentation::controllers::constants::DEFAULT_LOCK_TIMEOUT; /// use torrust_tracker_deployer_lib::shared::SystemClock; -/// use torrust_tracker_deployer_lib::domain::TemplateManager; /// /// let user_output = Arc::new(ReentrantMutex::new(RefCell::new(UserOutput::new(VerbosityLevel::Normal)))); /// let repository_factory = Arc::new(RepositoryFactory::new(DEFAULT_LOCK_TIMEOUT)); /// let clock = Arc::new(SystemClock); -/// let template_manager = Arc::new(TemplateManager::new(PathBuf::from("templates"))); -/// if let Err(e) = provision::handle_provision_command("test-env", Path::new("."), repository_factory, clock, template_manager, &user_output) { +/// if let Err(e) = provision::handle_provision_command("test-env", Path::new("."), repository_factory, clock, &user_output) { /// eprintln!("Provision failed: {e}"); /// eprintln!("Help: {}", e.help()); /// } @@ -178,14 +172,12 @@ pub fn handle_provision_command( working_dir: &std::path::Path, repository_factory: Arc, clock: Arc, - template_manager: Arc, user_output: &Arc>>, ) -> Result, ProvisionSubcommandError> { ProvisionCommandController::new( working_dir.to_path_buf(), repository_factory, clock, - template_manager, user_output.clone(), ) .execute(environment_name) @@ -217,7 +209,6 @@ pub struct ProvisionCommandController { repository: Arc, repository_factory: Arc, clock: Arc, - template_manager: Arc, user_output: Arc>>, progress: ProgressReporter, } @@ -232,17 +223,16 @@ impl ProvisionCommandController { working_dir: std::path::PathBuf, repository_factory: Arc, clock: Arc, - template_manager: Arc, user_output: Arc>>, ) -> Self { - let repository = repository_factory.create(working_dir); + let data_dir = working_dir.join("data"); + let repository = repository_factory.create(data_dir); let progress = ProgressReporter::new(user_output.clone(), PROVISION_WORKFLOW_STEPS); Self { repository, repository_factory, clock, - template_manager, user_output, progress, } @@ -316,17 +306,13 @@ impl ProvisionCommandController { /// Create application layer command handler /// /// Creates the application layer command handler with all required - /// dependencies (repository, clock, template manager, etc.). + /// dependencies (repository, clock). #[allow(clippy::result_large_err)] fn create_command_handler( &mut self, ) -> Result { self.progress.start_step("Creating command handler")?; - let handler = ProvisionCommandHandler::new( - self.clock.clone(), - self.template_manager.clone(), - self.repository.clone(), - ); + let handler = ProvisionCommandHandler::new(self.clock.clone(), self.repository.clone()); self.progress.complete_step(None)?; Ok(handler) @@ -363,7 +349,7 @@ impl ProvisionCommandController { .map_err( |source| ProvisionSubcommandError::ProvisionOperationFailed { name: env_name.to_string(), - source, + source: Box::new(source), }, )?; @@ -397,26 +383,18 @@ mod tests { /// - `user_output`: `ReentrantMutex`-wrapped `UserOutput` for thread-safe access /// - `repository_factory`: Factory for creating environment repositories /// - `clock`: System clock for timing operations - /// - `template_manager`: Template manager for rendering configuration files #[allow(clippy::type_complexity)] // Test helper with complex but clear types fn create_test_dependencies() -> ( Arc>>, Arc, Arc, - Arc, ) { - use tempfile::TempDir; - let (user_output, _, _) = TestUserOutput::new(VerbosityLevel::Normal).into_reentrant_wrapped(); let repository_factory = Arc::new(RepositoryFactory::new(DEFAULT_LOCK_TIMEOUT)); let clock = Arc::new(SystemClock); - // Create a temporary templates directory for testing - let temp_dir = TempDir::new().unwrap(); - let template_manager = Arc::new(TemplateManager::new(temp_dir.path())); - - (user_output, repository_factory, clock, template_manager) + (user_output, repository_factory, clock) } #[test] @@ -425,7 +403,7 @@ mod tests { let temp_dir = TempDir::new().unwrap(); - let (user_output, repository_factory, clock, template_manager) = create_test_dependencies(); + let (user_output, repository_factory, clock) = create_test_dependencies(); // Test with invalid environment name (contains underscore) let result = handle_provision_command( @@ -433,7 +411,6 @@ mod tests { temp_dir.path(), repository_factory, clock, - template_manager, &user_output, ); @@ -452,16 +429,10 @@ mod tests { let temp_dir = TempDir::new().unwrap(); - let (user_output, repository_factory, clock, template_manager) = create_test_dependencies(); + let (user_output, repository_factory, clock) = create_test_dependencies(); - let result = handle_provision_command( - "", - temp_dir.path(), - repository_factory, - clock, - template_manager, - &user_output, - ); + let result = + handle_provision_command("", temp_dir.path(), repository_factory, clock, &user_output); assert!(result.is_err()); match result.unwrap_err() { @@ -478,7 +449,7 @@ mod tests { let temp_dir = TempDir::new().unwrap(); - let (user_output, repository_factory, clock, template_manager) = create_test_dependencies(); + let (user_output, repository_factory, clock) = create_test_dependencies(); // Try to provision an environment that doesn't exist let result = handle_provision_command( @@ -486,7 +457,6 @@ mod tests { temp_dir.path(), repository_factory, clock, - template_manager, &user_output, ); @@ -508,7 +478,7 @@ mod tests { let temp_dir = TempDir::new().unwrap(); let working_dir = temp_dir.path(); - let (user_output, repository_factory, clock, template_manager) = create_test_dependencies(); + let (user_output, repository_factory, clock) = create_test_dependencies(); // Create a mock environment directory to test validation let env_dir = working_dir.join("test-env"); @@ -521,7 +491,6 @@ mod tests { working_dir, repository_factory, clock, - template_manager, &user_output, ); From 9a8c04c639c79fd4563b8ec279f52a859286cbfb Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 17 Nov 2025 22:14:37 +0000 Subject: [PATCH 17/19] refactor: [#174] make TemplateManager per-environment instead of global MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed Bug #6: Templates created at wrong location Problem: - Container created global TemplateManager with hardcoded "templates" path - Templates written to ./templates/ instead of data/{env}/templates/ - Violated multi-environment isolation Solution: - Removed TemplateManager from Container (no longer global) - Removed from ExecutionContext and all handlers - Create per-environment in build_infrastructure_dependencies() - Create per-environment in build_configuration_dependencies() - Uses environment.templates_dir() → data/{env}/templates/ Additional improvements: - Converted build methods to static functions (clippy unused_self) - Boxed large error variant (clippy result_large_err) Files modified: - src/application/command_handlers/provision/handler.rs - src/application/command_handlers/provision/tests/builders.rs - src/bootstrap/container.rs - src/domain/template/embedded.rs - src/presentation/controllers/provision/errors.rs - src/presentation/controllers/provision/handler.rs - src/presentation/controllers/provision/mod.rs - src/presentation/dispatch/context.rs - src/testing/e2e/tasks/virtual_machine/run_provision_command.rs Benefits: - Proper multi-environment isolation - Templates cleaned up with environment - Aligns with design intent - Reduced error variant size Verified: - All 1,468 tests passing - Manual test: templates at data/{env}/templates/ - Pre-commit checks: 100% passed (3m 46s) - Full E2E test: passed (66s) --- .../command_handlers/provision/handler.rs | 27 ++++++++++-------- .../provision/tests/builders.rs | 6 +--- src/bootstrap/container.rs | 25 ----------------- src/domain/template/embedded.rs | 2 +- .../controllers/provision/errors.rs | 6 ++-- src/presentation/controllers/provision/mod.rs | 6 ++-- src/presentation/dispatch/context.rs | 28 ------------------- .../virtual_machine/run_provision_command.rs | 7 ++--- 8 files changed, 24 insertions(+), 83 deletions(-) diff --git a/src/application/command_handlers/provision/handler.rs b/src/application/command_handlers/provision/handler.rs index 8ebac2d4..80276e4b 100644 --- a/src/application/command_handlers/provision/handler.rs +++ b/src/application/command_handlers/provision/handler.rs @@ -21,7 +21,7 @@ use crate::domain::environment::repository::{ }; use crate::domain::environment::state::{ProvisionFailureContext, ProvisionStep}; use crate::domain::environment::{Environment, Provisioned, Provisioning}; -use crate::domain::{EnvironmentName, TemplateManager}; +use crate::domain::EnvironmentName; use crate::infrastructure::external_tools::ansible::AnsibleTemplateRenderer; use crate::infrastructure::external_tools::tofu::TofuTemplateRenderer; use crate::shared::error::Traceable; @@ -53,7 +53,6 @@ use crate::shared::error::Traceable; /// Persistence failures are logged but don't fail the command handler (state remains valid in memory). pub struct ProvisionCommandHandler { clock: Arc, - template_manager: Arc, repository: TypedEnvironmentRepository, } @@ -62,12 +61,10 @@ impl ProvisionCommandHandler { #[must_use] pub fn new( clock: Arc, - template_manager: Arc, repository: Arc, ) -> Self { Self { clock, - template_manager, repository: TypedEnvironmentRepository::new(repository), } } @@ -228,7 +225,7 @@ impl ProvisionCommandHandler { environment: &Environment, ) -> StepResult { let (tofu_template_renderer, opentofu_client) = - self.build_infrastructure_dependencies(environment); + Self::build_infrastructure_dependencies(environment); let current_step = ProvisionStep::RenderOpenTofuTemplates; self.render_opentofu_templates(&tofu_template_renderer) @@ -260,17 +257,21 @@ impl ProvisionCommandHandler { /// - `TofuTemplateRenderer` - For rendering `OpenTofu` templates /// - `OpenTofuClient` - For executing `OpenTofu` operations fn build_infrastructure_dependencies( - &self, environment: &Environment, ) -> (Arc, Arc) { + let opentofu_client = Arc::new(OpenTofuClient::new(environment.tofu_build_dir())); + + let template_manager = Arc::new(crate::domain::TemplateManager::new( + environment.templates_dir(), + )); + let tofu_template_renderer = Arc::new(TofuTemplateRenderer::new( - self.template_manager.clone(), + template_manager, environment.build_dir(), environment.ssh_credentials().clone(), environment.instance_name().clone(), environment.profile_name().clone(), )); - let opentofu_client = Arc::new(OpenTofuClient::new(environment.tofu_build_dir())); (tofu_template_renderer, opentofu_client) } @@ -296,7 +297,7 @@ impl ProvisionCommandHandler { instance_ip: IpAddr, ) -> StepResult<(), ProvisionCommandHandlerError, ProvisionStep> { let (ansible_client, ansible_template_renderer) = - self.build_configuration_dependencies(environment); + Self::build_configuration_dependencies(environment); let ssh_credentials = environment.ssh_credentials(); @@ -332,13 +333,17 @@ impl ProvisionCommandHandler { /// - `AnsibleClient` - For executing Ansible playbooks /// - `AnsibleTemplateRenderer` - For rendering Ansible templates fn build_configuration_dependencies( - &self, environment: &Environment, ) -> (Arc, Arc) { let ansible_client = Arc::new(AnsibleClient::new(environment.ansible_build_dir())); + + let template_manager = Arc::new(crate::domain::TemplateManager::new( + environment.templates_dir(), + )); + let ansible_template_renderer = Arc::new(AnsibleTemplateRenderer::new( environment.build_dir(), - self.template_manager.clone(), + template_manager, )); (ansible_client, ansible_template_renderer) diff --git a/src/application/command_handlers/provision/tests/builders.rs b/src/application/command_handlers/provision/tests/builders.rs index 523a3b22..5876f101 100644 --- a/src/application/command_handlers/provision/tests/builders.rs +++ b/src/application/command_handlers/provision/tests/builders.rs @@ -49,10 +49,6 @@ impl ProvisionCommandHandlerTestBuilder { /// The `temp_dir` must be kept alive for the duration of the test. #[allow(dead_code)] pub fn build(self) -> (ProvisionCommandHandler, TempDir, SshCredentials) { - let template_manager = Arc::new(crate::domain::template::TemplateManager::new( - self.temp_dir.path(), - )); - // Use provided SSH credentials or create defaults let ssh_credentials = self.ssh_credentials.unwrap_or_else(|| { let ssh_key_path = self.temp_dir.path().join("test_key"); @@ -69,7 +65,7 @@ impl ProvisionCommandHandlerTestBuilder { let repository_factory = RepositoryFactory::new(std::time::Duration::from_secs(30)); let repository = repository_factory.create(self.temp_dir.path().to_path_buf()); - let command_handler = ProvisionCommandHandler::new(clock, template_manager, repository); + let command_handler = ProvisionCommandHandler::new(clock, repository); (command_handler, self.temp_dir, ssh_credentials) } diff --git a/src/bootstrap/container.rs b/src/bootstrap/container.rs index 32833c76..a7492f25 100644 --- a/src/bootstrap/container.rs +++ b/src/bootstrap/container.rs @@ -8,7 +8,6 @@ use std::sync::Arc; use parking_lot::ReentrantMutex; -use crate::domain::TemplateManager; use crate::infrastructure::persistence::repository_factory::RepositoryFactory; use crate::presentation::controllers::constants::DEFAULT_LOCK_TIMEOUT; use crate::presentation::views::{UserOutput, VerbosityLevel}; @@ -36,7 +35,6 @@ pub struct Container { user_output: Arc>>, repository_factory: Arc, clock: Arc, - template_manager: Arc, } impl Container { @@ -70,14 +68,11 @@ impl Container { )))); let repository_factory = Arc::new(RepositoryFactory::new(DEFAULT_LOCK_TIMEOUT)); let clock: Arc = Arc::new(SystemClock); - let template_manager = - Arc::new(TemplateManager::new(std::path::PathBuf::from("templates"))); Self { user_output, repository_factory, clock, - template_manager, } } @@ -142,26 +137,6 @@ impl Container { pub fn clock(&self) -> Arc { Arc::clone(&self.clock) } - - /// Get shared reference to template manager service - /// - /// Returns an `Arc` that can be cheaply cloned and shared - /// across threads and function calls. - /// - /// # Example - /// - /// ```rust - /// use torrust_tracker_deployer_lib::bootstrap::container::Container; - /// use torrust_tracker_deployer_lib::presentation::views::VerbosityLevel; - /// - /// let container = Container::new(VerbosityLevel::Normal); - /// let template_manager = container.template_manager(); - /// // Use template_manager for template operations - /// ``` - #[must_use] - pub fn template_manager(&self) -> Arc { - Arc::clone(&self.template_manager) - } } impl Default for Container { diff --git a/src/domain/template/embedded.rs b/src/domain/template/embedded.rs index d5d1601f..6449ab20 100644 --- a/src/domain/template/embedded.rs +++ b/src/domain/template/embedded.rs @@ -105,7 +105,7 @@ impl TemplateManager { /// Clean and prepare the templates directory to ensure fresh embedded templates /// /// This method combines `clean_templates_dir()` and `ensure_templates_dir()` to provide - /// a clean slate for template operations. It's particularly useful in testing and + /// a clean state for template operations. It's particularly useful in testing and /// development environments where you want to ensure fresh templates from embedded resources. /// /// # Errors diff --git a/src/presentation/controllers/provision/errors.rs b/src/presentation/controllers/provision/errors.rs index 6d36f66b..b49eef77 100644 --- a/src/presentation/controllers/provision/errors.rs +++ b/src/presentation/controllers/provision/errors.rs @@ -73,7 +73,7 @@ Tip: Check logs and try running with --log-output file-and-stderr for more detai ProvisionOperationFailed { name: String, #[source] - source: ProvisionCommandHandlerError, + source: Box, }, // ===== Internal Errors ===== @@ -140,13 +140,11 @@ impl ProvisionSubcommandError { /// use torrust_tracker_deployer_lib::presentation::views::{UserOutput, VerbosityLevel}; /// use torrust_tracker_deployer_lib::infrastructure::persistence::repository_factory::RepositoryFactory; /// use torrust_tracker_deployer_lib::shared::clock::SystemClock; - /// use torrust_tracker_deployer_lib::domain::TemplateManager; /// /// let output = Arc::new(ReentrantMutex::new(RefCell::new(UserOutput::new(VerbosityLevel::Normal)))); /// let repository_factory = Arc::new(RepositoryFactory::new(Duration::from_secs(30))); /// let clock = Arc::new(SystemClock); - /// let template_manager = Arc::new(TemplateManager::new(PathBuf::from("templates"))); - /// if let Err(e) = provision::handle_provision_command("test-env", Path::new("."), repository_factory, clock, template_manager, &output) { + /// if let Err(e) = provision::handle_provision_command("test-env", Path::new("."), repository_factory, clock, &output) { /// eprintln!("Error: {e}"); /// eprintln!("\nTroubleshooting:\n{}", e.help()); /// } diff --git a/src/presentation/controllers/provision/mod.rs b/src/presentation/controllers/provision/mod.rs index 52ce4c50..74e08ef5 100644 --- a/src/presentation/controllers/provision/mod.rs +++ b/src/presentation/controllers/provision/mod.rs @@ -55,7 +55,7 @@ //! ## Direct Usage (For Testing) //! //! ```rust -//! use std::path::{Path, PathBuf}; +//! use std::path::Path; //! use std::sync::Arc; //! use std::time::Duration; //! use parking_lot::ReentrantMutex; @@ -64,13 +64,11 @@ //! use torrust_tracker_deployer_lib::presentation::views::{UserOutput, VerbosityLevel}; //! use torrust_tracker_deployer_lib::infrastructure::persistence::repository_factory::RepositoryFactory; //! use torrust_tracker_deployer_lib::shared::clock::SystemClock; -//! use torrust_tracker_deployer_lib::domain::TemplateManager; //! //! let output = Arc::new(ReentrantMutex::new(RefCell::new(UserOutput::new(VerbosityLevel::Normal)))); //! let repository_factory = Arc::new(RepositoryFactory::new(Duration::from_secs(30))); //! let clock = Arc::new(SystemClock); -//! let template_manager = Arc::new(TemplateManager::new(PathBuf::from("templates"))); -//! if let Err(e) = provision::handle_provision_command("test-env", Path::new("."), repository_factory, clock, template_manager, &output) { +//! if let Err(e) = provision::handle_provision_command("test-env", Path::new("."), repository_factory, clock, &output) { //! eprintln!("Provision failed: {e}"); //! eprintln!("\n{}", e.help()); //! } diff --git a/src/presentation/dispatch/context.rs b/src/presentation/dispatch/context.rs index 1e5dc2e4..cfb718e5 100644 --- a/src/presentation/dispatch/context.rs +++ b/src/presentation/dispatch/context.rs @@ -49,7 +49,6 @@ use std::sync::Arc; use parking_lot::ReentrantMutex; use crate::bootstrap::Container; -use crate::domain::TemplateManager; use crate::infrastructure::persistence::repository_factory::RepositoryFactory; use crate::presentation::views::UserOutput; use crate::shared::clock::Clock; @@ -214,31 +213,4 @@ impl ExecutionContext { pub fn clock(&self) -> Arc { self.container.clock() } - - /// Get shared reference to template manager service - /// - /// Returns the template manager service for template operations. - /// The service is wrapped in `Arc` for shared access. - /// - /// # Examples - /// - /// ```rust,no_run - /// use torrust_tracker_deployer_lib::bootstrap::Container; - /// use torrust_tracker_deployer_lib::presentation::views::VerbosityLevel; - /// use torrust_tracker_deployer_lib::presentation::dispatch::ExecutionContext; - /// use std::sync::Arc; - /// - /// # fn example() -> Result<(), Box> { - /// let container = Container::new(VerbosityLevel::Normal); - /// let context = ExecutionContext::new(Arc::new(container)); - /// - /// let template_manager = context.template_manager(); - /// // Use template_manager for template operations - /// # Ok(()) - /// # } - /// ``` - #[must_use] - pub fn template_manager(&self) -> Arc { - self.container.template_manager() - } } diff --git a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs index 9b40fba4..d99b3f56 100644 --- a/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs +++ b/src/testing/e2e/tasks/virtual_machine/run_provision_command.rs @@ -151,11 +151,8 @@ pub async fn run_provision_command( .create(base_data_dir); // Use the new ProvisionCommandHandler to handle all infrastructure provisioning steps - let provision_command_handler = ProvisionCommandHandler::new( - Arc::clone(&test_context.services.clock), - Arc::clone(&test_context.services.template_manager), - repository, - ); + let provision_command_handler = + ProvisionCommandHandler::new(Arc::clone(&test_context.services.clock), repository); // Execute provisioning - application layer handles state validation let env_name = test_context.environment.name(); From 527b6e0cb743982942fbbe5ca433a5d1dd417ab8 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 18 Nov 2025 07:23:14 +0000 Subject: [PATCH 18/19] fix: [#174] fix E2E test repository path (Bug #7) Bug #7: E2E test was creating environment.json at repository root instead of in data/ directory. Root cause: run_create_command was passing working_dir directly to RepositoryFactory.create() instead of working_dir.join("data"). Impact: - E2E tests created ./e2e-full/environment.json at repo root - This caused cspell errors (username in paths) - Files were tracked by git (not in .gitignore) Fix: Pass working_dir.join("data") to repository factory, matching the pattern used in provision controller (Bug #5 fix). This is the same category of bug as #2, #3, and #5 - passing working_dir instead of data_dir to RepositoryFactory. --- e2e-full/environment.json | 25 --------------------- src/testing/e2e/tasks/run_create_command.rs | 5 +++-- 2 files changed, 3 insertions(+), 27 deletions(-) delete mode 100644 e2e-full/environment.json diff --git a/e2e-full/environment.json b/e2e-full/environment.json deleted file mode 100644 index 265e6409..00000000 --- a/e2e-full/environment.json +++ /dev/null @@ -1,25 +0,0 @@ -{ - "Created": { - "context": { - "user_inputs": { - "name": "e2e-full", - "instance_name": "torrust-tracker-vm-e2e-full", - "profile_name": "torrust-profile-e2e-full", - "ssh_credentials": { - "ssh_priv_key_path": "/home/josecelano/Documents/git/committer/me/github/torrust/torrust-tracker-deployer-agent-01/fixtures/testing_rsa", - "ssh_pub_key_path": "/home/josecelano/Documents/git/committer/me/github/torrust/torrust-tracker-deployer-agent-01/fixtures/testing_rsa.pub", - "ssh_username": "torrust" - }, - "ssh_port": 22 - }, - "internal_config": { - "build_dir": "/home/josecelano/Documents/git/committer/me/github/torrust/torrust-tracker-deployer-agent-01/build/e2e-full", - "data_dir": "/home/josecelano/Documents/git/committer/me/github/torrust/torrust-tracker-deployer-agent-01/data/e2e-full" - }, - "runtime_outputs": { - "instance_ip": null - } - }, - "state": null - } -} \ No newline at end of file diff --git a/src/testing/e2e/tasks/run_create_command.rs b/src/testing/e2e/tasks/run_create_command.rs index fba893b5..23fd169b 100644 --- a/src/testing/e2e/tasks/run_create_command.rs +++ b/src/testing/e2e/tasks/run_create_command.rs @@ -75,8 +75,9 @@ pub fn run_create_command( "Creating environment via CreateCommandHandler" ); - // Create repository using RepositoryFactory with working directory - let repository = repository_factory.create(working_dir.to_path_buf()); + // Create repository using RepositoryFactory with data directory + let data_dir = working_dir.join("data"); + let repository = repository_factory.create(data_dir); // Create the command handler let create_command = CreateCommandHandler::new(repository, clock); From e2cf68f2fdc414522361809988c967ee0861cd2b Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 18 Nov 2025 08:43:58 +0000 Subject: [PATCH 19/19] fix: [#164] standardize all relative paths to use explicit ./ prefix Ensure consistent path formatting across all code paths: - InternalConfig::new() now uses PathBuf::from(".").join() pattern - E2E tests use PathBuf::from(".") instead of current_dir() - All environment states use ./data/... and ./build/... format - Updated 9 doctest assertions to expect new format - Added ADR documenting the relative paths decision This eliminates the inconsistency where different constructors produced different path formats (data/... vs ./data/...) --- .../relative-paths-in-environment-state.md | 184 ++++++++++++++++++ src/bin/e2e_tests_full.rs | 5 +- src/domain/environment/context.rs | 4 +- src/domain/environment/internal_config.rs | 18 +- src/domain/environment/mod.rs | 22 +-- 5 files changed, 211 insertions(+), 22 deletions(-) create mode 100644 docs/decisions/relative-paths-in-environment-state.md diff --git a/docs/decisions/relative-paths-in-environment-state.md b/docs/decisions/relative-paths-in-environment-state.md new file mode 100644 index 00000000..a86a31ba --- /dev/null +++ b/docs/decisions/relative-paths-in-environment-state.md @@ -0,0 +1,184 @@ +# Decision: Use Relative Paths in Environment State + +## Status + +Accepted + +## Date + +2025-11-18 + +## Context + +The application persists environment state to JSON files, including `build_dir` and `data_dir` paths. We discovered an inconsistency between how the CLI and E2E tests store these paths: + +- **CLI/Manual usage**: Uses relative paths (default `working_dir = "."`) + - Results in: `./data/env-name`, `./build/env-name` +- **E2E tests**: Uses absolute paths (`std::env::current_dir()`) + - Results in: `/home/user/project/data/env-name`, `/home/user/project/build/env-name` + +This inconsistency was discovered when comparing manual test state files with E2E test state files. The environment state is an internal database that persists: + +- Environment name and instance identifiers +- SSH credentials (using absolute paths - user-controlled external resources) +- Build and data directories (application-managed internal resources) +- Runtime outputs like instance IP addresses + +The question is: should `build_dir` and `data_dir` be stored as relative or absolute paths? + +## Decision + +**Use relative paths for `build_dir` and `data_dir` in persisted environment state.** + +We will: + +1. **Fix E2E tests** to use `PathBuf::from(".")` instead of `std::env::current_dir()` +2. **Ensure consistency** between CLI and E2E test behavior +3. **Keep SSH credential paths absolute** as they reference user-controlled external resources + +The `InternalConfig::with_working_dir()` method will continue to join paths as-is (preserving the relativity/absoluteness of the input `working_dir`), but all callers will use relative paths. + +## Consequences + +### Positive + +✅ **Portability** - Environments can be moved to new locations + +- Copy entire workspace from `/opt/deployments` to `/home/user/deployments` +- State files remain valid without modification +- No path fixup required + +✅ **Environment Independence** - Not tied to specific users or systems + +- State files don't contain `/home/username/...` paths +- Works across different developers' machines +- No user-specific information in state files + +✅ **Backup/Restore Friendly** - State is location-agnostic + +- Backup `data/` directory +- Restore to any location +- Environment state travels with workspace + +✅ **Version Control Friendly** - Consistent across all developers + +- Relative paths don't expose user-specific information +- Same state file format for everyone +- Easier code reviews (no path noise) + +✅ **Container/Docker Ready** - Works in ephemeral environments + +- Mount workspace at any path (`/app`, `/workspace`, etc.) +- Relative paths work regardless of mount point +- No path remapping needed + +✅ **Testing Friendly** - Tests can run anywhere + +- CI/CD can use `/tmp/ci-build`, `/github/workspace`, etc. +- Local development in any directory +- No assumptions about absolute locations + +✅ **CLI Consistency** - Matches CLI default behavior + +- CLI uses `working_dir = "."` by default +- E2E tests now behave identically to CLI +- Predictable behavior across all use cases + +### Negative + +⚠️ **Current Directory Dependency** - Must run from correct directory + +- Users must `cd /path/to/project` before running commands +- Mitigated by: `--working-dir` CLI flag for flexibility +- Acceptable trade-off for portability benefits + +⚠️ **Path Resolution at Runtime** - Relative paths resolved during operations + +- Minimal overhead - standard library handles efficiently +- Not a performance concern in practice + +### Design Rationale + +**Why relative for internal resources, absolute for external:** + +- **SSH credentials** (absolute): User-controlled external files + + - User chooses where to store keys (`~/.ssh/id_rsa`, `/opt/keys/deploy.key`) + - Application doesn't manage these files + - Must reference exact location + +- **Build/data directories** (relative): Application-managed internal resources + - Application creates and manages these directories + - Part of the workspace that moves together + - Should be portable with the environment state + +This separation of concerns ensures: + +- External resources referenced by user-chosen absolute paths +- Internal resources use relative paths for portability +- Clear distinction between user-controlled and app-controlled resources + +## Alternatives Considered + +### Alternative 1: Use Absolute Paths + +**Pros:** + +- Explicit location - no ambiguity +- Works from any directory without `cd` + +**Cons:** + +- ❌ Not portable - tied to specific filesystem location +- ❌ User-specific paths in state files +- ❌ Cannot move workspace without breaking state +- ❌ Different paths for each developer +- ❌ Container/Docker issues (hardcoded paths don't exist) +- ❌ Backup/restore requires same absolute path + +**Decision:** Rejected due to portability concerns. The CLI already has `--working-dir` flag for cases where users need to run from different directories. + +### Alternative 2: Normalize Absolute to Relative in Domain Layer + +Add normalization logic in `InternalConfig::with_working_dir()` to convert absolute paths to relative: + +```rust +pub fn with_working_dir(env_name: &EnvironmentName, working_dir: &Path) -> Self { + let normalized = if working_dir.is_absolute() { + std::env::current_dir() + .ok() + .and_then(|cur| working_dir.strip_prefix(&cur).ok()) + .map(|rel| rel.to_path_buf()) + .unwrap_or_else(|| PathBuf::from(".")) + } else { + working_dir.to_path_buf() + }; + // ... use normalized path +} +``` + +**Pros:** + +- Defensive programming - handles both cases +- More robust to caller mistakes + +**Cons:** + +- Added complexity in domain layer +- Hides the problem instead of fixing it at the source +- Unnecessary - callers should just use relative paths + +**Decision:** Not implemented initially. We fix the E2E test to use relative paths like the CLI. If future use cases require absolute paths, we can add this normalization logic as defensive programming. + +## Related Decisions + +- [Environment Variable Prefix](./environment-variable-prefix.md) - Consistent naming for configuration +- [Error Context Strategy](./error-context-strategy.md) - How we persist trace information + +## References + +- Issue discovered in manual Phase 7 testing for provision command +- Analysis in conversation: relative vs absolute paths pros/cons +- File: `src/domain/environment/internal_config.rs` - Path construction logic +- File: `src/bin/e2e_tests_full.rs` - E2E test working directory setup +- File: `src/presentation/input/cli/args.rs` - CLI default `working_dir = "."` diff --git a/src/bin/e2e_tests_full.rs b/src/bin/e2e_tests_full.rs index 50a584f1..32c95d28 100644 --- a/src/bin/e2e_tests_full.rs +++ b/src/bin/e2e_tests_full.rs @@ -55,6 +55,7 @@ use anyhow::Result; use clap::Parser; +use std::path::PathBuf; use std::sync::Arc; use std::time::{Duration, Instant}; use torrust_dependency_installer::{verify_dependencies, Dependency}; @@ -150,8 +151,8 @@ pub async fn main() -> Result<()> { let repository_factory = RepositoryFactory::new(Duration::from_secs(30)); let clock: Arc = Arc::new(SystemClock); - // Get working directory (current directory for E2E tests) - let working_dir = std::env::current_dir().expect("Failed to get current directory"); + // Use relative path like CLI default for consistency + let working_dir = PathBuf::from("."); // Create environment via CreateCommandHandler let environment = run_create_command( diff --git a/src/domain/environment/context.rs b/src/domain/environment/context.rs index 870b8ef0..88c43d6d 100644 --- a/src/domain/environment/context.rs +++ b/src/domain/environment/context.rs @@ -143,8 +143,8 @@ impl EnvironmentContext { /// /// assert_eq!(context.user_inputs.instance_name.as_str(), "torrust-tracker-vm-production"); /// assert_eq!(context.user_inputs.profile_name.as_str(), "torrust-profile-production"); - /// assert_eq!(context.internal_config.data_dir, PathBuf::from("data/production")); - /// assert_eq!(context.internal_config.build_dir, PathBuf::from("build/production")); + /// assert_eq!(context.internal_config.data_dir, PathBuf::from("./data/production")); + /// assert_eq!(context.internal_config.build_dir, PathBuf::from("./build/production")); /// /// # Ok::<(), Box>(()) /// ``` diff --git a/src/domain/environment/internal_config.rs b/src/domain/environment/internal_config.rs index 5bddf5ee..c04c0e30 100644 --- a/src/domain/environment/internal_config.rs +++ b/src/domain/environment/internal_config.rs @@ -64,8 +64,8 @@ impl InternalConfig { /// # Returns /// /// A new `InternalConfig` with: - /// - `data_dir`: `data/{env_name}` - /// - `build_dir`: `build/{env_name}` + /// - `data_dir`: `./data/{env_name}` + /// - `build_dir`: `./build/{env_name}` /// /// # Examples /// @@ -77,15 +77,19 @@ impl InternalConfig { /// let env_name = EnvironmentName::new("production".to_string())?; /// let config = InternalConfig::new(&env_name); /// - /// assert_eq!(config.data_dir, PathBuf::from("data/production")); - /// assert_eq!(config.build_dir, PathBuf::from("build/production")); + /// assert_eq!(config.data_dir, PathBuf::from("./data/production")); + /// assert_eq!(config.build_dir, PathBuf::from("./build/production")); /// # Ok::<(), Box>(()) /// ``` #[must_use] pub fn new(env_name: &EnvironmentName) -> Self { - // Generate environment-specific directories (relative paths) - let data_dir = PathBuf::from(DATA_DIR_NAME).join(env_name.as_str()); - let build_dir = PathBuf::from(BUILD_DIR_NAME).join(env_name.as_str()); + let data_dir = PathBuf::from(".") + .join(DATA_DIR_NAME) + .join(env_name.as_str()); + + let build_dir = PathBuf::from(".") + .join(BUILD_DIR_NAME) + .join(env_name.as_str()); Self { build_dir, diff --git a/src/domain/environment/mod.rs b/src/domain/environment/mod.rs index 3cd687b1..1bddcd64 100644 --- a/src/domain/environment/mod.rs +++ b/src/domain/environment/mod.rs @@ -85,9 +85,9 @@ //! let environment = Environment::new(env_name, ssh_credentials, 22); //! //! // Environment automatically generates paths -//! assert_eq!(*environment.data_dir(), PathBuf::from("data/e2e-config")); -//! assert_eq!(*environment.build_dir(), PathBuf::from("build/e2e-config")); -//! assert_eq!(environment.templates_dir(), PathBuf::from("data/e2e-config/templates")); +//! assert_eq!(*environment.data_dir(), PathBuf::from("./data/e2e-config")); +//! assert_eq!(*environment.build_dir(), PathBuf::from("./build/e2e-config")); +//! assert_eq!(environment.templates_dir(), PathBuf::from("./data/e2e-config/templates")); //! //! # Ok::<(), Box>(()) //! ``` @@ -235,8 +235,8 @@ impl Environment { /// let environment = Environment::new(env_name, ssh_credentials, ssh_port); /// /// assert_eq!(environment.instance_name().as_str(), "torrust-tracker-vm-production"); - /// assert_eq!(*environment.data_dir(), PathBuf::from("data/production")); - /// assert_eq!(*environment.build_dir(), PathBuf::from("build/production")); + /// assert_eq!(*environment.data_dir(), PathBuf::from("./data/production")); + /// assert_eq!(*environment.build_dir(), PathBuf::from("./build/production")); /// /// # Ok::<(), Box>(()) /// ``` @@ -582,7 +582,7 @@ impl Environment { /// /// assert_eq!( /// environment.templates_dir(), - /// PathBuf::from("data/staging/templates") + /// PathBuf::from("./data/staging/templates") /// ); /// /// # Ok::<(), Box>(()) @@ -616,7 +616,7 @@ impl Environment { /// /// assert_eq!( /// environment.traces_dir(), - /// PathBuf::from("data/production/traces") + /// PathBuf::from("./data/production/traces") /// ); /// /// # Ok::<(), Box>(()) @@ -647,7 +647,7 @@ impl Environment { /// /// assert_eq!( /// environment.ansible_build_dir(), - /// PathBuf::from("build/dev/ansible") + /// PathBuf::from("./build/dev/ansible") /// ); /// /// # Ok::<(), Box>(()) @@ -678,7 +678,7 @@ impl Environment { /// /// assert_eq!( /// environment.tofu_build_dir(), - /// PathBuf::from("build/test/tofu/lxd") + /// PathBuf::from("./build/test/tofu/lxd") /// ); /// /// # Ok::<(), Box>(()) @@ -709,7 +709,7 @@ impl Environment { /// /// assert_eq!( /// environment.ansible_templates_dir(), - /// PathBuf::from("data/integration/templates/ansible") + /// PathBuf::from("./data/integration/templates/ansible") /// ); /// /// # Ok::<(), Box>(()) @@ -740,7 +740,7 @@ impl Environment { /// /// assert_eq!( /// environment.tofu_templates_dir(), - /// PathBuf::from("data/load-test/templates/tofu") + /// PathBuf::from("./data/load-test/templates/tofu") /// ); /// /// # Ok::<(), Box>(())