Merge #47: Update E2E full tests to use CreateCommandHandler and fix preflight cleanup

0f3f76a refactor: [#39] move cleanup_previous_test_data to preflight_cleanup module (Jose Celano)
0e7d5a0 fix: [#37] add data directory cleanup to E2E preflight (copilot-swe-agent[bot])
ce59c6a refactor: [#37] move create_environment_via_command to e2e tasks module (copilot-swe-agent[bot])
e5f2fb0 refactor: [#37] improve error handling in create_environment_via_command (copilot-swe-agent[bot])
4455c9b feat: [#37] update e2e-tests-full to use CreateCommandHandler (copilot-swe-agent[bot])
c0f248d Initial plan (copilot-swe-agent[bot])

Pull request description:

  Subissue 5/7 of #34 - The E2E full test suite was creating environments directly via `Environment::new()` instead of exercising the new `CreateCommandHandler`, missing test coverage of the command layer. Additionally, the E2E preflight cleanup was not removing stale environment data from previous test runs, causing test failures.

  ## Changes

  - **Created `run_create_command` task module**: New module in `src/testing/e2e/tasks/run_create_command.rs` that encapsulates environment creation using `CreateCommandHandler`, following the established pattern of other E2E tasks like `run_configure_command`
  - **Replaced direct environment creation**: Main test flow in `e2e_tests_full.rs` now uses the task module instead of `Environment::new()`, exercising the complete command pattern workflow
  - **Improved error handling**: Task module includes comprehensive error handling with `CreateTaskError` enum and `help()` method for detailed troubleshooting guidance
  - **Consistent patterns**: Uses `RepositoryFactory` and `Clock` as parameters, and `"data"` as base directory, matching the pattern in `TestContext.create_repository()`
  - **Added data directory cleanup**: New `cleanup_data_environment()` function in `src/testing/e2e/tasks/preflight_cleanup.rs` that removes `data/{environment_name}` directories from previous test runs, preventing "environment already exists" errors and ensuring proper test isolation

  ## Module Structure

  The new task follows the same structure as other E2E tasks:
  - Located in `src/testing/e2e/tasks/run_create_command.rs`
  - Exported from `src/testing/e2e/tasks/mod.rs`
  - Documented as an infrastructure-agnostic task

  ## Preflight Cleanup Enhancement

  The preflight cleanup now includes data directory removal in addition to build and templates cleanup:
  - **Container preflight** (`src/testing/e2e/tasks/container/preflight_cleanup.rs`): Calls `cleanup_data_environment()` before Docker cleanup
  - **VM preflight** (`src/testing/e2e/tasks/virtual_machine/preflight_cleanup.rs`): Calls `cleanup_data_environment()` before OpenTofu/LXD cleanup

  This ensures E2E tests always start with a clean slate, preventing failures from stale environment state left by interrupted previous test runs.

  ## Example

  ```rust
  // Before: Direct construction bypassed command layer
  let environment = Environment::new(environment_name, ssh_credentials, ssh_port);

  // After: Exercises CreateCommandHandler via task module with full validation and persistence
  let repository_factory = RepositoryFactory::new(Duration::from_secs(30));
  let clock: Arc<dyn Clock> = Arc::new(SystemClock);

  let environment = run_create_command(
      &repository_factory,
      clock,
      "e2e-full",
      ssh_private_key_path.to_string_lossy().to_string(),
      ssh_public_key_path.to_string_lossy().to_string(),
      "torrust",
      DEFAULT_SSH_PORT,
  )?;
  ```

  The E2E test suite now validates the command handler end-to-end through a properly structured task module, ensuring repository persistence and configuration validation work correctly in real deployment scenarios while maintaining consistency with the existing E2E testing architecture. The enhanced preflight cleanup guarantees reliable test execution by removing all artifacts from previous runs.

  - Fixes #39

  <!-- START COPILOT CODING AGENT SUFFIX -->

  <details>

  <summary>Original prompt</summary>

  >
  > ----
  >
  > *This section details on the original issue you should resolve*
  >
  > <issue_title>[Subissue 5/7] Update E2E Full Tests to Use Create Command</issue_title>
  > <issue_description>**Parent Epic**: #34 - Implement Create Environment Command
  > **Depends On**: #36 - Application Layer Command
  > **Estimated Time**: 1-2 hours
  >
  > ## Overview
  >
  > Update the `src/bin/e2e_tests_full.rs` to use the new create command handler instead of direct environment creation. This ensures the full E2E test exercises the complete create command functionality as part of the comprehensive test suite.
  >
  > ## Goals
  >
  > - [ ] Add new function to create environment using CreateCommand handler
  > - [ ] Update e2e_tests_full.rs to use the new create command for environment creation
  > - [ ] Maintain existing test flow while exercising create command logic
  > - [ ] Ensure comprehensive test coverage of create command in full E2E context
  > - [ ] Preserve existing test reliability and performance
  >
  > ## Implementation Summary
  >
  > - **Location**: `src/bin/e2e_tests_full.rs`
  > - **Approach**: Add `create_environment_via_command()` function that uses CreateCommand handler
  > - **Integration**: Replace direct environment creation with command-based creation
  > - **Testing**: Not black-box like Subissue 4 - this uses the command handler directly
  >
  > ## Key Changes
  >
  > ```rust
  > // Add function to create environment using CreateCommand
  > async fn create_environment_via_command(context: &TestContext) -> Result<Environment<Created>> {
  >     let create_command = CreateCommand::new(context.environment_repository.clone());
  >
  >     let config = EnvironmentCreationConfig {
  >         environment_name: context.environment_name.clone(),
  >         ssh_credentials: context.ssh_credentials.clone(),
  >     };
  >
  >     create_command.execute(config)
  > }
  >
  > // Replace direct environment creation in main test function
  > let environment = create_environment_via_command(&context)
  >     .await
  >     .expect("Failed to create environment via command");
  > ```
  >
  > ## Acceptance Criteria
  >
  > - [ ] E2E full tests use CreateCommand handler for environment creation
  > - [ ] All existing E2E test functionality remains intact
  > - [ ] Create command is properly exercised in comprehensive test suite
  > - [ ] Test execution time and reliability maintained
  > - [ ] Proper error handling and logging preserved
  >
  > For detailed specification, see: [docs/issues/epic-create-environment-command-subissue-5-update-e2e-full-tests.md](https://github.com/torrust/torrust-tracker-deployer/blob/main/docs/issues/epic-create-environment-command-subissue-5-update-e2e-full-tests.md)</issue_description>
  >
  > ## Comments on the Issue (you are @copilot in this section)
  >
  > <comments>
  > </comments>
  >

  </details>

  - Fixes #39

  <!-- START COPILOT CODING AGENT TIPS -->
  ---

  💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs.

ACKs for top commit:
  josecelano:
    ACK 0f3f76a

Tree-SHA512: 6f4ce20948a363d73fdf63e57193556d8a0ac9bdb06d01a4c7563c4cd51d649b9d8c8a0c1dd582f21c98c0a1c55509b027035551a8083bfdb2070a95c24558b0

Author: josecelano

src/bin/e2e_tests_full.rs

@@ -55,17 +55,20 @@
 
 use anyhow::Result;
 use clap::Parser;
-use std::time::Instant;
+use std::sync::Arc;
+use std::time::{Duration, Instant};
 use tracing::{error, info};
 
 // Import E2E testing infrastructure
-use torrust_tracker_deployer_lib::adapters::ssh::{SshCredentials, DEFAULT_SSH_PORT};
-use torrust_tracker_deployer_lib::domain::{Environment, EnvironmentName};
+use torrust_tracker_deployer_lib::adapters::ssh::DEFAULT_SSH_PORT;
+use torrust_tracker_deployer_lib::infrastructure::persistence::repository_factory::RepositoryFactory;
 use torrust_tracker_deployer_lib::logging::{LogFormat, LogOutput, LoggingBuilder};
-use torrust_tracker_deployer_lib::shared::Username;
+use torrust_tracker_deployer_lib::shared::{Clock, SystemClock};
 use torrust_tracker_deployer_lib::testing::e2e::context::{TestContext, TestContextType};
 use torrust_tracker_deployer_lib::testing::e2e::tasks::{
+    preflight_cleanup::cleanup_previous_test_data,
     run_configure_command::run_configure_command,
+    run_create_command::run_create_command,
     run_test_command::run_test_command,
     virtual_machine::{
         preflight_cleanup::preflight_cleanup_previous_resources,
@@ -109,6 +112,7 @@ struct Cli {
 ///
 /// May panic during the match statement if unexpected error combinations occur
 /// that are not handled by the current error handling logic.
+#[allow(clippy::too_many_lines)]
 #[tokio::main]
 pub async fn main() -> Result<()> {
     let cli = Cli::parse();
@@ -127,29 +131,38 @@ pub async fn main() -> Result<()> {
         "Starting E2E tests"
     );
 
-    // Create environment entity for e2e-full testing
-    let environment_name = EnvironmentName::new("e2e-full".to_string())?;
-
     // Use absolute paths to project root for SSH keys to ensure they can be found by Ansible
     let project_root = std::env::current_dir().expect("Failed to get current directory");
     let ssh_private_key_path = project_root.join("fixtures/testing_rsa");
     let ssh_public_key_path = project_root.join("fixtures/testing_rsa.pub");
-    let ssh_user = Username::new("torrust").expect("Valid hardcoded username");
-    let ssh_credentials = SshCredentials::new(
-        ssh_private_key_path.clone(),
-        ssh_public_key_path.clone(),
-        ssh_user.clone(),
-    );
 
-    let ssh_port = DEFAULT_SSH_PORT;
-    let environment = Environment::new(environment_name, ssh_credentials, ssh_port);
+    // Cleanup any artifacts from previous test runs BEFORE creating the environment
+    // This prevents "environment already exists" errors from stale state
+    // We do this before CreateCommandHandler because it checks if environment exists in repository
+    cleanup_previous_test_data("e2e-full").map_err(|e| anyhow::anyhow!("{e}"))?;
+
+    // Create repository factory and clock for environment creation
+    let repository_factory = RepositoryFactory::new(Duration::from_secs(30));
+    let clock: Arc<dyn Clock> = Arc::new(SystemClock);
+
+    // Create environment via CreateCommandHandler
+    let environment = run_create_command(
+        &repository_factory,
+        clock,
+        "e2e-full",
+        ssh_private_key_path.to_string_lossy().to_string(),
+        ssh_public_key_path.to_string_lossy().to_string(),
+        "torrust",
+        DEFAULT_SSH_PORT,
+    )
+    .map_err(|e| anyhow::anyhow!("{e}"))?;
 
     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
+    // 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();

src/testing/e2e/tasks/container/preflight_cleanup.rs

@@ -8,7 +8,8 @@
 use crate::shared::command::CommandExecutor;
 use crate::testing::e2e::context::TestContext;
 use crate::testing::e2e::tasks::preflight_cleanup::{
-    cleanup_build_directory, cleanup_templates_directory, PreflightCleanupError,
+    cleanup_build_directory, cleanup_data_environment, cleanup_templates_directory,
+    PreflightCleanupError,
 };
 use tracing::{info, warn};
 
@@ -44,6 +45,9 @@ pub fn preflight_cleanup_previous_resources(
     // Clean the templates directory to ensure fresh embedded template extraction for E2E tests
     cleanup_templates_directory(env)?;
 
+    // Clean the data directory to ensure fresh environment state for E2E tests
+    cleanup_data_environment(env)?;
+
     // Clean up any hanging Docker containers from interrupted test runs
     cleanup_hanging_docker_containers(env);
 

src/testing/e2e/tasks/mod.rs

@@ -9,6 +9,7 @@
 //! The tasks are organized by deployment target:
 //!
 //! ### Infrastructure-agnostic tasks (can be used with both containers and VMs):
+//! - `run_create_command` - Environment creation using `CreateCommandHandler`
 //! - `run_configure_command` - Infrastructure configuration via Ansible and playbook execution
 //! - `run_configuration_validation` - Configuration validation and testing
 //! - `run_test_command` - Deployment validation and testing
@@ -30,5 +31,6 @@ pub mod container;
 pub mod preflight_cleanup;
 pub mod run_configuration_validation;
 pub mod run_configure_command;
+pub mod run_create_command;
 pub mod run_test_command;
 pub mod virtual_machine;

src/testing/e2e/tasks/preflight_cleanup.rs

@@ -48,6 +48,100 @@ impl std::error::Error for PreflightCleanupError {
     }
 }
 
+// TODO: Refactor TestContext to eliminate the need for this workaround function
+//
+// Current issue: TestContext requires an Environment, but we need to clean up data
+// directories BEFORE creating the Environment (because CreateCommandHandler checks
+// if the environment already exists in the repository).
+//
+// Proposed solutions:
+// 1. Make Environment optional in TestContext (TestContext { environment: Option<Environment> })
+// 2. Move Environment out of TestContext (preferred - better separation of concerns)
+//
+// The second option is better because:
+// - TestContext should manage test infrastructure (services, config, temp directories)
+// - Environment is a domain entity that represents deployment state
+// - Separating them provides clearer responsibilities and easier testing
+//
+// After refactoring, we could eliminate this standalone function and have all cleanup
+// go through a single preflight_cleanup_previous_resources() that doesn't require
+// a fully initialized TestContext with an Environment.
+
+/// Cleans the data directory for a specific environment name before `TestContext` creation
+///
+/// This helper function removes the `data/{environment_name}` directory to prevent
+/// "environment already exists" errors when `CreateCommandHandler` checks the repository.
+/// Unlike `cleanup_data_environment`, this function works without a `TestContext` and is
+/// intended to be called early in the test setup before environment creation.
+///
+/// # 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 environments from previous test runs don't interfere.
+///
+/// # Arguments
+///
+/// * `environment_name` - The name of the environment to clean up
+///
+/// # Returns
+///
+/// Returns `Ok(())` if cleanup succeeds or if the directory doesn't exist.
+///
+/// # Errors
+///
+/// Returns a `PreflightCleanupError::ResourceConflicts` error if the data directory
+/// cannot be removed due to permission issues or file locks.
+pub fn cleanup_previous_test_data(environment_name: &str) -> Result<(), PreflightCleanupError> {
+    use std::path::Path;
+
+    let data_dir = Path::new("data").join(environment_name);
+
+    if !data_dir.exists() {
+        info!(
+            operation = "preflight_data_cleanup",
+            status = "clean",
+            path = %data_dir.display(),
+            "No previous data directory found, skipping cleanup"
+        );
+        return Ok(());
+    }
+
+    info!(
+        operation = "preflight_data_cleanup",
+        path = %data_dir.display(),
+        "Cleaning data directory from previous test run"
+    );
+
+    match std::fs::remove_dir_all(&data_dir) {
+        Ok(()) => {
+            info!(
+                operation = "preflight_data_cleanup",
+                status = "success",
+                path = %data_dir.display(),
+                "Data directory cleaned successfully"
+            );
+            Ok(())
+        }
+        Err(e) => {
+            warn!(
+                operation = "preflight_data_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 the build directory to ensure fresh template state for E2E tests
 ///
 /// This function removes the build directory if it exists, ensuring that
@@ -192,3 +286,79 @@ pub fn cleanup_templates_directory(
         }
     }
 }
+
+/// 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. This prevents "environment already exists"
+/// errors and ensures proper test isolation.
+///
+/// # 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 environment state for each test run.
+///
+/// # Arguments
+///
+/// * `test_context` - The test context containing the environment configuration
+///
+/// # 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.
+pub fn cleanup_data_environment(test_context: &TestContext) -> Result<(), PreflightCleanupError> {
+    use std::path::Path;
+
+    // Construct the data directory path: data/{environment_name}
+    let data_dir = Path::new("data").join(test_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
+                ),
+            })
+        }
+    }
+}