Skip to content

Commit 05e06be

Browse files
committed
Merge #244: feat: [#242] enhance provision command output with connection details
24c7c97 docs: add refactoring plan to simplify controller handler creation (Jose Celano) baa125c docs: [#242] update rustdoc for display_connection_details method (Jose Celano) Pull request description: ## Summary Enhances the `provision` command to display SSH connection details after successful infrastructure provisioning. This provides users with immediate, actionable information to connect to their newly provisioned instances. Closes #242 ## Changes ### New Features - **Connection Details Display**: After successful provisioning, users now see: - Instance IP address - SSH port - SSH private key path - SSH username - Ready-to-copy SSH connection command ### Architecture Implements a clean **MVC pattern** with proper **DDD layering**: - **View Layer** (`src/presentation/views/commands/provision/`): - `ConnectionDetailsView`: Renders formatted output - `ConnectionDetailsData`: DTO for connection information - Follows presentation → domain dependency (DDD compliant) - **Controller** (`src/presentation/controllers/provision/handler.rs`): - Orchestrates functional pipeline: `Environment<Provisioned>` → `ConnectionDetailsData` → `String` → stdout - Uses `From` trait for clean DTO conversion - Proper error propagation with `?` operator ### Code Quality - **Law of Demeter**: Uses environment convenience methods (`ssh_port()`, `ssh_username()`) - **Functional Style**: Pipeline composition eliminates intermediate variables - **String Templates**: Uses `format!` macro for efficient rendering - **Error Handling**: Proper `Result` propagation throughout the stack - **Missing IP Handling**: Gracefully displays warning (not error) if IP unavailable ### Testing - **5 unit tests** for view rendering: - Complete details with IP - Warning display when IP missing - Custom SSH ports - Absolute path handling - Path preservation - **4 controller tests** (existing): - Invalid environment name - Empty environment name - Nonexistent environment - Valid environment name All tests passing ✅ ### Output Example ``` Instance Connection Details: IP Address: 10.140.190.171 SSH Port: 22 SSH Private Key: /home/user/.ssh/deploy_key SSH Username: torrust Connect using: ssh -i /home/user/.ssh/deploy_key [email protected] -p 22 ``` ## Technical Decisions 1. **From Trait Placement**: Implemented in presentation layer (not domain) to maintain DDD - domain should not depend on presentation DTOs 2. **Output Channel**: Uses `ProgressReporter::result()` to route to stdout (not stderr) since connection details are final command results, not progress information 3. **View Separation**: Created new `commands/` directory structure for command-specific views, maintaining clear organization 4. **Functional Pipeline**: Used functional composition for clarity and to eliminate intermediate variables ## Quality Checks All pre-commit checks passed: - ✅ Linting (markdown, yaml, toml, cspell, clippy, rustfmt, shellcheck) - ✅ Dependencies (cargo machete) - ✅ Tests (unit + integration + E2E) - ✅ Documentation (cargo doc) ## Related Documentation - User Guide: [docs/user-guide/commands/](docs/user-guide/commands/) - Architecture: [docs/codebase-architecture.md](docs/codebase-architecture.md) - DDD Layer Placement: [docs/contributing/ddd-layer-placement.md](docs/contributing/ddd-layer-placement.md) - Output Handling: [docs/contributing/output-handling.md](docs/contributing/output-handling.md) ACKs for top commit: josecelano: ACK 24c7c97 Tree-SHA512: 038d0fac718a40e9b4f88a97de392b2219062f79bdbc376f18e66d8bd5b6c41d8a4482aaedf10bbfda31666bfc08499f1342d27101a26b8bc1fccbd4c3ecb44a
2 parents 8902f01 + 24c7c97 commit 05e06be

File tree

7 files changed

+606
-3
lines changed

7 files changed

+606
-3
lines changed
Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
# Active Refactoring Plans
22

3-
| Document | Status | Issue | Target | Created |
4-
| ------------------------------------------------------------- | ----------- | ----- | ---------------------------------- | ---------- |
5-
| [Secret Type Introduction](plans/secret-type-introduction.md) | 📋 Planning | TBD | Secret handling for sensitive data | 2025-12-17 |
3+
| Document | Status | Issue | Target | Created |
4+
| ----------------------------------------------------------------------------------------------------- | ----------- | ----- | ---------------------------------- | ---------- |
5+
| [Secret Type Introduction](plans/secret-type-introduction.md) | 📋 Planning | TBD | Secret handling for sensitive data | 2025-12-17 |
6+
| [Simplify Controller Command Handler Creation](plans/simplify-controller-command-handler-creation.md) | 📋 Planning | TBD | Remove unnecessary progress steps | 2025-12-17 |
Lines changed: 275 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,275 @@
1+
# Simplify Controller Command Handler Creation
2+
3+
## 📋 Overview
4+
5+
Remove unnecessary progress step for application command handler creation across all presentation layer controllers. Handler creation is instantaneous (just object construction with no I/O), so users don't need visibility into this internal implementation detail.
6+
7+
**Target Files:**
8+
9+
- `src/presentation/controllers/provision/handler.rs`
10+
- `src/presentation/controllers/configure/handler.rs`
11+
- `src/presentation/controllers/register/handler.rs`
12+
- `src/presentation/controllers/release/handler.rs`
13+
- `src/presentation/controllers/run/handler.rs`
14+
- Other controllers following similar pattern
15+
16+
**Scope:**
17+
18+
- Remove `CreateCommandHandler` step from `ProvisionStep` enum (and similar in other controllers)
19+
- Move handler creation inside the method that uses it (e.g., `provision_infrastructure`)
20+
- Simplify execute methods to focus on user-visible operations
21+
- Maintain consistent pattern across all controllers
22+
23+
## 📊 Progress Tracking
24+
25+
**Total Active Proposals**: 1
26+
**Total Postponed**: 0
27+
**Total Discarded**: 0
28+
**Completed**: 0
29+
**In Progress**: 0
30+
**Not Started**: 1
31+
32+
### Phase Summary
33+
34+
- **Phase 0 - Simplify All Controllers (Medium Impact, Low Effort)**: ⏳ 0/1 completed (0%)
35+
36+
### Discarded Proposals
37+
38+
None
39+
40+
### Postponed Proposals
41+
42+
None
43+
44+
## 🎯 Key Problems Identified
45+
46+
### 1. Unnecessary Progress Visibility
47+
48+
Controllers currently report handler creation as a separate step, but:
49+
50+
- Handler creation is instantaneous (no I/O, just object construction)
51+
- Users don't need visibility into this implementation detail
52+
- It clutters the progress output with non-meaningful steps
53+
- Creates coupling between execute method and internal implementation
54+
55+
### 2. Inconsistent Encapsulation
56+
57+
The handler is created in the `execute` method but only used within a single sub-method:
58+
59+
```rust
60+
pub async fn execute(&mut self, environment_name: &str) -> Result<...> {
61+
let env_name = self.validate_environment_name(environment_name)?;
62+
63+
let handler = self.create_command_handler()?; // Created here
64+
65+
let provisioned = self.provision_infrastructure(&handler, &env_name).await?; // Only used here
66+
67+
// ...
68+
}
69+
```
70+
71+
Better encapsulation would move handler creation inside `provision_infrastructure`.
72+
73+
## 🚀 Refactoring Phases
74+
75+
---
76+
77+
## Phase 0: Simplify All Controllers (Highest Priority)
78+
79+
Remove unnecessary progress steps and improve encapsulation by moving handler creation closer to where it's used.
80+
81+
### Proposal #1: Consolidate Handler Creation Across All Controllers
82+
83+
**Status**: ⏳ Not Started
84+
**Impact**: 🟢🟢 Medium
85+
**Effort**: 🔵 Low
86+
**Priority**: P0
87+
**Depends On**: None
88+
89+
#### Problem
90+
91+
All controllers follow the same pattern of creating application command handlers as a separate step with progress reporting:
92+
93+
**Before (current pattern in provision controller):**
94+
95+
```rust
96+
/// Steps in the provision workflow
97+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
98+
enum ProvisionStep {
99+
ValidateEnvironment,
100+
CreateCommandHandler, // ← Unnecessary step
101+
ProvisionInfrastructure,
102+
}
103+
104+
pub async fn execute(&mut self, environment_name: &str) -> Result<...> {
105+
let env_name = self.validate_environment_name(environment_name)?;
106+
107+
let handler = self.create_command_handler()?; // ← Separate method
108+
109+
let provisioned = self.provision_infrastructure(&handler, &env_name).await?;
110+
111+
self.complete_workflow(environment_name)?;
112+
self.display_connection_details(&provisioned)?;
113+
114+
Ok(provisioned)
115+
}
116+
117+
fn create_command_handler(&mut self) -> Result<...> {
118+
self.progress.start_step(ProvisionStep::CreateCommandHandler.description())?;
119+
let handler = ProvisionCommandHandler::new(self.clock.clone(), self.repository.clone());
120+
self.progress.complete_step(None)?;
121+
Ok(handler)
122+
}
123+
124+
async fn provision_infrastructure(
125+
&mut self,
126+
handler: &ProvisionCommandHandler, // ← Handler passed as parameter
127+
env_name: &EnvironmentName,
128+
) -> Result<...> {
129+
self.progress.start_step(ProvisionStep::ProvisionInfrastructure.description())?;
130+
let provisioned = handler.execute(env_name).await?;
131+
self.progress.complete_step(Some("Infrastructure provisioned"))?;
132+
Ok(provisioned)
133+
}
134+
```
135+
136+
**Issues:**
137+
138+
- Handler creation is not a meaningful user-visible step
139+
- Handler is created in one place but only used in one other place
140+
- Progress output is cluttered with internal details
141+
- Similar pattern repeated across all controllers
142+
143+
#### Proposed Solution
144+
145+
**After (simplified pattern):**
146+
147+
```rust
148+
/// Steps in the provision workflow
149+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
150+
enum ProvisionStep {
151+
ValidateEnvironment,
152+
ProvisionInfrastructure, // ← Only user-visible steps
153+
}
154+
155+
pub async fn execute(&mut self, environment_name: &str) -> Result<...> {
156+
let env_name = self.validate_environment_name(environment_name)?;
157+
158+
// Handler creation moved inside provision_infrastructure
159+
let provisioned = self.provision_infrastructure(&env_name).await?;
160+
161+
self.complete_workflow(environment_name)?;
162+
self.display_connection_details(&provisioned)?;
163+
164+
Ok(provisioned)
165+
}
166+
167+
// create_command_handler method removed
168+
169+
async fn provision_infrastructure(
170+
&mut self,
171+
env_name: &EnvironmentName, // ← Simpler signature
172+
) -> Result<...> {
173+
self.progress.start_step(ProvisionStep::ProvisionInfrastructure.description())?;
174+
175+
// Handler creation encapsulated here
176+
let handler = ProvisionCommandHandler::new(self.clock.clone(), self.repository.clone());
177+
178+
let provisioned = handler.execute(env_name).await?;
179+
self.progress.complete_step(Some("Infrastructure provisioned"))?;
180+
Ok(provisioned)
181+
}
182+
```
183+
184+
#### Rationale
185+
186+
1. **Better Encapsulation**: Handler creation is moved closer to where it's used
187+
2. **Cleaner Progress**: Only meaningful steps visible to users
188+
3. **Simpler Code**: Fewer methods, simpler signatures, clearer intent
189+
4. **Consistent Pattern**: Apply same simplification across all controllers
190+
191+
#### Benefits
192+
193+
- ✅ Reduces clutter in progress output
194+
- ✅ Improves code organization and encapsulation
195+
- ✅ Simplifies controller execute methods
196+
- ✅ Makes the code more maintainable
197+
- ✅ Reduces line count across all controllers
198+
199+
#### Implementation Checklist
200+
201+
Apply to all controllers in this order (to catch any issues early):
202+
203+
- [ ] Provision controller (`src/presentation/controllers/provision/handler.rs`)
204+
- [ ] Configure controller (`src/presentation/controllers/configure/handler.rs`)
205+
- [ ] Register controller (`src/presentation/controllers/register/handler.rs`)
206+
- [ ] Release controller (`src/presentation/controllers/release/handler.rs`)
207+
- [ ] Run controller (`src/presentation/controllers/run/handler.rs`)
208+
- [ ] Destroy controller (if it follows the same pattern)
209+
- [ ] Test controller (if it follows the same pattern)
210+
211+
For each controller:
212+
213+
1. Remove `CreateCommandHandler` variant from the step enum
214+
2. Update `count()` method to reflect new step count
215+
3. Remove `create_command_handler` method
216+
4. Move handler creation inside the method that uses it
217+
5. Update progress reporter step count in constructor
218+
6. Run tests to verify behavior unchanged
219+
7. Verify progress output still clear and informative
220+
221+
- [ ] Verify all tests pass after each controller update
222+
- [ ] Run linter and fix any issues
223+
- [ ] Update any affected documentation
224+
225+
#### Testing Strategy
226+
227+
For each controller:
228+
229+
1. Run existing unit tests to verify behavior unchanged
230+
2. Manually test progress output to ensure it's clear
231+
3. Verify step numbering is correct (e.g., "Step 1/2" instead of "Step 1/3")
232+
233+
No new tests needed - existing tests validate the behavior remains correct.
234+
235+
#### Expected Results
236+
237+
Per controller:
238+
239+
- **Lines Removed**: ~15-20 (entire method + enum variant)
240+
- **Lines Added**: ~1 (handler creation in existing method)
241+
- **Net Change**: -14 to -19 lines per controller
242+
- **Total Net Change**: ~-70 to -95 lines across 5 controllers
243+
244+
---
245+
246+
## 📈 Timeline
247+
248+
- **Start Date**: TBD
249+
- **Target Completion**: 1-2 hours (simple refactoring)
250+
- **Approach**: Incremental - one controller at a time with test verification
251+
252+
## 🎓 Principles Alignment
253+
254+
This refactoring aligns with:
255+
256+
- **Observability**: Users see only meaningful progress steps
257+
- **Maintainability**: Simpler code with better encapsulation
258+
- **Clean Code**: Each method has clear, focused responsibility
259+
260+
## 📚 Related Documentation
261+
262+
- [Development Principles](../../development-principles.md) - Observability and maintainability
263+
- [DDD Layer Placement](../../contributing/ddd-layer-placement.md) - Controller responsibilities
264+
265+
## 💡 Notes
266+
267+
- This is a pure refactoring - no behavior changes
268+
- Pattern discovered during implementation of issue #242 (connection details display)
269+
- Consider documenting this pattern in controller guidelines after implementation
270+
271+
---
272+
273+
**Created**: December 17, 2025
274+
**Status**: 📋 Planning
275+
**Related Issues**: None yet - discovered during #242 implementation

src/presentation/controllers/provision/handler.rs

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@ use crate::domain::environment::name::EnvironmentName;
1313
use crate::domain::environment::repository::EnvironmentRepository;
1414
use crate::domain::environment::state::Provisioned;
1515
use crate::domain::environment::Environment;
16+
use crate::presentation::views::commands::provision::connection_details::{
17+
ConnectionDetailsData, ConnectionDetailsView,
18+
};
1619
use crate::presentation::views::progress::ProgressReporter;
1720
use crate::presentation::views::UserOutput;
1821
use crate::shared::clock::Clock;
@@ -131,6 +134,8 @@ impl ProvisionCommandController {
131134

132135
self.complete_workflow(environment_name)?;
133136

137+
self.display_connection_details(&provisioned)?;
138+
134139
Ok(provisioned)
135140
}
136141

@@ -215,6 +220,51 @@ impl ProvisionCommandController {
215220
.complete(&format!("Environment '{name}' provisioned successfully"))?;
216221
Ok(())
217222
}
223+
224+
/// Display connection details after successful provisioning
225+
///
226+
/// Orchestrates a functional pipeline to display SSH connection information:
227+
/// `Environment<Provisioned>` → `ConnectionDetailsData` → `String` → stdout
228+
///
229+
/// The output is written to stdout (not stderr) as it represents the final
230+
/// command result rather than progress information.
231+
///
232+
/// # MVC Architecture
233+
///
234+
/// Following the MVC pattern with functional composition:
235+
/// - Model: `Environment<Provisioned>` (domain model)
236+
/// - DTO: `ConnectionDetailsData::from()` (data transformation)
237+
/// - View: `ConnectionDetailsView::render()` (formatting)
238+
/// - Controller (this method): Orchestrates the pipeline
239+
/// - Output: `ProgressReporter::result()` (routing to stdout)
240+
///
241+
/// # Arguments
242+
///
243+
/// * `provisioned` - The provisioned environment containing connection details
244+
///
245+
/// # Missing IP Handling
246+
///
247+
/// If the instance IP is missing (which should not happen after successful
248+
/// provisioning), the view displays a warning but does not cause an error.
249+
/// This is intentional - the controller's responsibility is to display
250+
/// information, not to validate state (that's the domain/application layer's job).
251+
///
252+
/// # Errors
253+
///
254+
/// Returns `ProvisionSubcommandError` if the `ProgressReporter` encounters
255+
/// a mutex error while writing to stdout.
256+
#[allow(clippy::result_large_err)]
257+
fn display_connection_details(
258+
&mut self,
259+
provisioned: &Environment<Provisioned>,
260+
) -> Result<(), ProvisionSubcommandError> {
261+
// Pipeline: Environment<Provisioned> → DTO → render → output to stdout
262+
self.progress.result(&ConnectionDetailsView::render(
263+
&ConnectionDetailsData::from(provisioned),
264+
))?;
265+
266+
Ok(())
267+
}
218268
}
219269

220270
#[cfg(test)]
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
//! Command-specific views
2+
//!
3+
//! This module contains view components organized by command.
4+
//! Each command has its own submodule with views for rendering
5+
//! command-specific output.
6+
7+
pub mod provision;

0 commit comments

Comments
 (0)